Make SCRIPT_VERIFY_STRICTENC compatible with BIP62 #5004

pull sipa wants to merge 1 commits into bitcoin:master from sipa:bip62a changing 8 files +234 −164

sipa commented at 11:25 PM on September 29, 2014: member
- Delete canonical_tests.cpp, and move the tests to script_tests.cpp.
- Split off SCRIPT_VERIFY_DERSIG from SCRIPT_VERIFY_STRICTENC (the BIP62 part of it).
- Change signature STRICTENC/DERSIG semantics to fail the script entirely rather than the CHECKSIG result (softfork safety, and BIP62 requirement).
- Add autogenerated tests for several odd cases.
sipa force-pushed on Sep 29, 2014
BitcoinPullTester commented at 1:18 AM on September 30, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p5004_1039e0a6cac595871710055408e60453f23fe09f/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
sipa commented at 12:37 AM on October 1, 2014: member

@schildbach @petertodd You guys may be interesting in this. Note the new DERSIG flag, and the changed semantics for encoding errors for the signatures.
laanwj commented at 10:45 AM on October 1, 2014: member

ACK
sipa commented at 1:59 AM on October 2, 2014: member

@gavinandresen @gmaxwell @jgarzik Comments?
laanwj cross-referenced this on Oct 2, 2014 from issue Remove core dependencies from CScript by theuni
in src/script/interpreter.cpp:None in 1039e0a6ca outdated
```
  55 | @@ -56,10 +56,7 @@ static inline void popstack(vector<valtype>& stack)
  56 |      stack.pop_back();
  57 |  }
  58 |  
  59 | -bool IsCanonicalPubKey(const valtype &vchPubKey, unsigned int flags) {
  60 | -    if (!(flags & SCRIPT_VERIFY_STRICTENC))
  61 | -        return true;
  62 | -
  63 | +bool IsStandardPubKey(const valtype &vchPubKey) {
```
gavinandresen commented at 3:41 PM on October 2, 2014:

I was confused by the use of the word "Standard" here when reviewing. Why the rename from Canonical to Standard? I think of "standard" pubkeys as the standard script forms that the wallet recognizes.

sipa commented at 5:37 PM on October 2, 2014:

'Canonical' seems to imply there is One Single Truth, but as several encoding rules get added through BIP62, that is no longer the case, so I want to get rid of that and have separate functions that check separate (and mostly independent) encoding rules. I agree 'standard' has the wrong meaning as well. I could go for 'IsCompressedOrUncompressedPubKey()' ?
gavinandresen commented at 3:42 PM on October 2, 2014: contributor

Code changes look good, except for a naming nit.
sipa force-pushed on Oct 2, 2014
sipa commented at 6:16 PM on October 2, 2014: member

Updated to use 'IsCompressedOrUncompressedPubKey()'.
sipa force-pushed on Oct 3, 2014
sipa force-pushed on Oct 6, 2014
sipa force-pushed on Oct 7, 2014
sipa commented at 7:53 AM on October 7, 2014: member

Rebased on top of #5055.
sipa force-pushed on Oct 8, 2014
sipa force-pushed on Oct 8, 2014
gmaxwell commented at 10:41 PM on October 8, 2014: contributor

ACK

Make SCRIPT_VERIFY_STRICTENC compatible with BIP62

* Delete canonical_tests.cpp, and move the tests to script_tests.cpp.
* Split off SCRIPT_VERIFY_DERSIG from SCRIPT_VERIFY_STRICTENC (the BIP62 part of it).
* Change signature STRICTENC/DERSIG semantics to fail the script entirely rather than the CHECKSIG result (softfork safety, and BIP62 requirement).
* Add many autogenerated tests for several odd cases.
* Mention specific BIP62 rules in the script verification flags.

9df9cf5a9f

sipa force-pushed on Oct 8, 2014
sipa merged this on Oct 8, 2014
sipa closed this on Oct 8, 2014
sipa referenced this in commit bf7b4fc6d3 on Oct 8, 2014
sipa cross-referenced this on Dec 16, 2014 from issue remove sig_canonical.json and sig_noncanonical.json by maraoz
fanquake cross-referenced this on Jan 3, 2015 from issue Canonical signature tests contain an invalid signature by davecgh
bitcoin locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4625 Canonical signature tests contain an invalid signature #4981 Remove core dependencies from CScript #5055 Prettify JSON tests and minimize diffs #5491 remove sig_canonical.json and sig_noncanonical.json