laantungir
/
bitcoin
mirror of https://github.com/bitcoin/bitcoin.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

p2p: Add witness mutation check inside FillBlock 

Since #29412, we have not allowed mutated blocks to continue
being processed immediately the block is received, but this
is only done for the legacy BLOCK message.

Extend these checks as belt-and-suspenders to not allow
similar mutation strategies to affect relay by honest peers
by applying the check inside
PartiallyDownloadedBlock::FillBlock, immediately before
returning READ_STATUS_OK.

This also removes the extraneous CheckBlock call.

Github-Pull: #32646
Rebased-From: bac9ee4830
This commit is contained in:
Greg Sanders 2025-05-21 13:36:43 -04:00 committed by fanquake
parent e97588fc3d
commit 9b95ab5e9d
No known key found for this signature in database
GPG Key ID: 2EEB9F5CC09526C1
5 changed files with 37 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
src/blockencodings.cpp
View File

@ -180,7 +180,7 @@ bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const
return txn_available[index] != nullptr;
}
ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing)
ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing, bool segwit_active)
{
if (header.IsNull()) return READ_STATUS_INVALID;
@ -205,16 +205,11 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
if (vtx_missing.size() != tx_missing_offset)
return READ_STATUS_INVALID;
BlockValidationState state;
CheckBlockFn check_block = m_check_block_mock ? m_check_block_mock : CheckBlock;
if (!check_block(block, state, Params().GetConsensus(), /*fCheckPoW=*/true, /*fCheckMerkleRoot=*/true)) {
// TODO: We really want to just check merkle tree manually here,
// but that is expensive, and CheckBlock caches a block's
// "checked-status" (in the CBlock?). CBlock should be able to
// check its own merkle root and cache that check.
if (state.GetResult() == BlockValidationResult::BLOCK_MUTATED)
return READ_STATUS_FAILED; // Possible Short ID collision
return READ_STATUS_CHECKBLOCK_FAILED;
// Check for possible mutations early now that we have a seemingly good block
IsBlockMutatedFn check_mutated{m_check_block_mutated_mock ? m_check_block_mutated_mock : IsBlockMutated};
if (check_mutated(/*block=*/block,
/*check_witness_root=*/segwit_active)) {
return READ_STATUS_FAILED; // Possible Short ID collision
}
LogDebug(BCLog::CMPCTBLOCK, "Successfully reconstructed block %s with %lu txn prefilled, %lu txn from mempool (incl at least %lu from extra pool) and %lu txn requested\n", hash.ToString(), prefilled_count, mempool_count, extra_count, vtx_missing.size());

7
src/blockencodings.h
View File

@ -141,15 +141,16 @@ public:
CBlockHeader header;
// Can be overridden for testing
using CheckBlockFn = std::function<bool(const CBlock&, BlockValidationState&, const Consensus::Params&, bool, bool)>;
CheckBlockFn m_check_block_mock{nullptr};
using IsBlockMutatedFn = std::function<bool(const CBlock&, bool)>;
IsBlockMutatedFn m_check_block_mutated_mock{nullptr};
explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {}
// 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);
// segwit_active enforces witness mutation checks just before reporting a healthy status
ReadStatus FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing, bool segwit_active);
};
#endif // BITCOIN_BLOCKENCODINGS_H

10
src/net_processing.cpp
View File

@ -3314,7 +3314,11 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl
}
PartiallyDownloadedBlock& partialBlock = *range_flight.first->second.second->partialBlock;
ReadStatus status = partialBlock.FillBlock(*pblock, block_transactions.txn);
// We should not have gotten this far in compact block processing unless it's attached to a known header
const CBlockIndex* prev_block{Assume(m_chainman.m_blockman.LookupBlockIndex(partialBlock.header.hashPrevBlock))};
ReadStatus status = partialBlock.FillBlock(*pblock, block_transactions.txn,
/*segwit_active=*/DeploymentActiveAfter(prev_block, m_chainman, Consensus::DEPLOYMENT_SEGWIT));
if (status == READ_STATUS_INVALID) {
RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); // Reset in-flight state in case Misbehaving does not result in a disconnect
Misbehaving(peer, "invalid compact block/non-matching block transactions");
@ -4462,7 +4466,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
return;
}
std::vector<CTransactionRef> dummy;
status = tempBlock.FillBlock(*pblock, dummy);
const CBlockIndex* prev_block{Assume(m_chainman.m_blockman.LookupBlockIndex(cmpctblock.header.hashPrevBlock))};
status = tempBlock.FillBlock(*pblock, dummy,
/*segwit_active=*/DeploymentActiveAfter(prev_block, m_chainman, Consensus::DEPLOYMENT_SEGWIT));
if (status == READ_STATUS_OK) {
fBlockReconstructed = true;
}

16
src/test/blockencodings_tests.cpp
View File

