consensus: Better document ComputeMerkleRoot & add test for return value #28457

1. 
   fanquake commented at 10:27 am on September 12, 2023: member

   At least twice we’ve had attempts to refactor ComputeMerkleRoot (#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:

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

2. 
   jonatack commented at 11:09 am on September 12, 2023: contributor

   subtle, unintended side-effects, see review comment here:

   2. If we break early and don’t compute the full merkle root anymore

   I’m not sure that review is correct. The break only exits the loop that sets the mutation localvar to save on needless further iterations. There shouldn’t be a change in behavior.

3. 
   mzumsande commented at 2:15 pm on September 12, 2023: contributor

   @jonatack is right, I misread the code, and the PR doesn’t change behavior. Might still be good to better document this code though.
4. 
   fanquake commented at 2:16 pm on September 12, 2023: member

   Might still be good to better document this code though.

   Yea. I don’t think we are going to make the suggested change in either case.

5. willcl-ark added the label Docs on Jan 24, 2024
6. willcl-ark added the label Tests on Jan 24, 2024
7. willcl-ark added the label Validation on Jan 24, 2024

Contributors
fanquake jonatack mzumsande

Labels
Docs Tests Validation