Page MenuHomePhabricator

[refactor multisig] remove redundant stack check
AbandonedPublic

Authored by markblundeberg on Jul 10 2019, 02:33.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

The presence of the dummy element is already checked with
the if ((int)stack.size() < i) { check near the beginning of this
opcode.

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
rms3
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6788
Build 11623: Bitcoin ABC Buildbot (legacy)
Build 11622: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Jul 10 2019, 02:35

This makes me a bit worried, to be honest. Isn't it possible to move the nulldummy check near the bound check? This will avoid the spooky action at a distance effect (which is probably why this redundant check survived here in the first place).

This revision now requires changes to proceed.Jul 10 2019, 02:35

This makes me a bit worried, to be honest. Isn't it possible to move the nulldummy check near the bound check? This will avoid the spooky action at a distance effect (which is probably why this redundant check survived here in the first place).

Yes, I intend to do this in a follow-up diff, however that's technically a semantic change and the redundant check has to be removed one way or another. I'm not sure what spooky interaction you're alluding to 🤔 .

reupload for unrelated test failure

deadalnix requested changes to this revision.Jul 10 2019, 15:02

This code must remain "obviously correct" as much as possible. Systematically doing bound check before accessing an element is absolutely part of that process.

If you move the access, then it become obvious that the previous bound check covers it. Right now it is spooky action at a distance.

This revision now requires changes to proceed.Jul 10 2019, 15:02