WIP: add Schnorr opcode support
AbandonedPublic
Actions

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 Passed

Unit

No 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 reviewer: deadalnix. · View Herald Transcript

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4576: Diff 6740.Jan 19 2019, 03:41

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.

linted

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4577: Diff 6741.Jan 19 2019, 04:44

In D2342#52945, @Mengerian wrote:

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.

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

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