@ -95,21 +95,21 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
CBlock block2;
{
PartiallyDownloadedBlock tmp = partialBlock;
BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_INVALID); // No transactions
BOOST_CHECK(partialBlock.FillBlock(block2, {}, /*segwit_active=*/true) == READ_STATUS_INVALID); // No transactions
partialBlock = tmp;
}
// Wrong transaction
{
PartiallyDownloadedBlock tmp = partialBlock;
partialBlock.FillBlock(block2, {block.vtx[2]}); // Current implementation doesn't check txn here, but don't require that
partialBlock.FillBlock(block2, {block.vtx[2]}, /*segwit_active=*/true); // Current implementation doesn't check txn here, but don't require that
partialBlock = tmp;
}
bool mutated;
BOOST_CHECK(block.hashMerkleRoot != BlockMerkleRoot(block2, &mutated));
CBlock block3;
BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[1]}) == READ_STATUS_OK);
BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[1]}, /*segwit_active=*/true) == READ_STATUS_OK);
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block3.GetHash().ToString());
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString());
BOOST_CHECK(!mutated);
@ -182,14 +182,14 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
CBlock block2;
{
PartiallyDownloadedBlock tmp = partialBlock;
BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_INVALID); // No transactions
BOOST_CHECK(partialBlock.FillBlock(block2, {}, /*segwit_active=*/true) == READ_STATUS_INVALID); // No transactions
partialBlock = tmp;
}
// Wrong transaction
{
PartiallyDownloadedBlock tmp = partialBlock;
partialBlock.FillBlock(block2, {block.vtx[1]}); // Current implementation doesn't check txn here, but don't require that
partialBlock.FillBlock(block2, {block.vtx[1]}, /*segwit_active=*/true); // Current implementation doesn't check txn here, but don't require that
partialBlock = tmp;
}
BOOST_CHECK_EQUAL(pool.get(block.vtx[2]->GetHash()).use_count(), SHARED_TX_OFFSET + 2); // +2 because of partialBlock and block2
@ -198,7 +198,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
CBlock block3;
PartiallyDownloadedBlock partialBlockCopy = partialBlock;
BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[0]}) == READ_STATUS_OK);
BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[0]}, /*segwit_active=*/true) == READ_STATUS_OK);
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block3.GetHash().ToString());
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString());
BOOST_CHECK(!mutated);
@ -252,7 +252,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
CBlock block2;
PartiallyDownloadedBlock partialBlockCopy = partialBlock;
BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_OK);
BOOST_CHECK(partialBlock.FillBlock(block2, {}, /*segwit_active=*/true) == READ_STATUS_OK);
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block2.GetHash().ToString());
bool mutated;
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block2, &mutated).ToString());
@ -300,7 +300,7 @@ BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest)
CBlock block2;
std::vector<CTransactionRef> vtx_missing;
BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_OK);
BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing, /*segwit_active=*/true) == READ_STATUS_OK);
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block2.GetHash().ToString());
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block2, &mutated).ToString());
BOOST_CHECK(!mutated);

39
src/test/fuzz/partially_downloaded_block.cpp
View File

@ -32,14 +32,10 @@ void initialize_pdb()
g_setup = testing_setup.get();
}
PartiallyDownloadedBlock::CheckBlockFn FuzzedCheckBlock(std::optional<BlockValidationResult> result)
PartiallyDownloadedBlock::IsBlockMutatedFn FuzzedIsBlockMutated(bool result)
{
return [result](const CBlock&, BlockValidationState& state, const Consensus::Params&, bool, bool) {
if (result) {
return state.Invalid(*result);
}
return true;
return [result](const CBlock& block, bool) {
return result;
};
}
@ -111,36 +107,23 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
skipped_missing |= (!pdb.IsTxAvailable(i) && skip);
}
// Mock CheckBlock
bool fail_check_block{fuzzed_data_provider.ConsumeBool()};
auto validation_result =
fuzzed_data_provider.PickValueInArray(
{BlockValidationResult::BLOCK_RESULT_UNSET,
BlockValidationResult::BLOCK_CONSENSUS,
BlockValidationResult::BLOCK_CACHED_INVALID,
BlockValidationResult::BLOCK_INVALID_HEADER,
BlockValidationResult::BLOCK_MUTATED,
BlockValidationResult::BLOCK_MISSING_PREV,
BlockValidationResult::BLOCK_INVALID_PREV,
BlockValidationResult::BLOCK_TIME_FUTURE,
BlockValidationResult::BLOCK_CHECKPOINT,
BlockValidationResult::BLOCK_HEADER_LOW_WORK});
pdb.m_check_block_mock = FuzzedCheckBlock(
fail_check_block ?
std::optional<BlockValidationResult>{validation_result} :
std::nullopt);
bool segwit_active{fuzzed_data_provider.ConsumeBool()};
// Mock IsBlockMutated
bool fail_block_mutated{fuzzed_data_provider.ConsumeBool()};
pdb.m_check_block_mutated_mock = FuzzedIsBlockMutated(fail_block_mutated);
CBlock reconstructed_block;
auto fill_status{pdb.FillBlock(reconstructed_block, missing)};
auto fill_status{pdb.FillBlock(reconstructed_block, missing, segwit_active)};
switch (fill_status) {
case READ_STATUS_OK:
assert(!skipped_missing);
assert(!fail_check_block);
assert(!fail_block_mutated);
assert(block->GetHash() == reconstructed_block.GetHash());
break;
case READ_STATUS_CHECKBLOCK_FAILED: [[fallthrough]];
case READ_STATUS_FAILED:
assert(fail_check_block);
assert(fail_block_mutated);
break;
case READ_STATUS_INVALID:
break;