refactor: Improve GetWitnessCommitmentIndex #19640

1. 
   promag commented at 11:16 pm on August 1, 2020: member

   GetWitnessCommitmentIndex finds the last output index of coinbase transaction. Currently iterates all outputs. This change reverses iteration and so it only iterates all outputs at worst case.
2. 
   promag commented at 11:18 pm on August 1, 2020: member

   Originally added in #8149 by @sipa. Got my attention while reviewing 3339d01af3e9c65f7a54b4f0e1c5194b5681dd71 from #18267.
3. promag force-pushed on Aug 1, 2020
4. refactor: Improve GetWitnessCommitmentIndex f4d5c7ce15
5. in src/validation.cpp:3409 in 3a25f18ce6 outdated

   3408                 vout.scriptPubKey[2] == 0xaa &&
   3409                 vout.scriptPubKey[3] == 0x21 &&
   3410                 vout.scriptPubKey[4] == 0xa9 &&
   3411                 vout.scriptPubKey[5] == 0xed) {
   3412-                commitpos = o;
   3413+                return o;
   

   ---

   ---

   

   ajtowns commented at 0:14 am on August 2, 2020:

   return o-1; ?

   ---

   

   promag commented at 0:28 am on August 2, 2020:

   Yup yup, forgot to fix this. Initially was bad because o is unsigned. Will fix.
6. promag force-pushed on Aug 2, 2020
7. DrahtBot added the label Refactoring on Aug 2, 2020
8. DrahtBot added the label Validation on Aug 2, 2020
9. 
   DrahtBot commented at 3:08 am on August 2, 2020: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #18267 (BIP-325: Signet [consensus] by kallewoof)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

10. in src/validation.cpp:3401 in f4d5c7ce15

    3395@@ -3396,22 +3396,21 @@ bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& pa
    3396 
    3397 int GetWitnessCommitmentIndex(const CBlock& block)
    3398 {
    3399-    int commitpos = -1;
    3400     if (!block.vtx.empty()) {
    3401-        for (size_t o = 0; o < block.vtx[0]->vout.size(); o++) {
    3402-            const CTxOut& vout = block.vtx[0]->vout[o];
    3403+        for (size_t o = block.vtx[0]->vout.size(); o > 0;) {
    3404+            const CTxOut& vout = block.vtx[0]->vout[--o];
    

    ---

    ---

    

    theStack commented at 2:46 pm on August 2, 2020:

    This 1-based to 0-based array indexing conversion within the body seems rather odd, why not simply 0-base it right from the start:

    0        for (int o = block.vtx[0]->vout.size()-1; o >= 0; o--) {
    1            const TxOut& vout = block.vtx[0]->vout[o];
    

    Drawback is that the loop counter type has to be changed from size_t (unsigned) to int signed, but I don’t see why that would be a problem.

    ---

    

    promag commented at 3:30 pm on August 2, 2020:

    Yes, need to change to signed. I’m fine with both TBH. Let’s see what others say.
11. 
    theStack commented at 2:54 pm on August 2, 2020: member

    Concept ACK
12. 
    sdaftuar commented at 3:44 pm on August 5, 2020: member

    Concept NACK. I say this in every PR of this type: I don’t think it makes sense to change consensus code unless we have some good reason to do so. See also my recent comment here.
13. promag closed this on Aug 5, 2020

    ---

14. promag deleted the branch on Aug 5, 2020
15. 
    promag commented at 4:18 pm on August 5, 2020: member

    @sdaftuar it’s fine. Just curious if it was intended to pick last element.
16. 
    promag commented at 11:02 pm on August 16, 2020: member

    @sipa ☝️ can you explain?
17. 
    MarcoFalke commented at 6:27 am on August 17, 2020: member

    The bip says that the last element has to be picked
18. DrahtBot locked this on Feb 15, 2022

Contributors
promag ajtowns DrahtBot theStack sdaftuar MarcoFalke

Labels
Refactoring Validation