In the presence of smaller transactions on the network, blocks can sustain a
higher relay rate than 7tx/second. In this event, the per-peer inventory queues
can grow too large.
This commit bumps the rate up to 14 tx/s (for inbound peers), increasing the
safety margin by a factor of 2.
Outbound peers continue to receive relayed transactions at 2.5x the rate of
inbound peers, for a rate of 35tx/second.
Co-Authored-By: Suhas Daftuar <sdaftuar@gmail.com>
Github-Pull: #28592
Rebased-From: b81f37031c8f2ccad9346f1b65ee0f8083c44796
49d569cb1f p2p: For assumeutxo, download snapshot chain before background chain (Martin Zumsande)
7a885518d5 p2p: Restrict downloading of blocks for snapshot chain (Martin Zumsande)
Pull request description:
After loading a snapshot, `pindexLastCommonBlock` is usually already set to some block for existing peers. That means we'd continue syncing the background chain from those peers instead of prioritising the snapshot chain, which defeats the purpose of doing assumeutxo in the first place. Only existing peers are affected by this bug.
ACKs for top commit:
fjahr:
re-ACK 49d569cb1f
achow101:
ACK 49d569cb1f
Sjors:
tACK 49d569cb1f
Tree-SHA512: 0eaebe1c29a8510d5ced57e14c09b128ccb34b491692815291df68bf12e2a15b52b1e7bf8d9f34808904e7f7bc20f70b0ad0f7e14df93bbdf456bd12cc02a5d2
After loading a snapshot, pindexLastCommonBlock is usually already set
to some block for existing peers. That means we'd continue syncing the
background chain from those peers instead of prioritising the snapshot
chain, which defeats the purpose of doing assumeutxo in the first place.
Only existing peers are affected by this bug.
If the best chain of the peer doesn't include the snapshot
block, it is futile to download blocks from this chain,
because we couldn't reorg to it. We'd also crash
trying to reorg because this scenario is not handled.
Now that m_txrequest and m_recent_confirmed_transactions are guarded by
the same mutex, there is no benefit to processing them separately.
Instead, just loop through pblock->vtx once.
Resetting m_recent_rejects once per block is more efficient than
comparing hashRecentRejectsChainTip with the chain tip every time we
call AlreadyHaveTx. We keep hashRecentRejectsChainTip for now to assert
that updates happen correctly; it is removed in the next commit.
We need to synchronize between various tx download structures.
TxRequest does not inherently need cs_main for synchronization, and it's
not appropriate to lock all of the tx download logic under cs_main.
16bd283b3a Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner)
0dbcd4c148 net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner)
66673f1c13 net: fix race condition in self-connect detection (Sebastian Falbesoner)
Pull request description:
This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368).
Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](bd5d1688b4/src/net.cpp (L2923-L2930)) method):
1. set up node state
2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](bd5d1688b4/src/net_processing.cpp (L1662-L1683)))
3. add new node to vector `m_nodes`
If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`).
Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`.
The temporarily reverted test introduced in #30362 is readded. Fixes#30368.
Thanks go to vasild, mzumsande and dergoegge for suggestions on how to fix this (see https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200625017 ff. and https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789).
ACKs for top commit:
naiyoma:
tested ACK [16bd283b3a), built and tested locally, test passes successfully.
mzumsande:
ACK 16bd283b3a
tdb3:
ACK 16bd283b3a
glozow:
ACK 16bd283b3a
dergoegge:
ACK 16bd283b3a
Tree-SHA512: 5b8aced6cda8deb38d4cd3fe4980b8af505d37ffa0925afaa734c5d81efe9d490dc48a42e1d0d45dd2961c0e1172a3d5b6582ae9a2d642f2592a17fbdc184445
5b7f70ba26 test: loadtxoutset in divergent chain with less work (Alfonso Roman Zubeldia)
d35efe1efc p2p: Start downloading historical blocks from common ancestor (Martin Zumsande)
Pull request description:
This PR adds a test to cover the scenario of loading an assumeutxo snapshot when the current chain tip is not an ancestor of the snapshot block but has less work.
During the review process, a bug was discovered where blocks between the last common ancestor and the background tip were not being requested if the background tip was not an ancestor of the snapshot block. mzumsande suggested a fix (65343ec49a6b73c4197dfc38e1c2f433b0a3838a) to start downloading historical blocks from the last common ancestor to address this issue. This fix has been incorporated into the PR with a slight modification.
Related to https://github.com/bitcoin/bitcoin/issues/28648
ACKs for top commit:
fjahr:
tACK 5b7f70ba26
achow101:
ACK 5b7f70ba26
mzumsande:
Code Review ACK 5b7f70ba26
Tree-SHA512: f8957349686a6a1292165ea9e0fd8c912d21466072632a10f8ef9d852a5f430bc6b2a531e6884a4dbf2e3adb28b3d512b25919e78f5804a67320ef54c3b1aaf6
6ecda04fef random: drop ad-hoc Shuffle in favor of std::shuffle (Pieter Wuille)
da28a26aae bench random: benchmark more functions, and add InsecureRandomContext (Pieter Wuille)
0a9bbc64c1 random bench refactor: move to new bench/random.cpp (Pieter Wuille)
Pull request description:
This adds benchmarks for various operations on `FastRandomContext` and `InsecureRandomContext`, and then removes the ad-hoc `Shuffle` functions, now that it appears that standard library `std::shuffle` has comparable performance. The other reason for keeping `Shuffle`, namely the fact that libstdc++ used self-move (which debug mode panics on) has been fixed as well (see https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1658344049).
ACKs for top commit:
achow101:
ACK 6ecda04fef
hodlinator:
ACK 6ecda04fef
dergoegge:
Code review ACK 6ecda04fef
Tree-SHA512: 2560b7312410581ff2b9bd0716e0f1558d910b5eadb9544785c972384985ac0f11f72d6b2797cfe2e7eb71fa57c30cffd98cc009cb4ee87a18b1524694211417
Now that the queueing of the VERSION messages has been moved out of
`InitializeNode`, there is no need to pass a mutable `CNode` reference any
more. With a const reference, trying to send messages in this method would
lead to a compile-time error, e.g.:
----------------------------------------------------------------------------------------------------------------------------------
...
net_processing.cpp: In member function ‘virtual void {anonymous}::PeerManagerImpl::InitializeNode(const CNode&, ServiceFlags)’:
net_processing.cpp:1683:21: error: binding reference of type ‘CNode&’ to ‘const CNode’ discards qualifiers
1683 | PushNodeVersion(node, *peer);
...
----------------------------------------------------------------------------------------------------------------------------------
Initiating an outbound network connection currently involves the
following steps after the socket connection is established (see
`CConnman::OpenNetworkConnection` method):
1. set up node state
2. queue VERSION message
3. add new node to vector `m_nodes`
If we connect to ourself, it can happen that the sent VERSION message
(step 2) is received and processed locally *before* the node object
is added to the connection manager's `m_nodes` vector (step 3). In this
case, the self-connect remains undiscovered, as the detection doesn't
find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`).
Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion`
call out of `InitializeNode` and doing that in the `SendMessages` method
instead, which is only called for `CNode` instances in `m_nodes`.
Thanks go to vasild, mzumsande, dergoegge and sipa for suggestions on
how to fix this.
There are only a few call sites of these throughout the codebase, so
move the functionality into FastRandomContext, and rewrite all call sites.
This requires the callers to explicit construct FastRandomContext objects,
which do add to the verbosity, but also make potentially apparent locations
where the code can be improved by reusing a FastRandomContext object (see
further commit).
55eea003af test: Make blockencodings_tests deterministic (AngusP)
4c99301220 test: Add ReceiveWithExtraTransactions Compact Block receive test. (AngusP)
4621e7cc8f test: refactor: Rename extra_txn to const empty_extra_txn as it is empty in all test cases (AngusP)
Pull request description:
This test uses the `extra_txn` (`vExtraTxnForCompact`) vector of optional orphan/conflicted/etc. transactions to provide transactions to a PartiallyDownloadedBlock that are not otherwise present in the mempool, and check that they are used.
This also covers a former nullptr deref bug that was fixed in #29752 (bf031a517c) where the `extra_txn` vec/circular-buffer was null-initialized and not yet filled when dereferenced in `PartiallyDownloadedBlock::InitData`.
ACKs for top commit:
marcofleon:
Code review ACK 55eea003af. I ran the `blockencodings` unit test and no issues with the new test case.
dergoegge:
Code review ACK 55eea003af
glozow:
ACK 55eea003af
Tree-SHA512: d7909c212bb069e1f6184b26390a5000dcc5f2b18e49b86cceccb9f1ec4f874dd43bc9bc92abd4207c71dd78112ba58400042c230c42e93afe55ba51b943262c
Otherwise, if the background tip is not an ancestor of the snapshot, blocks in between that ancestor up to the height of the background tip will never be requested.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Co-authored-by: Alfonso Roman Zubeldia <19962151+alfonsoromanz@users.noreply.github.com>
6eecba475e net_processing: make MaybePunishNodeFor{Block,Tx} return void (Pieter Wuille)
ae60d485da net_processing: remove Misbehavior score and increments (Pieter Wuille)
6457c31197 net_processing: make all Misbehaving increments = 100 (Pieter Wuille)
5120ab1478 net_processing: drop 8 headers threshold for incoming BIP130 (Pieter Wuille)
944c54290d net_processing: drop Misbehavior for unconnecting headers (Pieter Wuille)
9f66ac7cf1 net_processing: do not treat non-connecting headers as response (Pieter Wuille)
Pull request description:
So far, discouragement of peers triggers when their misbehavior score exceeds 100 points. Most types of misbehavior increment the score by 100, triggering immediate discouragement, but some types do not. This PR makes all increments equal to either 100 (meaning any misbehavior will immediately cause disconnection and discouragement) or 0 (making the behavior effectively unconditionally allowed), and then removes the logic for score accumulation.
This simplifies the code a bit, but also makes protocol expectations clearer: if a peer misbehaves, they get disconnected. There is no good reason why certain types of protocol violations should be permitted 4 times (howmuch=20) or 9 times (howmuch=10), while many others are never allowed. Furthermore, the distinction between these looks arbitrary.
The specific types of misbehavior that are changed to 100 are:
* Sending us a `block` which does not connect to our header tree (which necessarily must have been unsollicited). [used to be score 10]
* Sending us a `headers` with a non-continuous headers sequence. [used to be score 20]
* Sending us more than 1000 addresses in a single `addr` or `addrv2` message [used to be score 20]
* Sending us more than 50000 invs in a single `inv` message [used to be score 20]
* Sending us more than 2000 headers in a single `headers` message [used to be score 20]
The specific types of misbehavior that are changed to 0 are:
* Sending us 10 (*) separate BIP130 headers announcements that do not connect to our block tree [used to be score 20]
* Sending us more than 8 headers in a single `headers` message (which thus does not get treated as a BIP130 announcement) that does not connect to our block tree. [used to be score 10]
I believe that none of these behaviors are unavoidable, except for the one marked (*) which can in theory happen still due to interaction between BIP130 and variations in system clocks (the max 2 hour in the future rule). This one has been removed entirely. In order to remove the impact of the bug it was designed to deal with, without relying on misbehavior, a separate improvement is included that makes `getheaders`-tracking more accurate.
In another unrelated improvement, this also gets rid of the 8 header limit heuristic to determine whether an incoming non-connecting `headers` is a potential BIP130 announcement, as this rule is no longer needed to prevent spurious Misbehavior. Instead, any non-connecting `headers` is now treated as a potential announcement.
ACKs for top commit:
sr-gi:
ACK [6eecba4](6eecba475e)
achow101:
ACK 6eecba475e
mzumsande:
Code Review ACK 6eecba475e
glozow:
light code review / concept ACK 6eecba475e
Tree-SHA512: e11e8a652c4ec048d8961086110a3594feefbb821e13f45c14ef81016377be0db44b5311751ef635d6e026def1960aff33f644e78ece11cfb54f2b7daa96f946
refactor: CBlockHeaderAndShortTxIDs constructor now always takes an explicit nonce.
test: Make blockencodings_tests deterministic using fixed seed providing deterministic
CBlockHeaderAndShortTxID nonces and dummy transaction IDs.
Fixes very rare flaky test failures, where the ShortIDs of test transactions collide, leading to
`READ_STATUS_FAILED` from PartiallyDownloadedBlock::InitData and/or `IsTxAvailable` giving `false`
when the transaction should actually be available.
* Use a new `FastRandomContext` with a fixed seed in each test, to ensure 'random' uint256s
used as fake prevouts are deterministic, so in-turn test txids and short IDs are deterministic
and don't collide causing very rare but flaky test failures.
* Add new test-only/internal initializer for `CBlockHeaderAndShortTxIDs` that takes a specified
nonce to further ensure determinism and avoid rare but undesireable short ID collisions.
In a test context this nonce is set to a fixed known-good value. Normally it is random, as
previously.
Flaky test failures can be reproduced with:
```patch
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index 695e8d806a..64d635a97a 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -44,7 +44,8 @@ void CBlockHeaderAndShortTxIDs::FillShortTxIDSelector() const {
uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const Wtxid& wtxid) const {
static_assert(SHORTTXIDS_LENGTH == 6, "shorttxids calculation assumes 6-byte shorttxids");
- return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0xffffffffffffL;
+ // return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0xffffffffffffL;
+ return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0x0f;
}
```
to increase the likelihood of a short ID collision; and running
```shell
set -e;
n=0;
while (( n++ < 5000 )); do
src/test/test_bitcoin --run_test=blockencodings_tests;
done
```
