refactor: Improve GetWitnessCommitmentIndex #19640
Conversation
promag
force-pushed
the
2020-08-getwitnesscommitmentindex
branch
from
b4d3f99 to
3a25f18
Compare
promag
force-pushed
the
2020-08-getwitnesscommitmentindex
branch
from
3a25f18 to
f4d5c7c
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
| for (size_t o = block.vtx[0]->vout.size(); o > 0;) { | ||
| const CTxOut& vout = block.vtx[0]->vout[--o]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, need to change to signed. I'm fine with both TBH. Let's see what others say.
|
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. |
|
@sdaftuar it's fine. Just curious if it was intended to pick last element. |
|
@sipa ☝️ can you explain? |
|
The bip says that the last element has to be picked |
GetWitnessCommitmentIndexfinds 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.