Page MenuHomePhabricator

[refactor multisig] consolidate legacy mode logic
AbandonedPublic

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

deadalnix requested changes to this revision.Jun 30 2019, 21:13

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 retitled this revision from refactor multisig: consolidate logic for legacy mode to refactor OP_CHECKMULTISIG opcode implementation.
markblundeberg edited the summary of this revision. (Show Details)

include more refactors

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)

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.

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
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?

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.

src/script/interpreter.cpp
1081

(regarding executing anyway, looks like this wasn't covered, so I put up D3613)

markblundeberg retitled this revision from refactor OP_CHECKMULTISIG opcode implementation to [refactor multisig] consolidate legacy mode logic.
markblundeberg edited the summary of this revision. (Show Details)

reworked into parents