Merge bitcoin#30237: test: Add Compact Block Encoding test ReceiveWithExtraTransactions covering non-empty extra_txn

55eea00 test: Make blockencodings_tests deterministic (AngusP)
4c99301 test: Add ReceiveWithExtraTransactions Compact Block receive test. (AngusP)
4621e7c test: refactor: Rename extra_txn to const empty_extra_txn as it is empty in all test cases (AngusP)

Pull request description:

  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 bitcoin#29752 (bf031a5) where the `extra_txn` vec/circular-buffer was null-initialized and not yet filled when dereferenced in `PartiallyDownloadedBlock::InitData`.

ACKs for top commit:
  marcofleon:
    Code review ACK 55eea00. I ran the `blockencodings` unit test and no issues with the new test case.
  dergoegge:
    Code review ACK 55eea00
  glozow:
    ACK 55eea00

Tree-SHA512: d7909c212bb069e1f6184b26390a5000dcc5f2b18e49b86cceccb9f1ec4f874dd43bc9bc92abd4207c71dd78112ba58400042c230c42e93afe55ba51b943262c

Author: glozow

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

src/blockencodings.h

@@ -106,10 +106,15 @@ class CBlockHeaderAndShortTxIDs {
 
     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;
 
@@ -141,7 +146,7 @@ class PartiallyDownloadedBlock {
 
     explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {}
 
-    // extra_txn is a list of extra transactions to look at, in <witness hash, reference> form
+    // extra_txn is a list of extra orphan/conflicted/etc transactions to look at
     ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<CTransactionRef>& extra_txn);
     bool IsTxAvailable(size_t index) const;
     ReadStatus FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing);

src/net_processing.cpp

@@ -2124,7 +2124,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);
 
@@ -2522,7 +2522,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 {
@@ -5984,7 +5984,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;

src/test/blockencodings_tests.cpp

@@ -14,31 +14,36 @@
 
 #include <boost/test/unit_test.hpp>
 
-std::vector<CTransactionRef> extra_txn;
+const std::vector<CTransactionRef> 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(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);
@@ -58,15 +63,16 @@ 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]));
     BOOST_CHECK_EQUAL(pool.get(block.vtx[2]->GetHash()).use_count(), SHARED_TX_OFFSET + 0);
 
     // Do a simple ShortTxIDs RT
     {
-        CBlockHeaderAndShortTxIDs shortIDs{block};
+        CBlockHeaderAndShortTxIDs shortIDs{block, rand_ctx.rand64()};
 
         DataStream stream{};
         stream << shortIDs;
@@ -75,7 +81,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));
@@ -123,8 +129,8 @@ class TestHeaderAndShortIDs {
         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{};
@@ -141,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]));
@@ -151,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);
@@ -165,7 +172,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));
@@ -211,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]));
@@ -221,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
@@ -235,7 +243,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));
@@ -261,17 +269,14 @@ 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;
+    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;
@@ -281,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;
@@ -290,7 +295,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;
@@ -302,6 +307,53 @@ BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest)
     }
 }
 
+BOOST_AUTO_TEST_CASE(ReceiveWithExtraTransactions) {
+    CTxMemPool& pool = *Assert(m_node.mempool);
+    TestMemPoolEntryHelper entry;
+    auto rand_ctx(FastRandomContext(uint256{42}));
+
+    CMutableTransaction mtx = BuildTransactionTestCase();
+    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);
+    // 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, rand_ctx.rand64()};
+        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();

src/test/fuzz/partially_downloaded_block.cpp

@@ -52,7 +52,7 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
         return;
     }
 
-    CBlockHeaderAndShortTxIDs cmpctblock{*block};
+    CBlockHeaderAndShortTxIDs cmpctblock{*block, fuzzed_data_provider.ConsumeIntegral<uint64_t>()};
 
     bilingual_str error;
     CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error};