Skip to content

Conversation

@prusnak
Copy link
Contributor

@prusnak prusnak commented Feb 9, 2014

Bitcoin uses canonized format for signatures which enforce enforce low S values, by negating the value (modulo the order) if S is above order/2.

See https://github.com/bitcoin/bitcoin/blob/master/src/key.cpp#L214

I don't want to break existing unit tests and cleanest way how to add this functionality seems to be the introduction of canonized sigencode methods. Or do you think there is a better way?

these enforce low S values, by negating the value (modulo the order) if above order/2
@prusnak
Copy link
Contributor Author

prusnak commented Feb 10, 2014

Including https://en.bitcoin.it/wiki/Transaction_Malleability#Signature_Malleability for reference:

"In addition for every ECDSA signature (r,s), the signature (r, -s (mod N)) is a valid signature of the same message. Efforts are underway to first make Bitcoin nodes not relay non-standard signatures, and eventually disallow them from being included in new blocks entirely."

@warner
Copy link
Collaborator

warner commented Feb 16, 2014

What about adding a "canonicalize=bool" optional argument to those methods? With a default of "False"?

@prusnak
Copy link
Contributor Author

prusnak commented Feb 16, 2014

That would work if we would like to use these methods directly, but we are passing them as a parameter to sign_digest, e.g.:

signature = private_key.sign_digest_deterministic(tx_hash, hashfunc=sha256, sigencode=ecdsa.util.sigencode_der_canonize)

Or is there a pythonic way to achieve the same functionality with your method?

@warner
Copy link
Collaborator

warner commented Feb 18, 2014

Ah, sorry, I didn't look closely enough at your patch. Yes, given our API, creating extra encoding functions is probably the best approach. I'll figure out the Travis problem and merge this. Thanks!

warner pushed a commit that referenced this pull request Feb 18, 2014
canonical versions of sigencode methods
@warner warner merged commit 1a9453d into tlsfuzzer:master Feb 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants