[refactor multisig] consolidate legacy mode logic
AbandonedPublic
Actions

Authored by markblundeberg on Jun 29 2019, 21:30.

Details

Reviewers

deadalnix
jasonbcox
Fabien

Group Reviewers

Restricted Project

Maniphest Tasks

T528: Add Schnorr support to OP_CHECKMULTISIG (new mechanics)

Summary

For new multisig, we want to consolidate legacy-only logic from
shared logic.

Both fSuccess and scriptCode are needed for both modes.

The following are only needed for legacy multisig:

CleanupScriptCode
legacy signature checking loop
NULLDUMMY check

Depends on D3673

Test Plan

make check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

refactorms

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 6724
Build 11495: Bitcoin ABC Buildbot (legacy)
Build 11494: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jun 29 2019, 21:30

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 29 2019, 21:30

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

markblundeberg added a child revision: D3474: Implement new checkmultisig trigger logic and execution logic..Jun 29 2019, 21:31

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6555: Diff 9802.Jun 29 2019, 21:36

This would benefit from changing the code a bit more, but in more elementary steps.

src/script/interpreter.cpp
967 ↗	(On Diff #9802)	This gradual incrementalist of i is bullshit. If you are refactoring this, you can do it right. Compute the indices once and save them in const variables. Don't be afraid of doing a set of very small change for this type of code. Compute the indices and use that instead of i. Then move some piece of code around. etc...
968 ↗	(On Diff #9802)	Once you use fixed indices, and don't save them in ints, you won't need these casts.

This revision now requires changes to proceed.Jun 30 2019, 21:13

markblundeberg added a task: T528: Add Schnorr support to OP_CHECKMULTISIG (new mechanics).Jul 1 2019, 06:11

include more refactors

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

markblundeberg added inline comments.Jul 8 2019, 01:08

src/script/interpreter.cpp
968 ↗	(On Diff #9802)	They do need to be saved in ints, since we perform negation on them, e.g., `stacktop(-idummy)`

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6724: Diff 10070.Jul 8 2019, 01:18

Mengerian added a subscriber: Mengerian.Jul 8 2019, 03:44

Mengerian added inline comments.

src/script/interpreter.cpp
981	The term "last pubkey" seems confusing to me. I think of the topmost pubkey on the stack as the "first" one... And isn't the topmost item also first to be checked when looping through the signatures? In any case, maybe changing the comment to something like "topmost pubkey" would be less ambiguous. Same for the other "last" items in comments below.

markblundeberg added inline comments.Jul 8 2019, 04:16

src/script/interpreter.cpp
981	👍

deadalnix requested changes to this revision.Jul 8 2019, 14:44

deadalnix added inline comments.

src/script/interpreter.cpp
972	No need for magic constants. Just make the initial value for i const and give it a meaningful name.
981	Also, the fact you need a comment indicate the name is not good. Remove the comment and fix the name. Also make it const.
988	Stacktop is deifned as #define stacktop(i) (stack.at(stack.size() + (i))) So it also expect a `size_t`. No reasons not to use size_t for indices. Negating an unsigned integer is not a big deal, computer math are not real math :)
999	No magic constants.
1002	dito
1008	You don't need success to be moved here. What comes next isn't using it.
1081	You'd make this much clearer if you'd have something like if (!fSuccess) { // Checks for NULLFAIL. } for (int i = 0; i < whatever; i++) { popstack(stack); } You could even consider moving the NULLFAIL check ahead of signature check in a subsequent diff to avoid checking a bunch of signature for nothing when you expect a failure anyways.

This revision now requires changes to proceed.Jul 8 2019, 14:44

markblundeberg added inline comments.Jul 9 2019, 15:05

src/script/interpreter.cpp
981	This variable can't be const since it gets modified later on. Likewise for nPubKeys.
988	This feels pretty weird to me. Is that a change you'd like to see across the entire interpreter.cpp?

markblundeberg added inline comments.Jul 9 2019, 15:18

src/script/interpreter.cpp
1081	OK 👍 As for moving the nullfail check to the front, that is rather tricky since even when you know ahead of time that fSuccess=false, the rest of the code must execute anyway, including at least one iteration of the main loop (due to the pubkey encoding rule). Also, it's not clear what kind of actual optimization this offers given that `TransactionSignatureChecker::CheckSig` should return immediately when the signature is null. At best we would be optimizing for a very particular case of a transaction where the signature list contains both null and non-null signatures, which anyway causes the originating peer to be banned.