Page MenuHomePhabricator

Add SCRIPT_ENABLE_SCHNORR support to sigencoding
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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 subscriber: schancel. · View Herald Transcript
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 ↗(On Diff #7079)

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.

markblundeberg edited the test plan for this revision. (Show Details)Jan 31 2019, 22:19
Mengerian added inline comments.
src/script/sigencoding.cpp
223 ↗(On Diff #7079)

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.

deadalnix requested changes to this revision.Feb 1 2019, 01:20
deadalnix added inline comments.
src/script/sigencoding.cpp
223 ↗(On Diff #7079)

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 added inline comments.Feb 1 2019, 13:33
src/script/sigencoding.cpp
223 ↗(On Diff #7079)

@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 ↗(On Diff #7079)

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 ↗(On Diff #7079)

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.

markblundeberg updated this revision to Diff 7094.Feb 1 2019, 16:48

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

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)
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.

This revision now requires changes to proceed.Feb 1 2019, 17:06
markblundeberg edited the summary of this revision. (Show Details)Feb 1 2019, 23:17
markblundeberg edited the test plan for this revision. (Show Details)
markblundeberg updated this revision to Diff 7113.

rebase; add comments; use new lcg.h test module

deadalnix accepted this revision.Feb 1 2019, 23:35
deadalnix added inline comments.
src/test/sigencoding_tests.cpp
413 ↗(On Diff #7113)

negate hasSchnorr

This revision is now accepted and ready to land.Feb 1 2019, 23:35
This revision was automatically updated to reflect the committed changes.