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

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.

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

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

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.

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)
markblundeberg edited the test plan for this revision. (Show Details)

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

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