fix: unnecessary continuation after finding mutation #28430

1. 
   120c0 commented at 3:16 am on September 8, 2023: none

   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.

2. fix: unnecessary continuation after finding mutation 42b25bbd93
3. 
   DrahtBot commented at 3:16 am on September 8, 2023: contributor

   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.

4. 
   jonatack commented at 9:15 pm on September 8, 2023: member

   ACK 42b25bbd9397b8b2fe16f8f2e70547a04543eb16
5. 
   mzumsande commented at 2:05 am on September 9, 2023: contributor

   #22046 was a similar PR, just want to link to that discussion. This PR seems equivalent to “Tweak 1” from #22046#pullrequestreview-667658308.
6. luke-jr approved
7. 
   luke-jr commented at 5:40 pm on September 11, 2023: member

   crACK, though I wonder if it actually makes a noteworthy improvement?
8. 
   mzumsande commented at 8:06 pm on September 11, 2023: contributor

   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() 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. 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 #22046

9. 
   fanquake commented at 8:12 am on September 12, 2023: member

   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.
10. fanquake closed this on Sep 12, 2023

    ---

11. 
    maflcko commented at 8:21 am on September 12, 2023: member

    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?

12. 
    maflcko commented at 8:22 am on September 12, 2023: member

    Also, a test would be good that fails on the changes in this pull request.
13. 
    jonatack commented at 11:04 am on September 12, 2023: member

    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.

14. 
    mzumsande commented at 2:13 pm on September 12, 2023: contributor

    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.

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

    I retreat the NACK, though I’m still skeptical if we should change this kind of code without a measurable performance benefit.
16. luke-jr referenced this in commit c8ed62aa4d on Sep 16, 2023
17. bitcoin locked this on Sep 11, 2024

Contributors
120c0 DrahtBot jonatack mzumsande luke-jr fanquake maflcko