refactor: Improve GetWitnessCommitmentIndex #19640

pull promag wants to merge 1 commits into bitcoin:master from promag:2020-08-getwitnesscommitmentindex changing 1 files +4 −5

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.
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.
promag force-pushed on Aug 1, 2020
refactor: Improve GetWitnessCommitmentIndex f4d5c7ce15

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 12:14 AM on August 2, 2020:

return o-1; ?

promag commented at 12:28 AM on August 2, 2020:

Yup yup, forgot to fix this. Initially was bad because o is unsigned. Will fix.

promag force-pushed on Aug 2, 2020
DrahtBot added the label Refactoring on Aug 2, 2020
DrahtBot added the label Validation on Aug 2, 2020
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.

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:

        for (int o = block.vtx[0]->vout.size()-1; o >= 0; o--) {
            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.

theStack commented at 2:54 PM on August 2, 2020: member

Concept ACK
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.
promag closed this on Aug 5, 2020
promag deleted the branch on Aug 5, 2020
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.
promag commented at 11:02 PM on August 16, 2020: member

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

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

Contributors

Labels

Refactoring Validation

Linked (view graph)

#22059 Cirrus CI: Linker OOMKilled

refactor: Improve GetWitnessCommitmentIndex #19640

Conflicts