Page MenuHomePhabricator

API renames of script signature functions to mention ECDSA
AbandonedPublic

Authored by markblundeberg on Jan 19 2019, 19:33.

Details

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

Like with D2345 , this is a part of the effort to introduce schnorr sigs

  • CheckTransactionSignatureEncoding -> CheckTransactionECDSASignatureEncoding
  • CheckDataSignatureEncoding -> CheckDataECDSASignatureEncoding
  • CheckSig -> CheckSigECDSA
  • VerifySignature -> VerifySignatureECDSA
Test Plan

Trivial renames -- it compiles = success.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
rename-ecdsa-2
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4585
Build 7233: Bitcoin ABC Buildbot (legacy)
Build 7232: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jan 19 2019, 19:33
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 19 2019, 19:33
Herald added a subscriber: schancel. · View Herald Transcript
markblundeberg edited the summary of this revision. (Show Details)Jan 19 2019, 19:37
deadalnix requested changes to this revision.Jan 20 2019, 00:38
deadalnix added inline comments.
src/script/interpreter.cpp
879

See comments on checkdatasig.

931

You don't want to be doing this as this isn't a codepath that is specific to ECDSA. t is not the responsability of the opcode to know what is the encoding of the signature.

1031

This is the only place where you want to have a change in the interpreter because multisig is the only opcode that care about the sig being ECDSA or schnorr.

src/script/interpreter.h
39

I don't think it make sense to specialize this API on the type of signature.

src/script/sigencoding.h
35

This shouldn't change.

43

Keep the original, but move the implementation in CheckTransactionECDSASignatureEncoding and get CheckTransactionSignatureEncoding to forward to it. That'll give you a natural point to insert check for ECDSA and schnorr that doesn't clutter all the callsite and is consistent with each piece of code's responsibility.

This revision now requires changes to proceed.Jan 20 2019, 00:38
markblundeberg added inline comments.Jan 20 2019, 06:13
src/script/interpreter.h
39

It seems there are four ways to go. One choice:

  • prefix a flag byte (like in current Schnorr specification) so that schnorr signatures and ECDSA signatures can be distinguished without needing context.

Otherwise if we use 'naked' Schnorr signatures without flag byte, then we must pass along the decision of whether a 64 byte vchSig blob is supposed to be interpreted as ECDSA or Schnorr. Only EvalScript has enough information to make this classification, as the decision will depend on which opcode is being executed, how it is being executed, AND whether flag ENABLE_SCHNORR is active. I can think of three ways to do this:

  • add a flag parameter, boolean -- like in the initial D2342's isSchnorr.
  • add a flag parameter, enumerated (SIG_TYPE_SCHNORR / SIG_TYPE_ECDSA).
  • duplicate all the code pathways, to provide context -- like in this Diff.

Which do you prefer, or is there another way I haven't thought of?

deadalnix added inline comments.Jan 20 2019, 19:20
src/script/interpreter.cpp
931

See this comment ^

src/script/interpreter.h
39

What does CheckSig do ? It checks if a signature is valid, according to a certain pubkey, a certain message and a certain set of flags.

A caller can pass it down a signature, a pubkey, and tell what type of signature he/she is willing to accept or not. This seems to me to be a perfectly fine contract to present to the caller. At the end of the day, this is what an API is: a contract and this is what this diff is about: defining what the contract is in a way that allow for extension that we are interested in.

Contract are about defining responsibilities. The way we check to know if a signature is schnorr signature or not has nothing to do with it. There is a piece of code right now who's responsibility is to check if a signature is valid or not. It is the responsibility of piece of code to decide if a signature is valid. It seems like a totology? It's because it.

markblundeberg added inline comments.Jan 20 2019, 19:55
src/script/interpreter.cpp
931

For 'naked' 64 byte Schnorr signatures, EvalScript is the only place where all necessary information is available on which signatures are acceptable / accepted. Inspection of flags and vchSig is not sufficient.

src/script/interpreter.h
39

and tell what type of signature he/she is willing to accept or not.

OK, so are you saying we should extend the API with an enum parameter (EXPECT_ECDSA, ACCEPT_BOTH) to provide context on which signatures are expected / acceptable for that opcode executing in that mode? CheckSig needs additional information to know whether it is supposed to interpret a 64 byte blob as ECDSA or Schnorr: I.e.,

  • During OP_CHECKSIG it will be called as: CheckSig(vchSig, vchPubKey, scriptCode, flags, ACCEPT_BOTH)
  • During OP_CHECKMULTISIG (legacy mechanics) it will be called as: CheckSig(vchSig, vchPubKey, scriptCode, flags, EXPECT_ECDSA)
  • During OP_CHECKMULTISIG (new mechanics) it will be called as: CheckSig(vchSig, vchPubKey, scriptCode, flags, EXPECT_SCHNORR)

Or, we can triplicate the API into CheckSig, CheckMultiSigLegacy, CheckMultiSigNew.

(Or we can put a flag byte on the blockchain to make it so we don't have an ugly internal API.)

markblundeberg added inline comments.Jan 20 2019, 19:59
src/script/interpreter.h
39

CheckSig, CheckMultiSigLegacy, CheckMultiSigNew.

(or alternatively: CheckSigEither, CheckSigECDSAOnly, CheckSigSchnorrOnly)

markblundeberg abandoned this revision.Jan 22 2019, 00:03

abandoning this in favour of an alternate approach: D2373 etc.