Page MenuHomePhabricator

Implement new checkmultisig trigger logic and execution logic.
ClosedPublic

Authored by deadalnix on Jun 29 2019, 21:31.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
deadalnix requested changes to this revision.Jul 8 2019, 15:16
deadalnix added inline comments.
src/script/interpreter.cpp
1023 ↗(On Diff #10073)

You are much better off working with unsigned when you handle bitfields.

1026 ↗(On Diff #10073)

This is not the check you want to be doing.

You want to check that:

  • All the set bits are in range: (nCheckBits & ((1 << keyCount) - 1)) == nCheckBits
  • The number of set bits correspond to the number of expected signatures: popcount(nCheckBits) == sigCount
1031 ↗(On Diff #10073)

Use proper variable instead of mutating other variable that mean something other than being a loop counter.

You also probably want to have two loops, one to validate pubkey encoding and one to validate sigs. There is no point starting to validate sigs. More generally, you shoudl try to simplify this loop by shaving stuff out of it.

1032 ↗(On Diff #10073)

When you nest most of the logic in an if, you indicate that you probably should bail the opposite branch early.

1050 ↗(On Diff #10073)

It's not clear what this error stands for. It needs a better name.

This revision now requires changes to proceed.Jul 8 2019, 15:16
Mengerian added inline comments.Jul 8 2019, 16:50
src/script/interpreter.cpp
1031 ↗(On Diff #10073)

Having a separate loop here to check pubkey encoding results is strange behavior: It would check all pubkeys for successful Schnorr multisigs, but not for the "0-of-N" or "false" cases. (Or for the legacy case).

Two potential alternatives are:

  1. Check pubkey encoding only when it has a corresponding signature (ie, when the checkbit is "1"), and don't check the others. I can't think of any reason not to do this. It is similar to the current multisig operation, and seems to have minimal side effects. It would render some previously unspendable coins become spendable (when unused pubkeys encoding was sometimes checked due to how legacy multisig works).
  1. Move the loop that checks pubkey encoding to prior to the dummy element "mode selection" check. This would make it apply to both the legacy case (including 0-of-N and "false") as well as the new Schnorr mode. This disadvantage of this is that it would affect already existing coins, rendering them unspendable if they have invalidly encoded pubkeys. I'm not sure if this is common or not, so it's possible it's not a major problem.
markblundeberg added inline comments.Jul 8 2019, 16:58
src/script/interpreter.cpp
1031 ↗(On Diff #10073)

A separate loop to validate pubkey encoding would likely mean a change in behaviour -- all pubkeys must be correctly encoded. Is that intended?

1032 ↗(On Diff #10073)

When the predicate here is false, we aren't bailing. There are still statements that need to be executed.

1050 ↗(On Diff #10073)

I agree, any suggestions?

deadalnix added inline comments.Jul 8 2019, 17:21
src/script/interpreter.cpp
1031 ↗(On Diff #10073)

It doesn't check for 0 of n anyways and what is the false case ? If there is a discrepancy in behavior, this can make a good test case. I can't derive one from that comment.

1032 ↗(On Diff #10073)

Execute them before and them continue.

1050 ↗(On Diff #10073)

Thinking about it, it's a nullfail or a nulldummy error, not really a new kind of error.

markblundeberg added inline comments.Jul 8 2019, 17:48
src/script/interpreter.cpp
1031 ↗(On Diff #10073)

The appropriate test cases are in place. If you want to arc patch this Diff and parents, and try the modifications you suggest, you can see how it affects the test cases.

1032 ↗(On Diff #10073)

They can't be executed before since they modify values used in the body of this if-statement. It is possible to rewrite this to use if(!x) continue; but it just gets more verbose.

1050 ↗(On Diff #10073)

Indeed, that's exactly what I had in a prior version of this Diff. :-D I'm happy to revert if that's desirable.

markblundeberg edited the summary of this revision. (Show Details)

rebased & change to the new bitfield decoding scheme

markblundeberg added inline comments.Jul 19 2019, 06:08
src/test/script_tests.cpp
2114 ↗(On Diff #10354)

Todo: also add a test for the M=0 case for both legacy & new mode.

deadalnix requested changes to this revision.Jul 19 2019, 23:45
deadalnix added inline comments.
src/script/interpreter.cpp
1034 ↗(On Diff #10354)

It's really great to see you enforce assumptions this way.

1040 ↗(On Diff #10354)

You should extract this into its own function and have unit tests for it. This can be its own patch. The logic is simple, but it doesn't matter.

1042 ↗(On Diff #10354)

You can iterate over all sigs and start with a loop that find the suitable pubkey.

int ipubkey = 0;
for (int i = 0; i < nSigCount; i++, ipubkey++) {
  if ((checkBits >> ipubkey) == 0) {
    // Help, we fucked up the bitfield validation!
  }
  while ((checkBits >> ipubkey) & 1 == 0) {
    ipubkey++;
  }
  if (ipubkey >= nKeyCount) {
    // Help, we fucked up the bitfield validation 2.0!
  }

  // Check signature and pubkey.
}
if ((checkBits >> ipubkey) == 0) {
  // Help, we fucked up the bitfield validation 3.0!
}

This would require you to validate the bitfield ahead of time, but this is desirable anyways.

1050 ↗(On Diff #10354)

This is extremely confusing and error prone. Looks like you are going from the bottom key/sig to the topmost one. Can you explain why this is done in this order?

src/test/script_tests.cpp
2114 ↗(On Diff #10354)

I see you already know what to do here :)

This revision now requires changes to proceed.Jul 19 2019, 23:45
markblundeberg added inline comments.Jul 20 2019, 00:14
src/script/interpreter.cpp
1050 ↗(On Diff #10354)

Note these two lines are identical to the loop below, which you reviewed in D3675. It was clear enough then, has something changed?

The loop order starts on the top of the stack and moves deeper. The least significant bit of the initial checkbits corresponds to the top public key (the last public key pushed on stack).

markblundeberg marked an inline comment as not done.Jul 23 2019, 17:18
markblundeberg added inline comments.
src/script/interpreter.cpp
1066 ↗(On Diff #10354)

just nullfail

add more test cases

markblundeberg planned changes to this revision.Jul 25 2019, 02:04

@deadalnix ok to commandeer now

deadalnix commandeered this revision.Jul 25 2019, 23:38
deadalnix edited reviewers, added: markblundeberg; removed: deadalnix.
deadalnix updated this revision to Diff 10497.Jul 25 2019, 23:38

Rebase and clarify logic.

Hmm what a strange failure ... can't see how that is related at all.

src/script/interpreter.cpp
1025 ↗(On Diff #10497)

Ah, I forgot to change this comment - should just read NEW MULTISIG (SCHNORR)

markblundeberg requested changes to this revision.Jul 28 2019, 19:46
markblundeberg added inline comments.
src/script/interpreter.cpp
1054 ↗(On Diff #10497)

For such sanity checks, isn't it better to have an assert than to make a silent consensus judgement (script invalid & transaction invalid & block invalid)? If these conditions are violated then something is seriously wrong ... and there are already several asserts like this in interpreter.cpp.

1067 ↗(On Diff #10497)

ditto

1102 ↗(On Diff #10497)

ditto

This revision now requires changes to proceed.Jul 28 2019, 19:46
markblundeberg added inline comments.Jul 28 2019, 19:50
src/script/interpreter.cpp
1092 ↗(On Diff #10497)

NULLFAIL

markblundeberg added inline comments.Jul 28 2019, 20:58
src/test/script_tests.cpp
2040 ↗(On Diff #10497)

oops, this & ~SCRIPT_VERIFY_MINIMALDATA isn't needed here.

2150 ↗(On Diff #10497)

What's "Uxc'loo"? Also Antony pointed out that "first" / "last" is a bit vague since it could be interpreted as "first pushed" or "first popped".

Also the text says 011 but the test does 110

2160 ↗(On Diff #10497)

ditto on bitfield

2171 ↗(On Diff #10497)

ditto on first/last

2181 ↗(On Diff #10497)

ditto on bitfield, first/last

2215 ↗(On Diff #10497)

I just realized there is an important missing test case: run a new mode multisig with non-null bitfield but put in an ECDSA signature. (Error -> SIG_NONSCHNORR)

tests.push_back(TestBuilder(CScript() << OP_1 << ToByteVector(keys.pubkey0)
                                      << OP_1 << OP_CHECKMULTISIG,
                            "CHECKMULTISIG 1-of-1 ECDSA signature in Schnorr mode",
                            newmultisigflags)
                    .Num(0b1)
                    .PushSigECDSA(keys.key0).SetScriptError(ScriptError::SIG_NONSCHNORR));
tests.push_back(TestBuilder(CScript() << OP_3 << ToByteVector(keys.pubkey0C)
                                      << ToByteVector(keys.pubkey1C)
                                      << ToByteVector(keys.pubkey2C) << OP_3
                                      << OP_CHECKMULTISIG,
                            "CHECKMULTISIG 3-of-3 Schnorr with mixed-in ECDSA signature",
                            newmultisigflags)
                    .Num(0b111)
                    .PushSigECDSA(keys.key0)
                    .PushSigSchnorr(keys.key1)
                    .PushSigSchnorr(keys.key2).SetScriptError(ScriptError::SIG_NONSCHNORR));
Mengerian added inline comments.Aug 1 2019, 22:14
src/script/script_error.cpp
49 ↗(On Diff #10497)

bit -> bits

deadalnix updated this revision to Diff 10568.Aug 1 2019, 23:31

Add mixed ECDSA/Schnorr test cases.
Fix various typos.

deadalnix updated this revision to Diff 10569.Aug 1 2019, 23:43

bottom => first

markblundeberg accepted this revision.Aug 1 2019, 23:47
This revision is now accepted and ready to land.Aug 1 2019, 23:47
deadalnix updated this revision to Diff 10570.Aug 2 2019, 00:18

Make sure we have strictly less than 32 pubkeys

This revision was landed with ongoing or failed builds.Aug 2 2019, 00:24
This revision was automatically updated to reflect the committed changes.