From 55eea003af24169c883e1761beb997e151845225 Mon Sep 17 00:00:00 2001 From: AngusP Date: Wed, 12 Jun 2024 22:09:15 +0100 Subject: 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 ``` --- src/blockencodings.cpp | 4 +-- src/blockencodings.h | 9 ++++-- src/net_processing.cpp | 6 ++-- src/test/blockencodings_tests.cpp | 44 ++++++++++++++++------------ 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 -CBlockHeaderAndShortTxIDs::CBlockHeaderAndShortTxIDs(const CBlock& block) : - nonce(GetRand()), +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 &blo */ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { - auto pcmpctblock = std::make_shared(*pblock); + auto pcmpctblock = std::make_shared(*pblock, GetRand()); 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()}; 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()}; 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 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 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()}; CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)}; PartiallyDownloadedBlock pdb{&pool}; -- cgit v1.2.3