Merge bitcoin/bitcoin#32646: p2p: Add witness mutation check inside FillBlock

28299ce776 p2p: remove vestigial READ_STATUS_CHECKBLOCK_FAILED (Greg Sanders)
bac9ee4830 p2p: Add witness mutation check inside FillBlock (Greg Sanders)

Pull request description:

  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`.

ACKs for top commit:
  Crypt-iQ:
    ACK 28299ce776
  achow101:
    ACK 28299ce776
  stratospher:
    ACK 28299ce7.
  dergoegge:
    Code review ACK 28299ce776

Tree-SHA512: 883d7c12ca096234b425e6fe12e46b0611607600916e6ac8d1c8112224aa76924b7b074754910163ac2ec15379075d618a9ece3642649ac7629cddb0d4e432ea
This commit is contained in:
merge-script
2025-06-30 13:15:37 -04:00
5 changed files with 38 additions and 71 deletions

View File

@@ -3360,7 +3360,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");
@@ -3377,23 +3381,7 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl
return;
}
} else {
// Block is either okay, or possibly we received
// READ_STATUS_CHECKBLOCK_FAILED.
// Note that CheckBlock can only fail for one of a few reasons:
// 1. bad-proof-of-work (impossible here, because we've already
// accepted the header)
// 2. merkleroot doesn't match the transactions given (already
// caught in FillBlock with READ_STATUS_FAILED, so
// impossible here)
// 3. the block is otherwise invalid (eg invalid coinbase,
// block is too big, too many legacy sigops, etc).
// So if CheckBlock failed, #3 is the only possibility.
// Under BIP 152, we don't discourage the peer unless proof of work is
// invalid (we don't require all the stateless checks to have
// been run). This is handled below, so just treat this as
// though the block was successfully read, and rely on the
// handling in ProcessNewBlock to ensure the block index is
// updated, etc.
// Block is okay for further processing
RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); // it is now an empty pointer
fBlockRead = true;
// mapBlockSource is used for potentially punishing peers and
@@ -4529,7 +4517,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;
}