Page MenuHomePhabricator

Enable Schnorr signature verification in CHECK(DATA)SIG(VERIFY)
ClosedPublic

Authored by markblundeberg on Jan 30 2019, 21:54.

Details

Summary

previously in D2469 the sigencoding changes were done; this completes the
schnorr implementation by modifying VerifySignature. Mostly this is a battery
of tests on the opcodes themselves.

Test Plan

added numerous tests in script_tests.
also, new opcode-probing module that uses pseudorandom flags testing.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
markblundeberg added inline comments.Jan 31 2019, 18:45
src/script/interpreter.cpp
1037 ↗(On Diff #7063)

true, CheckPubKeyEncoding doesn't care about this flag

src/script/sigencoding.cpp
161 ↗(On Diff #7063)

ah, interesting idea

markblundeberg updated this revision to Diff 7117.Feb 2 2019, 00:16
markblundeberg retitled this revision from add Schnorr support to opcodes to add Schnorr support to opcodes' VerifySignature.
markblundeberg edited the summary of this revision. (Show Details)

rebased

Note that when the old tests were applied, 7 of them failed due to the change in specification to ban 64+1-byte ECDSA sigs in CHECKMULTISIG. They were updated accordingly.

Possibly the whole business about generating valid 64 byte ECDSA sigs can be taken out...

deadalnix requested changes to this revision.Feb 2 2019, 00:43

I stopped the review of test cases mid way because they are disorganized and with non descriptive comments, which makes it basically impossible to know if all bases are covered.

src/test/script_tests.cpp
237 ↗(On Diff #7117)

Never include in the middle of a file

238 ↗(On Diff #7117)

you don't need extern C

279 ↗(On Diff #7117)

I find it a bit unfortunate that this require to be added. This create a lot more dependencies. I don't really think it is require as you can do CHECKSIG NOT on a signature that is properly encoded as long as you odn't pass the NULLFAIL flag.

1620 ↗(On Diff #7117)

Other groups use //

1642 ↗(On Diff #7117)

Check that it works with another key + that it fails with mismatched public/private keys + check that it passes with strict encoding+schnorr.

1686 ↗(On Diff #7117)

This last one seems to be testing P2SH. It's not really necessary. It doesn't test anything related to schnorr.

1764 ↗(On Diff #7117)

You should have 3 time the same pattern here:
1/ classic ECDSA with no flag/schnorr/strictenc/schnorr+strictenc
2/ schnorr, same set of flags.
3/ 64 bytes ECDSA + same set of flag. An OP_NOT can be added so the fact the sig is invalid isn't a problem.

Without some order in there, it's impossible to figure out if you covered all bases.

1767 ↗(On Diff #7117)

Please use more descriptive description. illegal doesn't help at all figuring out what this is about. It gets worse if you consider this not in isolation but in association with all the other tests that already exist.

1776 ↗(On Diff #7117)

You reverse the order of the tests compared to other tests cases. This is confusing. This end up being the test above without the schnorr flag, which is not clear from anything.

This revision now requires changes to proceed.Feb 2 2019, 00:43

OK, thanks for the in depth ideas on how to organize this.

Upon further thought I think I'll leave out the 'make valid but nonstrict 64 byte ECDSA' thing with secp256k1.h. I think that was a bit more relevant before, when we were passing these signatures through CHECKMULTISIG with Schnorr flag on instead of banning them. It was a fun trick, though. :-D

src/test/script_tests.cpp
279 ↗(On Diff #7117)

Yes, the previous sigencoding diff added some tests to script_tests.json that did this sort of thing. These can be expanded if needed.

markblundeberg marked 9 inline comments as done.Feb 2 2019, 02:10
markblundeberg updated this revision to Diff 7120.Feb 2 2019, 02:48

rewrote tests; removed trick for generating valid 64-byte ecdsa

64-byte checks already covered both in sigencoding_tests.cpp AND in script_tests.json (see section "64/65-byte sig length tests")

markblundeberg edited the test plan for this revision. (Show Details)Feb 2 2019, 05:05

You may want to add a unit test for similar to checkdatasig_tests (also, that guys would benefit from using the LCG) for the schnorr sigs. Unit test allows to be more thorough. script_tests allows to have test vector everybody can reuse or test against. Both have their purpose an are necessary.

deadalnix requested changes to this revision.Feb 2 2019, 16:06

So there are tests missing. Most notably, there should be a unit test leveraging LCG to test things with all kind of flags.

src/test/script_tests.cpp
1605 ↗(On Diff #7120)

Test case missing:

  • Add a test with a different key (it passes).
  • Add a test with mismatched public/private keys (it fails).
1649 ↗(On Diff #7120)

Same as for CHECKSIG

1703 ↗(On Diff #7120)

Same as for checksig plus:

  • check with one different, but correct message
  • one erroneous message.
1757 ↗(On Diff #7120)

Same as CHECKDATASIG

1811 ↗(On Diff #7120)

You need to check that a 64 bytes ECDSA sig will be rejected with the proper error. The sig do not need to be valid, you can do CHECKMULTISIG NOT

You also need a CHECKMULTISIG of more than 1-1 so that you can check mixed schnorr/ECDSA.

1875 ↗(On Diff #7120)

Stick comments on test cases.

1878 ↗(On Diff #7120)

You need to check with strict der encoding as well.

1902 ↗(On Diff #7120)

Ok so these are actually what I asked for before. I think you should just move these in proper sections.

1954 ↗(On Diff #7120)

dito. It's confusing to not see this next to the other multisig tests.

1975 ↗(On Diff #7120)

We also probably need a test with an uncompressed pubkey

2013 ↗(On Diff #7120)

Good call, I did not think of this.

This revision now requires changes to proceed.Feb 2 2019, 16:06

You may want to add a unit test for similar to checkdatasig_tests (also, that guys would benefit from using the LCG) for the schnorr sigs. Unit test allows to be more thorough. script_tests allows to have test vector everybody can reuse or test against. Both have their purpose an are necessary.

Interesting idea. I think this is feasible as long as we feed strictly invalid signatures so we don't have to build a fund/spend transaction setup. Then we can just use BaseSignatureChecker whose CheckSig always returns false regardless, or, TransactionSignatureChecker with dummy data.

markblundeberg added inline comments.Feb 2 2019, 16:14
src/test/script_tests.cpp
1902 ↗(On Diff #7120)

ah so you mean this should be retested on every opcode?

1954 ↗(On Diff #7120)

hmm, I thought it was clearer to put here as it falls outside the pattern (no equivalent check can be done on CHECKSIG/CHECKDATASIG)

markblundeberg added inline comments.Feb 2 2019, 16:23
src/test/script_tests.cpp
1811 ↗(On Diff #7120)

You need to check that a 64 bytes ECDSA sig will be rejected with the proper error. The sig do not need to be valid, you can do CHECKMULTISIG NOT

This is actually already checked, though it was added directly in script_tests.json in the previous Diff. Look for the section called ""64/65-byte sig length tests" in script_tests.json, already landed.

markblundeberg updated this revision to Diff 7129.Feb 3 2019, 06:53

add new test module for 64/65 byte sigs ; tweaks on script_tests

Mengerian requested changes to this revision.Feb 3 2019, 17:32

Can the title of this be changed? I find it a bit difficult to parse.

Something like "Enable Schnorr signature verification in CHECK(DATA)SIG(VERIFY)" would be clearer IMO.

This revision now requires changes to proceed.Feb 3 2019, 17:32
markblundeberg retitled this revision from add Schnorr support to opcodes' VerifySignature to Enable Schnorr signature verification in CHECK(DATA)SIG(VERIFY).Feb 3 2019, 20:24
markblundeberg edited the test plan for this revision. (Show Details)

Can the title of this be changed? I find it a bit difficult to parse.

Something like "Enable Schnorr signature verification in CHECK(DATA)SIG(VERIFY)" would be clearer IMO.

👍 done

Mengerian accepted this revision as: Mengerian.Feb 3 2019, 20:46
markblundeberg requested review of this revision.Feb 4 2019, 00:09
deadalnix requested changes to this revision.Feb 4 2019, 02:24
deadalnix added inline comments.
src/test/schnorrsize_tests.cpp
1 ↗(On Diff #7129)

Just schnorr_test, it test way more than the size. It actually test end to end schnorr.

86 ↗(On Diff #7129)

Use words.

119 ↗(On Diff #7129)

Please chose more explicit names. I have no idea what hb stands for and there is n tax on extra characters yet.

211 ↗(On Diff #7129)

This whole test is really unreadable. Plaese pick proper names for all this. I don't want to be tracking what obscure acronyms stands for, making sure all the test cases are present and behave as expect is hard enough.

This revision now requires changes to proceed.Feb 4 2019, 02:24
markblundeberg updated this revision to Diff 7148.Feb 4 2019, 03:45

naming changes; add comments

  • schnorrsize_tests -> schnorr_tests
  • expand out variable names for readability
deadalnix requested changes to this revision.Feb 4 2019, 13:13
deadalnix added inline comments.
src/test/schnorr_tests.cpp
74 ↗(On Diff #7148)

Why do you need VERIFY here ?

80 ↗(On Diff #7148)

dito

219 ↗(On Diff #7148)

This isn't testing any valid ECDSA or schnorr signatures.

This revision now requires changes to proceed.Feb 4 2019, 13:13
deadalnix accepted this revision.Feb 4 2019, 17:36
deadalnix added inline comments.
src/test/script_tests.cpp
1579 ↗(On Diff #7148)

You should rename this to make it explicit this is ECDSA. It doesn't need to be part of this patch.

This revision is now accepted and ready to land.Feb 4 2019, 17:36
markblundeberg added inline comments.Feb 4 2019, 19:49
src/test/script_tests.cpp
1579 ↗(On Diff #7148)

Yep, I was thinking the same.

markblundeberg updated this revision to Diff 7171.Feb 4 2019, 20:42

rebased (fixed merge conflict with other added script_tests)