Page MenuHomePhabricator

WIP: add Schnorr opcode support
AbandonedPublic

Authored by markblundeberg on Jan 19 2019, 03:30.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
T527: Add Schnorr support to OP_CHECKSIG and OP_CHECKDATASIG
Summary

This implements
https://github.com/markblundeberg/bitcoincash.org/blob/schnorr/spec/schnorr-spec.md
Actually, this codes a variant where the flag byte is removed and instead
Schnorr are identified by size. It is however easy to convert to flag byte.

Depends on D2169 (secp256k1 schnorr upgrade)

Test Plan

need to add tests, among them:

  • - script with tests of flag activation
  • - test op_checkmultisig still accepts 64 byte ECDSA
  • - many more...

Diff Detail

Repository
rABC Bitcoin ABC
Branch
add-schnorr-opcodes
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4577
Build 7217: Bitcoin ABC Buildbot (legacy)
Build 7216: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jan 19 2019, 03:30
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 19 2019, 03:30
Herald added a subscriber: schancel. · View Herald Transcript

Question, regarding this To Do item: "test op_checkmultisig still accepts 64 byte ECDSA"

Would it make sense to restrict MultiSig to non-64-byte ECDSA? If the plan is to eventually implement Schnorr on MultiSig also, I just wonder if avoiding 64-byte ECDSA would be better.

markblundeberg added a comment.EditedJan 19 2019, 05:49

Question, regarding this To Do item: "test op_checkmultisig still accepts 64 byte ECDSA"
Would it make sense to restrict MultiSig to non-64-byte ECDSA? If the plan is to eventually implement Schnorr on MultiSig also, I just wonder if avoiding 64-byte ECDSA would be better.

Good question, I've been thinking about it a lot.

Here are some thoughts on the possible mechanics, put elsewhere as they are beyond the scope of this Diff: https://gist.github.com/markblundeberg/2c2a542d3dfbcda4d9a80157a5269d68
As you can see, I don't think it's possible to make a new OP_CHECKMULTISIG mechanism that triggers based on discriminating among Schnorr/ECDSA signatures themselves, without having ambiguity issues. This is because of the problem of OP_0 being a legal "signature" in some cases. Because of that, I don't think it will help us at all to forbid 64-byte ECDSA within the multisigs at this time.

deadalnix requested changes to this revision.Jan 19 2019, 13:30

This is going to get big real quick and I suggest you start splitting this up in logicial chunks. There is one immediate item that appear to distinguish itself, it is the renaming of various function that assume simply "signing" to make it explicit they are using ECDSA. Theses are simple change that do not affect how the codebase function in any way and could be merged quickly. It would also ensure the rest of the change stands out and are easier to review as a result.

For isntance:

  • CheckTransactionSignatureEncoding => CheckTransactionECDSASignatureEncoding
  • CKey::Sign => CKey::SignECDSA
  • CPubKey::Verify => CPubKey::VerifyECDSA
src/script/interpreter.cpp
898

CheckTransactionSignatureEncoding is passed the flags, so it knows if it is admissible that this is a shnorr signature. This whole thing should be internal mechanic of checking the signature encoding, IMO.

src/script/interpreter.h
32

D not add bool parameters to functions if it can be avoided. It makes for terrible API. See https://softwareengineering.stackexchange.com/questions/147977/is-it-wrong-to-use-a-boolean-parameter-to-determine-behavior for instance.

src/script/script_flags.h
105

Simply adding this flag + tests checking that it does nothing should be a diff on its own.

src/script/sigcache.cpp
43

What is the goal of this ?

src/script/sigencoding.cpp
229

It's required we accept this for NULLFAILto work properly anyways.

src/script/sigencoding.h
35

You are passed the flags down, so it is probably the respectability of that function to check if the encoding correspond to schnorr and is enabled or not rather than the caller.

This revision now requires changes to proceed.Jan 19 2019, 13:30
markblundeberg planned changes to this revision.Jan 19 2019, 15:41

OK, thanks for the quick feedback! I will fix the booling API. I can split this diff up like so for starters:

  • "trivial" Diff that merely alters the names/APIs of existing functions in obvious ways.
  • Diff that adds new Schnorr accessory functions but they are unused. Relevant tests of these functions such as possible interaction with ECDSA (like via sigcache).
  • Diff that adds the new flag and modifies evalscript mechanics; heavy testing.
src/script/interpreter.cpp
898

CheckTransactionSignatureEncoding gets used in CHECKMULTISIG too, where schnorrs are inadmissible regardless of flags. It's a bit awkward, but the whole logic of this diff is built around the fact that it is the responsibility of opcodes in EvalScript to already decide whether a signature ought to be put into the ECDSA or Schnorr pipelines. (the specification will have to be clarified on this front, it's an important conceptual point)

src/script/interpreter.h
32

Yes, it's possible to do it with duplicated API instead.

src/script/script_flags.h
105

OK. Makes sense.

src/script/sigcache.cpp
43

If we do the unflagged 64-byte signature approach, I think this explicit distinguishing is important for consensus security.

Consider the case where a valid 64-byte ECDSA signature is put into the cache somehow: either during pre-upgrade or from checkmultisig. It then gets maliciously reused post-upgrade as a "Schnorr" signature. This is supposed to fail since it is an incorrect schnorr signature, but it would instead pass due to cache lookup.

I think I know how to turn this into an attack where an attacker could cause a mining node to mine only invalid blocks, but it's too elaborate to get into here...

src/script/sigencoding.cpp
229

True.

Actually right now only length=65 sigs get passed to CheckTransactionSchnorrSignatureEncoding. However in future when we add the new Schnorr CHECKMULTISIG mechanics, this function will be getting inputs of all lengths. I am going to rewrite the logic here.

src/script/sigencoding.h
35

ditto to previous comment

Currently this diff being split into D2345 D2347 D2348 ... more to come.

deadalnix added inline comments.Jan 20 2019, 00:50
src/script/sigcache.cpp
43

I see.

markblundeberg abandoned this revision.Jan 22 2019, 01:58

Obsolete. This is being replaced by other Diffs that can be seen in T527