This was a beast of a method, with a lot of duplication and a condition trailing off into infinity. Now you can view it in one glance.
Also, exit once the output is found - no need to continue searching for what
is already found.
refactoring: Clean up GetWitnessCommitmentIndex #13267
pull Empact wants to merge 1 commits into bitcoin:master from Empact:witness-committment-index changing 1 files +11 −2-
Empact commented at 12:15 AM on May 18, 2018: member
- fanquake added the label Refactoring on May 18, 2018
- fanquake added the label Validation on May 18, 2018
-
sipa commented at 12:22 AM on May 18, 2018: member
If you're going to break after the first match you must search backwards (there can be multiple outputs matching the pattern; the last one counts).
-
sdaftuar commented at 12:23 AM on May 18, 2018: member
Concept NACK. I can understand the desire for easier to read code, but this kind of refactor in consensus logic is generally not worth the review burden.
In this case, exiting early -- a seemingly nice optimization -- would introduce a consensus bug(!).
- Empact force-pushed on May 18, 2018
- Empact renamed this:
refactoring: Clean up GetWitnessCommitmentIndex, and exit early on success
refactoring: Clean up GetWitnessCommitmentIndex
on May 18, 2018 -
Empact commented at 12:32 AM on May 18, 2018: member
Thanks guys, TIL. Updated to remove the early exit. Now it's strictly a refactor. Bonus comment to warn future readers.
-
110dfa1c55
Clean up GetWitnessCommitmentIndex
This was a beast of a method, with a lot of duplication and a condition trailing off into infinity. Now you can view it in one glance.
- Empact force-pushed on May 18, 2018
-
kallewoof commented at 5:25 AM on May 18, 2018: member
Concept ACK
The new code looks a LOT easier to read. Will utACK after double checking it does the same thing (it looks like it does though).
-
GreatSock commented at 10:48 PM on May 18, 2018: contributor
-
Empact commented at 8:38 AM on July 2, 2018: member
Closing due to NACK and the loads of other code in need of consideration.
- Empact closed this on Jul 2, 2018
- MarcoFalke locked this on Sep 8, 2021
Labels