Page MenuHomePhabricator

Implement new checkmultisig trigger logic and execution logic.
Needs RevisionPublic

Authored by markblundeberg on Sat, Jun 29, 21:31.

Details

Reviewers
deadalnix
Mengerian
jasonbcox
Fabien
Group Reviewers
Restricted Project
Maniphest Tasks
T528: Add Schnorr support to OP_CHECKMULTISIG (new mechanics)
Summary
Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
schnorrmultisig
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6727
Build 11501: Bitcoin ABC Teamcity Staging
Build 11500: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Sat, Jun 29, 21:31
Herald added a reviewer: Restricted Project. · View Herald TranscriptSat, Jun 29, 21:31
markblundeberg added inline comments.Sat, Jun 29, 21:37
src/script/interpreter.cpp
1052 ↗(On Diff #9803)

may be worth making a new error message for the invalid signature case here (SCRIPT_ERROR_INVALID_SIGNATURE) ?

markblundeberg planned changes to this revision.Tue, Jul 2, 16:41

shelving for now, until parents are updated

  • rebase for new flag name
  • add a new error SCRIPT_ERR_INVALID_CHECKBITS_SIGNATURE (for want of a better name...)
  • small tweaks to tests
deadalnix requested changes to this revision.Mon, Jul 8, 15:16
deadalnix added inline comments.
src/script/interpreter.cpp
1023

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

1026

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

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

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

1050

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

This revision now requires changes to proceed.Mon, Jul 8, 15:16
Mengerian added inline comments.Mon, Jul 8, 16:50
src/script/interpreter.cpp
1031

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.Mon, Jul 8, 16:58
src/script/interpreter.cpp
1031

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

1032

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

1050

I agree, any suggestions?

deadalnix added inline comments.Mon, Jul 8, 17:21
src/script/interpreter.cpp
1031

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

Execute them before and them continue.

1050

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

markblundeberg added inline comments.Mon, Jul 8, 17:48
src/script/interpreter.cpp
1031

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

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

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