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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 6856
Build 11759: Bitcoin ABC Buildbot (legacy)
Build 11758: 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
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

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)

Mengerian added inline comments.
src/script/interpreter.cpp
981 ↗(On Diff #10070)

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 ↗(On Diff #10070)

👍

deadalnix requested changes to this revision.Jul 8 2019, 14:44
deadalnix added inline comments.
src/script/interpreter.cpp
972 ↗(On Diff #10070)

No need for magic constants. Just make the initial value for i const and give it a meaningful name.

981 ↗(On Diff #10070)

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 ↗(On Diff #10070)

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 ↗(On Diff #10070)

No magic constants.

1002 ↗(On Diff #10070)

dito

1008 ↗(On Diff #10070)

You don't need success to be moved here. What comes next isn't using it.

1081 ↗(On Diff #10070)

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 ↗(On Diff #10070)

This variable can't be const since it gets modified later on. Likewise for nPubKeys.

988 ↗(On Diff #10070)

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 ↗(On Diff #10070)

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.

markblundeberg added inline comments.Jul 9 2019, 16:25
src/script/interpreter.cpp
1081 ↗(On Diff #10070)

(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

markblundeberg planned changes to this revision.Jul 14 2019, 01:10
markblundeberg abandoned this revision.Jul 30 2019, 01:37

obsolete