Issue Details

Number

28457

Title

consensus: Better document ComputeMerkleRoot & add test for return value

Description

At least twice we've had attempts to refactor [`ComputeMerkleRoot`](https://github.com/bitcoin/bitcoin/blob/fd69ffbbfb3e08b474b33540e56cf4f81e5c21d4/src/consensus/merkle.cpp#L45) (#22046 & #22046). While the value of any sort of refactor is dubious, it may also come with subtle, unintended side-effects, [see review comment here](https://github.com/bitcoin/bitcoin/pull/28430#issuecomment-1714504535): > 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()](https://github.com/bitcoin/bitcoin/blob/8f7b9eb8711fdb32e8bdb59d2a7462a46c7a8086/src/validation.cpp#L3534-L3542) https://github.com/bitcoin/bitcoin/blob/8f7b9eb8711fdb32e8bdb59d2a7462a46c7a8086/src/validation.cpp#L3534-L3542 would now return "bad-txnmrklroot" as an error instead of "bad-txns-duplicate", because now the first check already fails. This seems incorrect. It might be good to better document this code, and add a test that would fail if a similar refactor is attempted in future.

URL

https://github.com/bitcoin/bitcoin/issue/28457

Closed by

Notes

Attention of

Kill Factor (1-10)

Save

Back to List