Page MenuHomePhabricator

checkmultisig: refactor nullfail check
ClosedPublic

Authored by markblundeberg on Dec 10 2019, 07:40.

Details

Summary

For the schnorr mode, this check is meaningless since fSuccess=true
always, so move it into legacy mode.

Also, make available the allsigsnull value for the sake of SigChecks
counting in a future Diff.

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
sigchecksA
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 8590
Build 15168: Default Diff Build & Tests
Build 15167: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Dec 11 2019, 01:54
deadalnix added inline comments.
src/script/interpreter.cpp
1139 ↗(On Diff #14717)
allsigsnull = allsigsnull && (vchSig.size() == 0);
1156 ↗(On Diff #14717)

Put that before the loop and see previous comment. The name ain't great either. I suggest isNullFail . It goes well with the flag that says it verify nullfail.

1167 ↗(On Diff #14717)

You'd get !isNullFail && (flags & SCRIPT_VERIFY_NULLFAIL) which seems much more natural.

This revision now requires changes to proceed.Dec 11 2019, 01:54
markblundeberg added inline comments.
src/script/interpreter.cpp
1139 ↗(On Diff #14717)

This is not equivalent and would change consensus behaviour: the loop is while (fSuccess && nSigsRemaining > 0), not while (nSigsRemaining > 0).

I will make sure there is a script test so your chain-splitting optimization will be warned about if it is introduced in future.

deadalnix requested changes to this revision.Dec 13 2019, 02:47
deadalnix added inline comments.
src/script/interpreter.cpp
1139 ↗(On Diff #14717)

Ok, explains this is in a comment and change the name.

This revision now requires changes to proceed.Dec 13 2019, 02:47
src/script/interpreter.cpp
1139 ↗(On Diff #14717)

"isNullFail" sounds a bit weird to me since it won't always be associated with a fail of the signature check. It will sometimes be True when the OP_CHECKMULTISIG operation succeeds (0-of-N).

I'd prefer a name that describes what it is ("are all signatures null?") rather than naming it after one of its use cases. (once sigchecks is implemented, there will be two independent use cases: the nullfail check, and the decision whether to add sigchecks).

src/script/interpreter.cpp
1139 ↗(On Diff #14717)

This naming is a bit weird as it uses nullfail as a noun. A nullfail is the situation in which the checksig fails due to all signature being null. While I concede it is weird, this is consistent with the flag name areAllSignatureNull or alike works as well.

ok, i'll go with areAllSignaturesNull

This revision is now accepted and ready to land.Dec 17 2019, 23:47