Skip to content

Commit

Permalink
Merge bitcoin#30237: test: Add Compact Block Encoding test `ReceiveWi…
Browse files Browse the repository at this point in the history 
…thExtraTransactions` 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
  • Loading branch information
@glozow
glozow committed Jul 1, 2024
2 parents 4c573e5 + 55eea00 commit 0bd2bd1
Show file tree
Hide file tree
Showing 5 changed files with 91 additions and 34 deletions.
4 changes: 2 additions & 2 deletions src/blockencodings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 8 additions & 3 deletions src/blockencodings.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions src/net_processing.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down
102 changes: 77 additions & 25 deletions src/test/blockencodings_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
Expand All @@ -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));
Expand Down Expand Up @@ -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{};
Expand All @@ -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]));
Expand All @@ -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);
Expand All @@ -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));
Expand Down Expand Up @@ -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]));
Expand All @@ -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
Expand All @@ -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));
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion src/test/fuzz/partially_downloaded_block.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down

0 comments on commit 0bd2bd1

Please sign in to comment.