diff options
author | AngusP <angus@toaster.cc> | 2024-06-12 22:09:15 +0100 |
---|---|---|
committer | AngusP <angus@toaster.cc> | 2024-06-19 22:56:30 +0100 |
commit | 55eea003af24169c883e1761beb997e151845225 (patch) | |
tree | d6cc5564dbea79a85029c8d94658e15502cf993b | |
parent | 4c99301220ab44e98d0d0e1cc8d774d96a25b7aa (diff) |
test: Make blockencodings_tests deterministic
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
```
-rw-r--r-- | src/blockencodings.cpp | 4 | ||||
-rw-r--r-- | src/blockencodings.h | 9 | ||||
-rw-r--r-- | src/net_processing.cpp | 6 | ||||
-rw-r--r-- | src/test/blockencodings_tests.cpp | 44 | ||||
-rw-r--r-- | src/test/fuzz/partially_downloaded_block.cpp | 2 |
5 files changed, 38 insertions, 27 deletions
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index 5a4513d281..2bcb4b0c3d 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -17,8 +17,8 @@ #include <unordered_map> -CBlockHeaderAndShortTxIDs::CBlockHeaderAndShortTxIDs(const CBlock& block) : - nonce(GetRand<uint64_t>()), +CBlockHeaderAndShortTxIDs::CBlockHeaderAndShortTxIDs(const CBlock& block, const uint64_t nonce) : + nonce(nonce), shorttxids(block.vtx.size() - 1), prefilledtxn(1), header(block) { FillShortTxIDSelector(); //TODO: Use our mempool prior to block acceptance to predictively fill more than just the coinbase diff --git a/src/blockencodings.h b/src/blockencodings.h index 01594db527..bc1d08ba5a 100644 --- a/src/blockencodings.h +++ b/src/blockencodings.h @@ -106,10 +106,15 @@ public: CBlockHeader header; - // Dummy for deserialization + /** + * Dummy for deserialization + */ CBlockHeaderAndShortTxIDs() {} - CBlockHeaderAndShortTxIDs(const CBlock& block); + /** + * @param[in] nonce This should be randomly generated, and is used for the siphash secret key + */ + CBlockHeaderAndShortTxIDs(const CBlock& block, const uint64_t nonce); uint64_t GetShortID(const Wtxid& wtxid) const; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6374cb52c1..a7b4452967 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2151,7 +2151,7 @@ void PeerManagerImpl::BlockDisconnected(const std::shared_ptr<const CBlock> &blo */ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock) { - auto pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs>(*pblock); + auto pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs>(*pblock, GetRand<uint64_t>()); LOCK(cs_main); @@ -2549,7 +2549,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& if (a_recent_compact_block && a_recent_compact_block->header.GetHash() == pindex->GetBlockHash()) { MakeAndPushMessage(pfrom, NetMsgType::CMPCTBLOCK, *a_recent_compact_block); } else { - CBlockHeaderAndShortTxIDs cmpctblock{*pblock}; + CBlockHeaderAndShortTxIDs cmpctblock{*pblock, GetRand<uint64_t>()}; MakeAndPushMessage(pfrom, NetMsgType::CMPCTBLOCK, cmpctblock); } } else { @@ -6033,7 +6033,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) CBlock block; const bool ret{m_chainman.m_blockman.ReadBlockFromDisk(block, *pBestIndex)}; assert(ret); - CBlockHeaderAndShortTxIDs cmpctblock{block}; + CBlockHeaderAndShortTxIDs cmpctblock{block, GetRand<uint64_t>()}; MakeAndPushMessage(*pto, NetMsgType::CMPCTBLOCK, cmpctblock); } state.pindexBestHeaderSent = pBestIndex; diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp index ff854c16eb..b0749c851c 100644 --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -27,23 +27,23 @@ static CMutableTransaction BuildTransactionTestCase() { return tx; } -static CBlock BuildBlockTestCase() { +static CBlock BuildBlockTestCase(FastRandomContext& ctx) { CBlock block; CMutableTransaction tx = BuildTransactionTestCase(); block.vtx.resize(3); block.vtx[0] = MakeTransactionRef(tx); block.nVersion = 42; - block.hashPrevBlock = InsecureRand256(); + block.hashPrevBlock = ctx.rand256(); block.nBits = 0x207fffff; - tx.vin[0].prevout.hash = Txid::FromUint256(InsecureRand256()); + tx.vin[0].prevout.hash = Txid::FromUint256(ctx.rand256()); tx.vin[0].prevout.n = 0; block.vtx[1] = MakeTransactionRef(tx); tx.vin.resize(10); for (size_t i = 0; i < tx.vin.size(); i++) { - tx.vin[i].prevout.hash = Txid::FromUint256(InsecureRand256()); + tx.vin[i].prevout.hash = Txid::FromUint256(ctx.rand256()); tx.vin[i].prevout.n = 0; } block.vtx[2] = MakeTransactionRef(tx); @@ -63,7 +63,8 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest) { CTxMemPool& pool = *Assert(m_node.mempool); TestMemPoolEntryHelper entry; - CBlock block(BuildBlockTestCase()); + auto rand_ctx(FastRandomContext(uint256{42})); + CBlock block(BuildBlockTestCase(rand_ctx)); LOCK2(cs_main, pool.cs); pool.addUnchecked(entry.FromTx(block.vtx[2])); @@ -71,7 +72,7 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest) // Do a simple ShortTxIDs RT { - CBlockHeaderAndShortTxIDs shortIDs{block}; + CBlockHeaderAndShortTxIDs shortIDs{block, rand_ctx.rand64()}; DataStream stream{}; stream << shortIDs; @@ -128,8 +129,8 @@ public: stream << orig; stream >> *this; } - explicit TestHeaderAndShortIDs(const CBlock& block) : - TestHeaderAndShortIDs(CBlockHeaderAndShortTxIDs{block}) {} + explicit TestHeaderAndShortIDs(const CBlock& block, FastRandomContext& ctx) : + TestHeaderAndShortIDs(CBlockHeaderAndShortTxIDs{block, ctx.rand64()}) {} uint64_t GetShortID(const Wtxid& txhash) const { DataStream stream{}; @@ -146,7 +147,8 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest) { CTxMemPool& pool = *Assert(m_node.mempool); TestMemPoolEntryHelper entry; - CBlock block(BuildBlockTestCase()); + auto rand_ctx(FastRandomContext(uint256{42})); + CBlock block(BuildBlockTestCase(rand_ctx)); LOCK2(cs_main, pool.cs); pool.addUnchecked(entry.FromTx(block.vtx[2])); @@ -156,7 +158,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest) // Test with pre-forwarding tx 1, but not coinbase { - TestHeaderAndShortIDs shortIDs(block); + TestHeaderAndShortIDs shortIDs(block, rand_ctx); shortIDs.prefilledtxn.resize(1); shortIDs.prefilledtxn[0] = {1, block.vtx[1]}; shortIDs.shorttxids.resize(2); @@ -216,7 +218,8 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest) { CTxMemPool& pool = *Assert(m_node.mempool); TestMemPoolEntryHelper entry; - CBlock block(BuildBlockTestCase()); + auto rand_ctx(FastRandomContext(uint256{42})); + CBlock block(BuildBlockTestCase(rand_ctx)); LOCK2(cs_main, pool.cs); pool.addUnchecked(entry.FromTx(block.vtx[1])); @@ -226,7 +229,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest) // Test with pre-forwarding coinbase + tx 2 with tx 1 in mempool { - TestHeaderAndShortIDs shortIDs(block); + TestHeaderAndShortIDs shortIDs(block, rand_ctx); shortIDs.prefilledtxn.resize(2); shortIDs.prefilledtxn[0] = {0, block.vtx[0]}; shortIDs.prefilledtxn[1] = {1, block.vtx[2]}; // id == 1 as it is 1 after index 1 @@ -269,10 +272,11 @@ BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest) CMutableTransaction coinbase = BuildTransactionTestCase(); CBlock block; + auto rand_ctx(FastRandomContext(uint256{42})); block.vtx.resize(1); block.vtx[0] = MakeTransactionRef(std::move(coinbase)); block.nVersion = 42; - block.hashPrevBlock = InsecureRand256(); + block.hashPrevBlock = rand_ctx.rand256(); block.nBits = 0x207fffff; bool mutated; @@ -282,7 +286,7 @@ BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest) // Test simple header round-trip with only coinbase { - CBlockHeaderAndShortTxIDs shortIDs{block}; + CBlockHeaderAndShortTxIDs shortIDs{block, rand_ctx.rand64()}; DataStream stream{}; stream << shortIDs; @@ -306,15 +310,17 @@ BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest) BOOST_AUTO_TEST_CASE(ReceiveWithExtraTransactions) { CTxMemPool& pool = *Assert(m_node.mempool); TestMemPoolEntryHelper entry; - const CBlock block(BuildBlockTestCase()); - std::vector<CTransactionRef> extra_txn; - extra_txn.resize(10); + auto rand_ctx(FastRandomContext(uint256{42})); CMutableTransaction mtx = BuildTransactionTestCase(); - mtx.vin[0].prevout.hash = Txid::FromUint256(InsecureRand256()); + mtx.vin[0].prevout.hash = Txid::FromUint256(rand_ctx.rand256()); mtx.vin[0].prevout.n = 0; const CTransactionRef non_block_tx = MakeTransactionRef(std::move(mtx)); + CBlock block(BuildBlockTestCase(rand_ctx)); + std::vector<CTransactionRef> extra_txn; + extra_txn.resize(10); + LOCK2(cs_main, pool.cs); pool.addUnchecked(entry.FromTx(block.vtx[2])); BOOST_CHECK_EQUAL(pool.get(block.vtx[2]->GetHash()).use_count(), SHARED_TX_OFFSET + 0); @@ -326,7 +332,7 @@ BOOST_AUTO_TEST_CASE(ReceiveWithExtraTransactions) { BOOST_CHECK_EQUAL(pool.get(block.vtx[1]->GetHash()), nullptr); { - const CBlockHeaderAndShortTxIDs cmpctblock{block}; + const CBlockHeaderAndShortTxIDs cmpctblock{block, rand_ctx.rand64()}; PartiallyDownloadedBlock partial_block(&pool); PartiallyDownloadedBlock partial_block_with_extra(&pool); diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp index 2bf47930f4..0c874c76e8 100644 --- a/src/test/fuzz/partially_downloaded_block.cpp +++ b/src/test/fuzz/partially_downloaded_block.cpp @@ -50,7 +50,7 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb) return; } - CBlockHeaderAndShortTxIDs cmpctblock{*block}; + CBlockHeaderAndShortTxIDs cmpctblock{*block, fuzzed_data_provider.ConsumeIntegral<uint64_t>()}; CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)}; PartiallyDownloadedBlock pdb{&pool}; |