fix: unnecessary continuation after finding mutation

[120c0]

A simple increment so that when the mutation is successfully found, the break already exits the for to avoid an unnecessary continuation.

The concept is simple, this change should improve the performance of this function, providing a faster completion when meeting the given objective.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jonatack
Concept NACK	mzumsande
Concept ACK	luke-jr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[jonatack]

ACK 42b25bb

[mzumsande]

#22046 was a similar PR, just want to link to that discussion. This PR seems equivalent to "Tweak 1" from #22046 (review).

[luke-jr]

crACK, though I wonder if it actually makes a noteworthy improvement?

[mzumsande]

I tend to be concept NACK: unless I'm missing something, this does not improve performance in a meaningful way and makes error reporting wrong:

1. The added break only saves time if we receive a mutated block with repeated transactions. So this change isn't relevant unless a peer send us a block they mutated on purpose, in which case they get disconnected for that. So it really shouldn't change anything in a non-adversarial scenario.

2. If we break early and don't compute the full merkle root anymore, the return value of BlockMerkleRoot() changes. As a result, code calling BlockMerkleRoot()

   bitcoin/src/validation.cpp

   Lines 3534 to 3542 in 8f7b9eb

   	uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated);
   	if (block.hashMerkleRoot != hashMerkleRoot2)
   	return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txnmrklroot", "hashMerkleRoot mismatch");
   	// Check for merkle tree malleability (CVE-2012-2459): repeating sequences
   	// of transactions in a block without affecting the merkle root of a block,
   	// while still invalidating it.
   	if (mutated)
   	return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txns-duplicate", "duplicate transaction");

   would now return "bad-txnmrklroot" as an error instead of "bad-txns-duplicate", because now the first check already fails. This seems incorrect.
   this is wrong, see below.

3. I don't think it's wise to change consensus-critical code like this without a tangible benefit, see discussion in consensus: remove redundant checks in merkle root computation #22046

[fanquake]

Thanks for the PR, however we are going to leave this as-is for now. If you're interested in contributing, you can also checkout the "Good first issue" label.

[maflcko]

would now return "bad-txnmrklroot" as an error instead of "bad-txns-duplicate", because now the first check already fails. This seems incorrect.

I think this will come up again in the future, so it would probably make sense to add a comment why break can't be used here?

[maflcko]

Also, a test would be good that fails on the changes in this pull request.

[jonatack]

makes error reporting wrong:

2. If we break early and don't compute the full merkle root anymore, the return value of BlockMerkleRoot() changes.

I don’t think that’s right. The break only exits the loop that sets mutation to save on needless further iterations. There is no change in behavior.

[mzumsande]

I don’t think that’s right. The break only exits the loop that sets mutation to save on needless further iterations. There is no change in behavior.

Oh, you are right, I misread the code. For some reason I thought the break would refer to the while loop.

[mzumsande]

I retreat the NACK, though I'm still skeptical if we should change this kind of code without a measurable performance benefit.