From 4621e7cc8f8e2b71393a2b30d5dbe56165bfb854 Mon Sep 17 00:00:00 2001 From: AngusP Date: Wed, 22 May 2024 21:06:35 +0100 Subject: test: refactor: Rename extra_txn to const empty_extra_txn as it is empty in all test cases --- src/test/blockencodings_tests.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp index 05355fb21d..2388fed881 100644 --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -14,7 +14,7 @@ #include -std::vector extra_txn; +const std::vector empty_extra_txn; BOOST_FIXTURE_TEST_SUITE(blockencodings_tests, RegTestingSetup) @@ -75,7 +75,7 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest) stream >> shortIDs2; PartiallyDownloadedBlock partialBlock(&pool); - BOOST_CHECK(partialBlock.InitData(shortIDs2, extra_txn) == READ_STATUS_OK); + BOOST_CHECK(partialBlock.InitData(shortIDs2, empty_extra_txn) == READ_STATUS_OK); BOOST_CHECK( partialBlock.IsTxAvailable(0)); BOOST_CHECK(!partialBlock.IsTxAvailable(1)); BOOST_CHECK( partialBlock.IsTxAvailable(2)); @@ -165,7 +165,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest) stream >> shortIDs2; PartiallyDownloadedBlock partialBlock(&pool); - BOOST_CHECK(partialBlock.InitData(shortIDs2, extra_txn) == READ_STATUS_OK); + BOOST_CHECK(partialBlock.InitData(shortIDs2, empty_extra_txn) == READ_STATUS_OK); BOOST_CHECK(!partialBlock.IsTxAvailable(0)); BOOST_CHECK( partialBlock.IsTxAvailable(1)); BOOST_CHECK( partialBlock.IsTxAvailable(2)); @@ -235,7 +235,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest) stream >> shortIDs2; PartiallyDownloadedBlock partialBlock(&pool); - BOOST_CHECK(partialBlock.InitData(shortIDs2, extra_txn) == READ_STATUS_OK); + BOOST_CHECK(partialBlock.InitData(shortIDs2, empty_extra_txn) == READ_STATUS_OK); BOOST_CHECK( partialBlock.IsTxAvailable(0)); BOOST_CHECK( partialBlock.IsTxAvailable(1)); BOOST_CHECK( partialBlock.IsTxAvailable(2)); @@ -290,7 +290,7 @@ BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest) stream >> shortIDs2; PartiallyDownloadedBlock partialBlock(&pool); - BOOST_CHECK(partialBlock.InitData(shortIDs2, extra_txn) == READ_STATUS_OK); + BOOST_CHECK(partialBlock.InitData(shortIDs2, empty_extra_txn) == READ_STATUS_OK); BOOST_CHECK(partialBlock.IsTxAvailable(0)); CBlock block2; -- cgit v1.2.3 From 4c99301220ab44e98d0d0e1cc8d774d96a25b7aa Mon Sep 17 00:00:00 2001 From: AngusP Date: Tue, 4 Jun 2024 19:27:30 +0100 Subject: test: Add ReceiveWithExtraTransactions Compact Block receive test. 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`. --- src/blockencodings.h | 2 +- src/test/blockencodings_tests.cpp | 60 ++++++++++++++++++++++++++++++++++----- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/src/blockencodings.h b/src/blockencodings.h index 2b1fabadd6..01594db527 100644 --- a/src/blockencodings.h +++ b/src/blockencodings.h @@ -141,7 +141,7 @@ public: explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {} - // extra_txn is a list of extra transactions to look at, in form + // extra_txn is a list of extra orphan/conflicted/etc transactions to look at ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector& extra_txn); bool IsTxAvailable(size_t index) const; ReadStatus FillBlock(CBlock& block, const std::vector& vtx_missing); diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp index 2388fed881..ff854c16eb 100644 --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -18,13 +18,18 @@ const std::vector empty_extra_txn; BOOST_FIXTURE_TEST_SUITE(blockencodings_tests, RegTestingSetup) -static CBlock BuildBlockTestCase() { - CBlock block; +static CMutableTransaction BuildTransactionTestCase() { CMutableTransaction tx; tx.vin.resize(1); tx.vin[0].scriptSig.resize(10); tx.vout.resize(1); tx.vout[0].nValue = 42; + return tx; +} + +static CBlock BuildBlockTestCase() { + CBlock block; + CMutableTransaction tx = BuildTransactionTestCase(); block.vtx.resize(3); block.vtx[0] = MakeTransactionRef(tx); @@ -261,11 +266,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest) BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest) { CTxMemPool& pool = *Assert(m_node.mempool); - CMutableTransaction coinbase; - coinbase.vin.resize(1); - coinbase.vin[0].scriptSig.resize(10); - coinbase.vout.resize(1); - coinbase.vout[0].nValue = 42; + CMutableTransaction coinbase = BuildTransactionTestCase(); CBlock block; block.vtx.resize(1); @@ -302,6 +303,51 @@ 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); + + CMutableTransaction mtx = BuildTransactionTestCase(); + mtx.vin[0].prevout.hash = Txid::FromUint256(InsecureRand256()); + mtx.vin[0].prevout.n = 0; + const CTransactionRef non_block_tx = MakeTransactionRef(std::move(mtx)); + + 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); + // Ensure the non_block_tx is actually not in the block + for (const auto &block_tx : block.vtx) { + BOOST_CHECK_NE(block_tx->GetHash(), non_block_tx->GetHash()); + } + // Ensure block.vtx[1] is not in pool + BOOST_CHECK_EQUAL(pool.get(block.vtx[1]->GetHash()), nullptr); + + { + const CBlockHeaderAndShortTxIDs cmpctblock{block}; + PartiallyDownloadedBlock partial_block(&pool); + PartiallyDownloadedBlock partial_block_with_extra(&pool); + + BOOST_CHECK(partial_block.InitData(cmpctblock, extra_txn) == READ_STATUS_OK); + BOOST_CHECK( partial_block.IsTxAvailable(0)); + BOOST_CHECK(!partial_block.IsTxAvailable(1)); + BOOST_CHECK( partial_block.IsTxAvailable(2)); + + // Add an unrelated tx to extra_txn: + extra_txn[0] = non_block_tx; + // and a tx from the block that's not in the mempool: + extra_txn[1] = block.vtx[1]; + + BOOST_CHECK(partial_block_with_extra.InitData(cmpctblock, extra_txn) == READ_STATUS_OK); + BOOST_CHECK(partial_block_with_extra.IsTxAvailable(0)); + // This transaction is now available via extra_txn: + BOOST_CHECK(partial_block_with_extra.IsTxAvailable(1)); + BOOST_CHECK(partial_block_with_extra.IsTxAvailable(2)); + } +} + BOOST_AUTO_TEST_CASE(TransactionsRequestSerializationTest) { BlockTransactionsRequest req1; req1.blockhash = InsecureRand256(); -- cgit v1.2.3 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