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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

review-ACK 49f9b64 🐮

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review-ACK 49f9b645ea3f42c1b9e0a83b26af8fc8f6fa159d 🐮
DBaDDPWYrgqPiqxeTlRzcfIADMMqBxcLr0V/soQ4agaKh7fHhMUyEW77K17VoAMEla59o3GiThk4mPy14oGtAw==

ACK 49f9b64

For the future: you could have made the commit here an actual scripted diff too.

For the future: you could have made the commit here an actual scripted diff too.

Good point, thanks, still getting the hang of scripted diffs. Makes sense in case new stuff using uint256S gets merged into master before this.

For the future: you could have made the commit here an actual scripted diff too.

Good point, thanks, still getting the hang of scripted diffs. Makes sense in case new stuff using uint256S gets merged into master before this.

A scripted-diff won't help here, because it is applied on top of the commit it is based on, which won't change when master is changed, unless you rebase on every push to master.

Moreover, I think scripted-diffs should only be used when it makes sense. As a rule of thumb, I think when the script is larger than the diff, they do not make sense, because:

• A script in the scripted-diff must be reviewed.
• Otherwise, it can accidentally or intentionally modify files that are not tracked by git (Which has happened in the past).
• Failing statements in the scripted-diff won't cause the CI to fail, which is another way in which reviewers could be accidentally or intentionally tricked to think a script has executed when it did not, and they may miss a bug.
• Seeing a scripted-diff could encourage reviewers to skip or skim over the actual diff, missing bugs.

It seems easier for reviewers who like grep to just call git grep --extended-regexp '\buint256S\("(0x)?([^"]{64})"\)' 49f9b645ea3f42c1b9e0a83b26af8fc8f6fa159d~1 (and ...~0 to see the difference).

A scripted-diff won't help here, because it is applied on top of the commit it is based on, which won't change when master is changed, unless you rebase on every push to master.

Couldn't CI at least run scripted diffs when the PR they belong to gets merged into master - so it could be caught post-merge?

When a commit from a pull request (which may contain a scripted-diff) is merged into another branch, the commit itself does not change, so it won't affect the execution or the result of the scripted-diff in that commit.

In theory, the commits could be rebased/cherry-picked instead of merged. However, that'd make auditing harder, because it makes it easier to inject malicious code accidentally or intentionally. Also, it wouldn't help here, because then the scripted diff check may permanently fail on the rebased/cherry-picked commits. (It could only be fixed up in a follow-up, which seems easier to do as-is done today).

#29775 was merged into master as da083d4, and #30560 was merged into master as 21c2879.

I'm thinking CI running on 21c2879 upon merge, could detect a new scripted-diff, re-run it on 21c2879 and in theory detect issues with #29775 if it had been merged into master earlier (wouldn't have caught this case though, as da083d4 was merged later).

Is that mental model of Git correct?

Are you concerned that scripted-diffs should only be run on the tree of their own commit, and might cause issues when run on merge-commits when PRs include later commits after their scripted-diff that happen to introduce text that would be replaced?

What I'm after is less reliance on having to remember to re-run scripted-diffs.

Needs backport to 28.x branch

Needs backport to 28.x branch

Unless this fixes a bug that I'm missing, refactors generally are not candidates for backport.