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.
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project - Maniphest Tasks
- T704: sigChecks implementation
- Commits
- rSTAGINGace2b19624bd: checkmultisig: refactor nullfail check
rABCace2b19624bd: checkmultisig: refactor nullfail check
make check
Diff Detail
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
| 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. |
| 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. |
| src/script/interpreter.cpp | ||
|---|---|---|
| 1139 ↗ | (On Diff #14717) | Ok, explains this is in a comment and change the name. |
| 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. |