Fix usage of NegateSignatureS in script_tests#5784
Merged
laanwj merged 2 commits intobitcoin:masterfrom Feb 18, 2015
Merged
Conversation
NegateSignatureS is called with a signature without a hashtype, so do not save the last byte and append it after S negation. Updates the two tests which were affected by this bug.
The fix to NegateSignatureS caused a test which had been failing in IsValidSignatureEncoding to then fail in IsLowDERSignature. Add new test so the original check remains exercised.
Member
|
ACK |
1 similar comment
Contributor
|
ACK |
laanwj
added a commit
that referenced
this pull request
Feb 18, 2015
random-zebra
referenced
this pull request
in PIVX-Project/PIVX
Sep 15, 2021
a6d09a9 Policy: clean zc txs from the standard policies. (furszy) fcf8c87 Get rid of inaccurate ScriptSigArgsExpected (Pieter Wuille) 08c2874 Fix invalid memory access in CScript::operator+= (Anthony Towns) a01ee06 Adding P2SH(p2pkh) script test case (Chris Stewart) 1dab098 script_tests.json: 0xd1 and 0xd2 are reserved opcodes. (furszy) 8cdde3c Merge script_valid and script_invalid tests (Pieter Wuille) 67ccfda Get rid of expect in script_tests as it's implied by scripterror (Pieter Wuille) 555b146 Make script_error a mandatory 4th field for script_tests (Pieter Wuille) ca14c77 Fix some misconstructed tests (Pieter Wuille) 8b88936 Fix formatting of NOPs for generated script tests (Pieter Wuille) 587aef8 Fix JSON pretty printing in script_tests (Pieter Wuille) da4ef2b test: script_tests.cpp update. (furszy) 7e051a3 clarify obscure uses of EvalScript() (furszy) 4ee696c Make SCRIPT_VERIFY_CLEANSTACK a standardness requirement (furszy) 1f604ea Add SCRIPT_VERIFY_CLEANSTACK (BIP62 rule 6) (Pieter Wuille) 533edfa No semantic change: reuse stack variable in P2SH evaluation (Pieter Wuille) ce30d7b Add test for DER-encoding edge case (Suhas Daftuar) 7faeeb8 Fix NegateSignatureS to not duplicate last byte of S (Suhas Daftuar) 395ecb1 Document standard.h functions. (furszy) 9f4e950 Extract CSipHasher to it's own file in crypto/ directory. This is a move-only commit with the exception of changes to includes. (Jim Posen) Pull request description: We are way too behind in terms of the script unit tests, this PR pushes us further into upstream's direction. Back ported PRs: * dashpay#5143 * dashpay#5617 * dashpay#5784 * dashpay#5952 (de236f5 only) * dashpay#6075 * bitcoin#7387 * bitcoin#7517 * bitcoin#7818 * bitcoin#8090 * bitcoin#11284 * And 4fb789e ACKs for top commit: random-zebra: utACK a6d09a9 Fuzzbawls: ACK a6d09a9 Tree-SHA512: 60ee3e774f8e1c3cebf11d028442697725c93a61940c50b9776407ee3dd5a80bbfcca01eea5c6be43c78f4c215d4b238a6dcb640641e86e123b19bf1b7c33350
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NegateSignatureS is called with a signature which has not yet had the hashtype appended to it, yet the function assumed a hashtype was there and was incorrectly saving and then appending the last byte of its input (ie the last byte of the original S instead of a hashtype).
Only one pair of the test scripts was triggering this bug ("P2PK with high S"), which was actually causing the invalid version of the test to fail in the wrong place -- it was failing in the IsValidSignatureEncoding function rather than further down in IsLowDERSignature where the S value is checked. Fixing NegateSignatureS causes this test to change so that IsLowDERSignature is now being tested as I believe was intended.
Fixing that test resulted in there no longer being code coverage for the check in IsValidSignatureEncoding relating to extra bytes after the S, so the second commit here adds a pair of tests to exercise that check.