aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAngusP <angus@toaster.cc>2024-06-12 22:09:15 +0100
committerAngusP <angus@toaster.cc>2024-06-19 22:56:30 +0100
commit55eea003af24169c883e1761beb997e151845225 (patch)
treed6cc5564dbea79a85029c8d94658e15502cf993b
parent4c99301220ab44e98d0d0e1cc8d774d96a25b7aa (diff)
downloadbitcoin-55eea003af24169c883e1761beb997e151845225.tar.xz
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.cpp4
-rw-r--r--src/blockencodings.h9
-rw-r--r--src/net_processing.cpp6
-rw-r--r--src/test/blockencodings_tests.cpp44
-rw-r--r--src/test/fuzz/partially_downloaded_block.cpp2
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};