ReceiveWithExtraTransactions covering non-empty extra_txn
#30237
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 (bf031a517c79cec5b43420bcd40291ab0e9f68a8) where the extra_txn vec/circular-buffer was null-initialized and not yet filled when dereferenced in PartiallyDownloadedBlock::InitData.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | marcofleon, dergoegge, glozow |
| Concept ACK | instagibbs |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
This new test uses the `vExtraTxnForCompact` (`extra_txn`) vector of
optional orphan/conflicted/etc. transactions to provide a transaction
in a compact block that was not otherwise present in our mempool.
This also covers an improbable nullptr deref bug addressed in
bf031a517c79cec5b43420bcd40291ab0e9f68a8 (#29752) where the
`extra_txn` vec/circular-buffer was sometimes null-initialized and
not yet filled when dereferenced in `PartiallyDownloadedBlock::InitData`.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
336+ BOOST_CHECK( partial_block.IsTxAvailable(2));
337+
338+ // Add an unrelated tx to extra_txn:
339+ extra_txn[0] = non_block_tx;
340+ // and a tx from the block that's not in the mempool:
341+ extra_txn[1] = block.vtx[1];
L325 has pool.addUnchecked(entry.FromTx(block.vtx[2])); which adds a tx from the block to the mempool, giving .IsTxAvailable(2) (.IsTxAvailable(0) should always be true as that’s the coinbase which is in the compact block header, IIUC).
SimpleRoundTripTest and a couple others in here also already covered the add to mempool case.
Code review ACK 4c99301220ab44e98d0d0e1cc8d774d96a25b7aa
I’ve verified that this can reproduce the nullptr deref bug by reverting a8203e94123b6ea6e4f4a6320e3ad20457f44a28 and making short id collisions more likely (see top commits on: https://github.com/dergoegge/bitcoin/commits/2024-06-cmpctblk-extra-txn-test/).
0$ n=0; while (( n++ < 100 )); do src/test/test_bitcoin --run_test=blockencodings_tests/ReceiveWithExtraTransactions ; done
1...
2Running 1 test case...
3unknown location(0): fatal error: in "blockencodings_tests/ReceiveWithExtraTransactions": memory access violation at address: 0x00000059: no mapping at fault address
4../../src/test/blockencodings_tests.cpp(333): last checkpoint
5
6*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
7...
While it might be extremely rare, I believe this test is flaky due to possible short id collisions. Flakes can be observed by cherry picking just https://github.com/dergoegge/bitcoin/commit/540fb4ed526839c10ea8b746505276d04e92fb6f (i.e. increasing the likelyhood of collisions):
0$ n=0; while (( n++ < 100 )); do src/test/test_bitcoin --run_test=blockencodings_tests/ReceiveWithExtraTransactions ; done
1...
2Running 1 test case...
3../../src/test/blockencodings_tests.cpp(347): error: in "blockencodings_tests/ReceiveWithExtraTransactions": check partial_block_with_extra.IsTxAvailable(2) has failed
4
5*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
6...
7Running 1 test case...
8../../src/test/blockencodings_tests.cpp(333): error: in "blockencodings_tests/ReceiveWithExtraTransactions": check partial_block.InitData(cmpctblock, extra_txn) == READ_STATUS_OK has failed
9../../src/test/blockencodings_tests.cpp(336): error: in "blockencodings_tests/ReceiveWithExtraTransactions": check partial_block.IsTxAvailable(2) has failed
10../../src/test/blockencodings_tests.cpp(343): error: in "blockencodings_tests/ReceiveWithExtraTransactions": check partial_block_with_extra.InitData(cmpctblock, extra_txn) == READ_STATUS_OK has failed
11../../src/test/blockencodings_tests.cpp(346): error: in "blockencodings_tests/ReceiveWithExtraTransactions": check partial_block_with_extra.IsTxAvailable(1) has failed
12../../src/test/blockencodings_tests.cpp(347): error: in "blockencodings_tests/ReceiveWithExtraTransactions": check partial_block_with_extra.IsTxAvailable(2) has failed
13
14*** 5 failures are detected in the test module "Bitcoin Core Test Suite"
Not sure if that is worth addressing since it’s so rare but perhaps simply adding a comment that the test might fail every 2^48 runs is good?
Not sure if that is worth addressing since it’s so rare but perhaps simply adding a comment that the test might fail every 2^48 runs is good?
I think it’s worth avoiding, I don’t really like the notion of flaky tests maybe failing for a random reason, even if it is very improbable. I’d guess short-ID collisions would also affect some of the other existing tests in here so there’s additional benefit in them not being flaky too.
Concept ACK
Not sure if that is worth addressing since it’s so rare but perhaps simply adding a comment that the test might fail every 2^48 runs is good?
I think it’s worth avoiding, I don’t really like the notion of flaky tests maybe failing for a random reason, even if it is very improbable. I’d guess short-ID collisions would also affect some of the other existing tests in here so there’s additional benefit in them not being flaky too.
Not sure exactly how flaky it is and don’t want to blow this out of proportion. Comment sounds good to save a future debugger from headache. If we might hit it, you can knock it down closer to 0 by running it e.g. 5 times and asserting that it doesn’t fail every time, like in coinselector_tests: https://github.com/bitcoin/bitcoin/blob/4a020ca443ba370bf41583962d16aa8551876f53/src/wallet/test/coinselector_tests.cpp#L752-L777
Would it be difficult to make the test deterministic?
Yes, the straightforward way would be to replace the InsecureRand256() with fixed hashes that don’t result in transaction short IDs that collide. Less-straightforward but maybe better would be to keep using random hashes but check for a short ID collision and try again with a different random hash until the collision is gone – or start with one random hash and change it in a way that guarantees the short ID (siphash) will be different, but I’m not sure if siphash is easily ‘attackable’ in that sense.
InsecureRand256
I’d say longer them those global non-deterministic functions should go away anyway. Not something you need to do here, but if you want you can create a commit to accept a FastRandomContext reference as param to BuildBlockTestCase, then in each unit test, create a local FastRandomContext and bind it to a lambda named BuildBlockTestCase.
Pushed 09b90c6 which makes the tests deterministic by using predictable ‘random’ numbers. I’ve jsut picked them, there’s nothing particularly special about their values.
Using this to reproduce the issue @dergoegge pointed out:
0diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
1index 695e8d806a..64d635a97a 100644
2--- a/src/blockencodings.cpp
3+++ b/src/blockencodings.cpp
4@@ -44,7 +44,8 @@ void CBlockHeaderAndShortTxIDs::FillShortTxIDSelector() const {
5
6 uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const Wtxid& wtxid) const {
7 static_assert(SHORTTXIDS_LENGTH == 6, "shorttxids calculation assumes 6-byte shorttxids");
8- return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0xffffffffffffL;
9+ // return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0xffffffffffffL;
10+ return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0x0f;
11 }
and then running
0set -e;
1n=0;
2while (( n++ < 5000 )); do
3 src/test/test_bitcoin --run_test=blockencodings_tests;
4done
the failures were either READ_STATUS_FAILED from InitData, or IsTxAvailable(...) == false when we were expecting true.
On the above-patched 09b90c6, I’ve not seen any failures with 0x0f short IDs and ~10k re-runs of all the blockencodings tests.
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.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.
108@@ -109,6 +109,9 @@ class CBlockHeaderAndShortTxIDs {
109 // Dummy for deserialization
110 CBlockHeaderAndShortTxIDs() {}
111
112+ // Unsafe init with fixed nonce for testing/internal-use only
113+ CBlockHeaderAndShortTxIDs(const CBlock& block, const uint64_t nonce);
114+
115 CBlockHeaderAndShortTxIDs(const CBlock& block);
FastRandomContext and getting a rand64() for the nonce that way? peerman would pass in its own `m_rng. Making the rng a param feels preferable to adding a test-only version of the ctor.
uint64_t nonce, as then it doesn’t “impose” how you got the nonce; in peerman, m_rng has locks guarding it, so using GetRand<uint64_t>() is easier (and claims to be threadsafe), plus this matches the previous behaviour where the CBlockHeaderAndShortTxIDs constructor would just call GetRand itself, rather than switching to a FastRandomContext
285@@ -282,7 +286,7 @@ BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest)
286
287 // Test simple header round-trip with only coinbase
288 {
289- CBlockHeaderAndShortTxIDs shortIDs{block};
290+ CBlockHeaderAndShortTxIDs shortIDs{block, 1341};
rand_ctx.rand64()
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
108@@ -109,7 +109,7 @@ class CBlockHeaderAndShortTxIDs {
109 // Dummy for deserialization
110 CBlockHeaderAndShortTxIDs() {}
111
112- CBlockHeaderAndShortTxIDs(const CBlock& block);
113+ CBlockHeaderAndShortTxIDs(const CBlock& block, const uint64_t nonce);
50@@ -50,7 +51,8 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
51 return;
52 }
53
54- CBlockHeaderAndShortTxIDs cmpctblock{*block};
55+ FastRandomContext fast_random_context{ConsumeUInt256(fuzzed_data_provider)};
56+ CBlockHeaderAndShortTxIDs cmpctblock{*block, fast_random_context.rand64()};
No need for the actual rng, we can just consume the nonce from the fuzz data.
0 CBlockHeaderAndShortTxIDs cmpctblock{*block, fuzzed_data_provider.ConsumeIntegral<uint64_t>()};
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
```
blockencodings unit test and no issues with the new test case.
2548@@ -2549,7 +2549,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
2549 if (a_recent_compact_block && a_recent_compact_block->header.GetHash() == pindex->GetBlockHash()) {
2550 MakeAndPushMessage(pfrom, NetMsgType::CMPCTBLOCK, *a_recent_compact_block);
2551 } else {
2552- CBlockHeaderAndShortTxIDs cmpctblock{*pblock};
2553+ CBlockHeaderAndShortTxIDs cmpctblock{*pblock, GetRand<uint64_t>()};
PeerManagerImpl::m_rng though it would require taking off the g_msgproc_mutex guard.