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 Passed
Unit
No Test Coverage
Build Status
Buildable 6219
Build 10485: Bitcoin ABC Buildbot (legacy)
Build 10484: arc lint + arc unit

Event Timeline

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

tweak names; add multisig test too

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 marked an inline comment as done.

update comment per suggestion