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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32646.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/43189542399
LLM reason (✨ experimental): Lint errors caused the CI failure.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
217@@ -218,6 +218,13 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
218 return READ_STATUS_CHECKBLOCK_FAILED;
219 }
220
221+ // Belt-and-suspenders check for other mutations now that we have a seemingly good block
222+ if (IsBlockMutated(/*block=*/block,
check_block by not calling this function anymore? If I understand the comment correctly it only calls CheckBlock() to allow the caching of the check result and avoid repeated Merkle checks, which IsBlockMutated() also would do today?
CheckMerkleRoot() or CheckWitnessMalleation()) has failed.
Both merkle checks can fail due to collisions
Interesting, I did not know this. Since the header merkle check is first… if the witness merkle check fails due to collision, does that mean that the transaction that collided had the same txid but a different wtxid? Or is there another way for short ID collisions to affect the witness merkle check?
if the witness merkle check fails due to collision, does that mean that the transaction that collided had the same txid but a different wtxid?
That would be my understanding as well, i.e. the txs are only different in their witness. Pretty sure that is the only scenario, as otherwise the prior merkle root check would fail.
3359@@ -3360,7 +3360,11 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl
3360 }
3361
3362 PartiallyDownloadedBlock& partialBlock = *range_flight.first->second.second->partialBlock;
3363- ReadStatus status = partialBlock.FillBlock(*pblock, block_transactions.txn);
3364+
3365+ const CBlockIndex* prev_block{m_chainman.m_blockman.LookupBlockIndex(partialBlock.header.hashPrevBlock)};
3366+ Assume(prev_block); // we should not have gotten this far in compact block processing unless it's attached to a known header
nit
0 const CBlockIndex* prev_block{Assume(m_chainman.m_blockman.LookupBlockIndex(partialBlock.header.hashPrevBlock))};
218- return READ_STATUS_CHECKBLOCK_FAILED;
219+ // Check for possible mutations early now that we have a seemingly good block
220+ IsBlockMutatedFn check_mutated{m_check_block_mutated_mock ? m_check_block_mutated_mock : IsBlockMutated};
221+ if (check_mutated(/*block=*/block,
222+ /*check_witness_root=*/segwit_active)) {
223+ return READ_STATUS_FAILED; // Possible Short ID collision
READ_STATUS_CHECKBLOCK_FAILED? It’s unused now.
4516@@ -4529,7 +4517,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4517 return;
4518 }
4519 std::vector<CTransactionRef> dummy;
4520- status = tempBlock.FillBlock(*pblock, dummy);
4521+ const CBlockIndex* prev_block = m_chainman.m_blockman.LookupBlockIndex(cmpctblock.header.hashPrevBlock);
4522+ Assume(prev_block); // checked prior
130- std::nullopt);
131+ bool segwit_active{fuzzed_data_provider.ConsumeBool()};
132+
133+ // Mock IsBlockMutated
134+ bool fail_block_mutated{fuzzed_data_provider.ConsumeBool()};
135+ // True if txid merkle commitment will be found valid, false if both will not be marked valid
fail_block_mutated is true, doesn’t this mean the txid merkle commitment won’t be found valid? I might be misreading the comment.
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.