Paths

Table of Contentst

Differential D2469

Add SCRIPT_ENABLE_SCHNORR support to sigencoding
ClosedPublic
Actions

Authored by markblundeberg on Jan 31 2019, 21:56.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Maniphest Tasks

T527: Add Schnorr support to OP_CHECKSIG and OP_CHECKDATASIG

Commits

rSTAGINGcfd7f1b65be4: Add SCRIPT_ENABLE_SCHNORR support to sigencoding
rABCcfd7f1b65be4: Add SCRIPT_ENABLE_SCHNORR support to sigencoding

Summary

(includes a new error code SCRIPT_ERR_SIG_BADLENGTH)

motivation for banning 65 byte sigs in CHECKMULTISIG: When Schnorr signatures (64 bytes + 1 hashtype byte) are added, we have the option of leaving CHECKMULTISIG untouched which would mean it would still accept 64+1 byte signatures and interpret them as ECDSA while at the same time, CHECKSIG would accept 64+1 byte signatures and interpret them as Schnorr. This is a sort of awkward ambiguity however, and hopefully it will create less ambiguity, and less risk of consensus break with other implementations, if post-fork *all* 64+1 byte signatures are now Schnorr signatures.

Depends on D2477 and D2475

Test Plan

added direct tests in sigencoding_tests
added script_tests that ensure each opcode is calling the right function

Diff Detail

Repository

rABC Bitcoin ABC

Branch

ban-64ecdsa

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4802
Build 7667: Bitcoin ABC Buildbot (legacy)
Build 7666: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jan 31 2019, 21:56

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 31 2019, 21:56

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.

markblundeberg retitled this revision from ban 64-byte signatures in CHECKMULTISIG when SCRIPT_ENABLE_SCHNORR to ban 64+1 byte signatures in CHECKMULTISIG when SCRIPT_ENABLE_SCHNORR.Jan 31 2019, 21:57

deadalnix added inline comments.Jan 31 2019, 22:01

src/test/data/script_tests.json
1579	Use `CHECKSIG NOT` so you can check it eval to true, so you know there is not some other weird failure that cause the thing to be false.

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

Harbormaster completed remote builds in B4802: Diff 7079.Jan 31 2019, 22:11

markblundeberg edited the test plan for this revision. (Show Details)Jan 31 2019, 22:19

Mengerian added a subscriber: Mengerian.Jan 31 2019, 23:56

Mengerian added inline comments.

src/script/sigencoding.cpp
223	Not sure if it's worth changing, but to me the code organization seems a bit confusing. It would seem more natural to me to have CheckTransactionSignatureEncoding be the outer function that wraps CheckTransactionECDSASignatureEncoding (and also checking Schnorr encoding when that is added) within it. This would change D2466 which is already landed. Then everything that deals with checking ECDSA would all be within CheckTransactionECDSASignatureEncoding. As it is now, the ECDSA signature checking happens in two places, the 64-byte check happens out here, and then the rest of the DER checks happen inside CheckTransactionSignatureEncoding. It makes the code confusing IMO.

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

deadalnix requested changes to this revision.Feb 1 2019, 01:20

deadalnix added inline comments.

src/script/sigencoding.cpp
223	Yes. You probably want to extract the code that manage the sighashtype into its own function and call that function from both `CheckTransactionSignatureEncoding` and `CheckTransactionECDSASignatureEncoding`. Both should also call `CheckRawSignatureEncoding` . That one may/should also be renamed as it is now ACDSA specific (it always was but now it is ambiguous).

This revision now requires changes to proceed.Feb 1 2019, 01:20

markblundeberg edited the summary of this revision. (Show Details)Feb 1 2019, 02:28

Mengerian mentioned this in D2477: Refactor signature encoding checks.Feb 1 2019, 13:19

Mengerian added inline comments.Feb 1 2019, 13:33

src/script/sigencoding.cpp
223	@deadalnix I created D2477 which is a minimal refactor to pave the way for this Diff. After the refactor, this portion of the code can go into CheckRawECDSASignatureEncoding I will also work on a refactor of the sighashtype checks, but that doesn't need to be a blocker for this Diff IMO.

markblundeberg added inline comments.Feb 1 2019, 13:46

src/script/sigencoding.cpp
223	The refactor of sighashtypes would have to be also done before it's usable here, see comments on that diff. :D

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

src/script/sigencoding.cpp
223	OK, I actually ended up doing the full refactor in D2477. It was too confusing to split it. So once that Diff is landed, this one should be ready to go.

rebased on top of D2477 and D2475 ; finished changes to sigencoding; added sigencoding_tests.

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

markblundeberg retitled this revision from ban 64+1 byte signatures in CHECKMULTISIG when SCRIPT_ENABLE_SCHNORR to Add SCRIPT_ENABLE_SCHNORR support to sigencoding.Feb 1 2019, 16:49

markblundeberg edited the summary of this revision. (Show Details)

markblundeberg edited the test plan for this revision. (Show Details)

markblundeberg edited the summary of this revision. (Show Details)Feb 1 2019, 16:53

markblundeberg added parent revisions: D2477: Refactor signature encoding checks, D2475: sigencoding_tests : use pseudorandom generator for flag patterns.

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

Harbormaster completed remote builds in B4816: Diff 7094.Feb 1 2019, 17:03

deadalnix requested changes to this revision.Feb 1 2019, 17:06

deadalnix added inline comments.

src/script/sigencoding.cpp
177 ↗	(On Diff #7094)	Add a comment.
src/test/sigencoding_tests.cpp
432 ↗	(On Diff #7094)	3rd use of that guy. It definitively deserve to be its own class with its own test.