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.
Details
Details
- Reviewers
deadalnix Fabien Mengerian - Group Reviewers
Restricted Project - Maniphest Tasks
- T527: Add Schnorr support to OP_CHECKSIG and OP_CHECKDATASIG
- Commits
- rSTAGINGde9774b507b2: Enable Schnorr signature verification in CHECK(DATA)SIG(VERIFY)
rABCde9774b507b2: Enable Schnorr signature verification in CHECK(DATA)SIG(VERIFY)
added numerous tests in script_tests.
also, new opcode-probing module that uses pseudorandom flags testing.
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- enable_schnorr_opcodes
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 4836 Build 7735: Bitcoin ABC Buildbot (legacy) Build 7734: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
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.
Comment Actions
Possibly the whole business about generating valid 64 byte ECDSA sigs can be taken out...
Comment Actions
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: 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. |
Comment Actions
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. |
Comment Actions
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")
Comment Actions
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.
Comment Actions
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 | Test case missing:
| |
| 1649 | Same as for CHECKSIG | |
| 1703 | Same as for checksig plus:
| |
| 1757 | Same as CHECKDATASIG | |
| 1811 | 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 | Stick comments on test cases. | |
| 1878 | You need to check with strict der encoding as well. | |
| 1902 | Ok so these are actually what I asked for before. I think you should just move these in proper sections. | |
| 1954 | dito. It's confusing to not see this next to the other multisig tests. | |
| 1975 | We also probably need a test with an uncompressed pubkey | |
| 2013 | Good call, I did not think of this. | |
Comment Actions
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.
| src/test/script_tests.cpp | ||
|---|---|---|
| 1811 |
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. | |
Comment Actions
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.
| 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. |
Comment Actions
naming changes; add comments
- schnorrsize_tests -> schnorr_tests
- expand out variable names for readability
| 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. |
| src/test/script_tests.cpp | ||
|---|---|---|
| 1579 ↗ | (On Diff #7148) | Yep, I was thinking the same. |