Page MenuHomePhabricator

checkmultisig: refactor nullfail check
ClosedPublic

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
T704: sigChecks implementation
Commits
rABCace2b19624bd: checkmultisig: refactor nullfail check
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markblundeberg created this revision.Dec 10 2019, 07:40
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 10 2019, 07:40
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 requested review of this revision.Dec 11 2019, 11:25
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
markblundeberg added inline comments.Dec 13 2019, 04:23
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).

markblundeberg requested review of this revision.Dec 14 2019, 01:48
deadalnix added inline comments.Dec 16 2019, 14:04
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.

markblundeberg planned changes to this revision.Dec 17 2019, 00:11

ok, i'll go with areAllSignaturesNull

rebase & tweak name

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