Page MenuHomePhabricator

Add script tests with valid 64-byte ECDSA signatures.
ClosedPublic

Authored by markblundeberg on Jun 8 2019, 14:15.

Details

Summary

It would be a real pain to get these through grinding, but we can do a
cheat by specifying an arbitrary DER signature and then using recovery to
get the required public key that makes it valid. This only works if the public
key is in scriptSig, so these aren't exactly secure locking scripts.

This adds a test before/after Schnorr flag activation showing that the same
valid 64-byte ECDSA signature gets rejected upon activation. Also tests 63/65
bytes. We knew how to do this before the upgrade of course (credit to Florian
for pointing out the mechanism), but didn't want to give people any ideas that
would spoil retroactivity.

This will come in handy for multisignature tests.

For fun, a test is included that uses ECDSA's quirky wrapped r value; this
can only occur for r <= 0x14551231950b75fc4402da1722fc9baed so it is basically
impossible to happen in normal spends.

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
add_backwards_ecdsa
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6237
Build 10521: Bitcoin ABC Buildbot (legacy)
Build 10520: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jun 8 2019, 14:15
Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 8 2019, 14:15
markblundeberg updated this revision to Diff 9255.Jun 8 2019, 14:16

recomment json-gen line

markblundeberg edited the test plan for this revision. (Show Details)Jun 8 2019, 14:19
Harbormaster completed remote builds in B6220: Diff 9255.
markblundeberg edited the summary of this revision. (Show Details)Jun 8 2019, 15:26
Mengerian accepted this revision as: Mengerian.Jun 8 2019, 19:20

This looks good to me. I played around with this a bit, it all seems to work.

I think it would be good to land this before D3253, so that people have a revision that they can build, and it will accept 64-byte ECDSA signatures. This is good to show that the tests work. Then D3253 will have to be rebased on top of this Diff.

This revision is now accepted and ready to land.Jun 8 2019, 19:20
markblundeberg updated this revision to Diff 9270.Jun 9 2019, 03:11

tweak names; add multisig test too

markblundeberg requested review of this revision.Jun 9 2019, 05:59
Mengerian accepted this revision as: Mengerian.Jun 9 2019, 18:52

Checked throught the Diff again, looks good to me.

I like the new naming scheme of "recovered-pubkey".

I left a minor suggestion for a comment to clarify what's going on.

src/test/script_tests.cpp
2352 ↗(On Diff #9270)

I'd suggest maybe putting a comment in here to explain that this value is basically arbitrary, since that's not obvious from just looking at the value.

Maybe something like:
// Set r value to an arbitrary number ~29 bytes long

This revision is now accepted and ready to land.Jun 9 2019, 18:52
markblundeberg updated this revision to Diff 9276.Jun 9 2019, 22:33
markblundeberg marked an inline comment as done.

update comment per suggestion