ECSignature: fixes for canonical signatures #220
Merged
Conversation
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
Contributor
Author
There was a problem hiding this comment.
Is this suitable? Or should we just enforce secp256k1 as we have in other parts. If so, the signature length is at most going to be less than 73 bytes if so, as enforced by the reference implementation.
Contributor
There was a problem hiding this comment.
I support enforcing secp256k1 until we have a need for supporting other curves
Contributor
Author
There was a problem hiding this comment.
That won't help us if we ever use another ecdsa (with its own ECSignature) library other than the one currently included.
src/ecsignature.js
Outdated
Contributor
There was a problem hiding this comment.
If you don't intend to differentiate the two error cases, why not just combine them? assert(hashTypeMod > 0x00 && hashTypeMod < 0x04, 'Invalid hashType')
Contributor
|
+1 |
1 similar comment
Contributor
|
+1 |
kyledrake
added a commit
that referenced
this pull request
Jun 22, 2014
ECSignature: fixes for canonical signatures
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This pull request accounts for all the edge cases allowed by the Bitcoin client, and adds the respective core tests to
tests/bitcoin.core.js.Namely adding the missing test cases for:
R<0,S<0, and checking the DER padding is not extraneous.Hash type checking has also been added, but to avoid the circular dependency the boundaries were hard coded. This is not ideal, but I'm still not sure if
*ScriptSignatureis relevant toECSignatureorScript. It seems odd to haveTransactionas a dependency ofECSignature.Thoughts on this?
Reference for implementation: https://github.com/bitcoin/bitcoin/blob/master/src/script.cpp#L241