Details

Reviewers

markblundeberg
deadalnix

Group Reviewers

Restricted Project

Maniphest Tasks

T527: Add Schnorr support to OP_CHECKSIG and OP_CHECKDATASIG

Commits

rSTAGINGc27207fed9c7: Refactor signature encoding checks
rABCc27207fed9c7: Refactor signature encoding checks

Summary

Isolate ECDSA-specific parts in preparation for Schnorr.

Next steps would be:

Disallow 64 byte ECDSA signatures in CheckRawECDSASignatureEncoding when SCRIPT_ENABLE_SCHNORR flag is set (D2469)
Add Schnorr-specific functions for checking signature encodings

Test Plan

make check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

sigencoding

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4821
Build 7705: Bitcoin ABC Buildbot (legacy)
Build 7704: arc lint + arc unit

Event Timeline

Mengerian created this revision.Feb 1 2019, 13:19

Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 1 2019, 13:19

Herald added a subscriber: schancel. · View Herald Transcript

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

simplify refactor

Mengerian mentioned this in D2469: Add SCRIPT_ENABLE_SCHNORR support to sigencoding.Feb 1 2019, 13:33

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

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

Harbormaster completed remote builds in B4812: Diff 7090.Feb 1 2019, 13:35

markblundeberg requested changes to this revision.Feb 1 2019, 13:39

markblundeberg added inline comments.

src/script/sigencoding.cpp
226 ↗	(On Diff #7090)	This is backwards. When I add Schnorr, I can't put `if (Schnorrflag & Size == 65) return true;` at the start of this. Even Schnorr signatures need to have a valid hashtype byte, so before returning true we must first check the hashtype byte. CheckTransactionECDSASignatureEncoding however must have `if (Schnorrflag & Size == 65) return false;` at the start since it gets directly called from CHECKMULTISIG. Since it's the inner function for CheckTransactionSignatureEncoding this would then also block schnorr signatures for CHECKSIG too.

This revision now requires changes to proceed.Feb 1 2019, 13:39

(CheckTransactionECDSASignatureEncoding could also be called CheckTransactionSignatureEncodingButNo65ByteSignaturesAllowedIfSchnorrEnabled, at least that is the function that CHECKMULTISIG must call per specification.)

Harbormaster completed remote builds in B4813: Diff 7091.Feb 1 2019, 13:48

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

Mengerian added a child revision: D2478: Refactor Signature encoding check to prepare for Schnorr inclusion..Feb 1 2019, 14:33

@markblundeberg Have a look at D2478. Does it make more sense with that one added after?

I split the refactor into two, this one to pave the way for banning 64-byte ECDSA signatures, and D2478 to make it easy to add Schnorr.

My idea is to add the code to restrict 64-byte signature inside CheckRawECDSASignatureEncoding, and add the code allowing 64-byte signatures (with no DER checks) in CheckRawSignatureEncoding

Maybe it would be less confusing if I combine the diffs into one.

I think I will combine D2478 with this Diff.

It seems splitting them just made it more confusing

In D2477#56464, @Mengerian wrote:

@markblundeberg Have a look at D2478. Does it make more sense with that one added after?

I split the refactor into two, this one to pave the way for banning 64-byte ECDSA signatures, and D2478 to make it easy to add Schnorr.

My idea is to add the code to restrict 64-byte signature inside CheckRawECDSASignatureEncoding, and add the code allowing 64-byte signatures (with no DER checks) in CheckRawSignatureEncoding

Maybe it would be less confusing if I combine the diffs into one.

OK yup, the end result after the second diff looks good 👍 .

Combine D2478 in this Diff.
it's less confusing this way

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

Mengerian mentioned this in D2478: Refactor Signature encoding check to prepare for Schnorr inclusion..Feb 1 2019, 15:19

OK looks good, I added a couple of comments showing how this will have to integrate with the schnorr flag

src/script/sigencoding.cpp
174 ↗	(On Diff #7093)	my diffs will add just before this line if(size==64 & schnorr) return true;
218 ↗	(On Diff #7093)	my diffs will add here: if(size==64 & schnorr) set error & return false;

This revision is now accepted and ready to land.Feb 1 2019, 15:26

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

Harbormaster completed remote builds in B4815: Diff 7093.Feb 1 2019, 15:32

markblundeberg added inline comments.Feb 1 2019, 15:44

src/script/sigencoding.cpp
218 ↗	(On Diff #7093)	correction: size == 65

markblundeberg added a task: T527: Add Schnorr support to OP_CHECKSIG and OP_CHECKDATASIG.Feb 1 2019, 15:56

Mengerian added inline comments.Feb 1 2019, 16:19

src/script/sigencoding.cpp
174 ↗	(On Diff #7093)	Yes, agreed.
218 ↗	(On Diff #7093)	I was actually imagining the 64-byte ban being inside CheckRawECDSASignatureEncoding But it works in either place.

markblundeberg added inline comments.Feb 1 2019, 16:22

src/script/sigencoding.cpp
218 ↗	(On Diff #7093)	Oh! Good point, yes that works too, even better...

deadalnix requested changes to this revision.Feb 1 2019, 16:50

deadalnix added inline comments.

src/script/sigencoding.cpp

27 ↗

(On Diff #7093)

It has nothing to do with ECDSA, it is DER encoding. Please adjust the name accordingly. CheckRawECDSASignatureEncoding is the only part doing anything ECDSA specific.

244 ↗

(On Diff #7093)

Because both have the exact same structure, as the exception of the CheckRawSignatureEncoding call, it's probaby a good idea to build a template.

template<typename F> static bool CheckTransactionSignatureEncodingImpl(const valtype &vchSig, uint32_t flags, ScriptError *serror, F fun) {
  // Empty signature. Not strictly DER encoded, but allowed to provide a
  // compact way to provide an invalid signature for use with CHECK(MULTI)SIG
  if (vchSig.size() == 0) {
      return true;
  }

  if (!fun(vchSig | boost::adaptors::sliced(0, vchSig.size() - 1), flags, serror)) {
      // serror is set
      return false;
  }

  return CheckSighashEncoding(vchSig, flags, serror);
}

bool CheckTransactionSignatureEncoding(const valtype &vchSig, uint32_t flags,
                                     ScriptError *serror) {
  return CheckTransactionSignatureEncodingImpl(vchSig, flags, serror,
      [](const slicedvaltype &sig, uint32_t flags, ScriptError *serror) {
        return CheckRawSignatureEncoding(sig, flags, serror);
      });
}

And so on.

This revision now requires changes to proceed.Feb 1 2019, 16:50

markblundeberg added a child revision: D2469: Add SCRIPT_ENABLE_SCHNORR support to sigencoding.Feb 1 2019, 16:53

rename IsValidECDSASignatureEncoding to IsValidDERSignatureEncoding

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

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

Harbormaster completed remote builds in B4819: Diff 7097.Feb 1 2019, 18:15

Use template for CheckTransactionSignatureEncoding and
CheckTransactionECDSASignatureEncoding

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

OK, I put in the template to implement CheckTransactionSignatureEncoding and CheckTransactionECDSASignatureEncoding

src/script/sigencoding.cpp
27 ↗	(On Diff #7093)	OK, renamed to `IsValidDERSignatureEncoding`
244 ↗	(On Diff #7093)	OK, I got the template working. The compiler didn't like the parameter names (vchSig, flags, serror) repeated, so I used the names "templateSig", "templateFlags", and "templateSerror" because I couldn't think of anything better. Let me know if you would prefer other variable names.