-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update btcec to v2 and use the latest btcutil #9250
Conversation
We have recently changed the default branch to Is |
I have checked the test cases from both the older and newer versions. The are the same. I have also updated the tests in tendermint to make sure the signature verification tests pass. Hence, the new version should still be able to verify old secp256k1 signatures. Of course, as always, more tests on it will be better. I will finish the rebase as soon as possible, but it is not trivial. In the worst case, I will submit a new PR. |
@cmwaters I have rebased my branch to |
Kindly cc-ing @veorq and @elias-orijtech to help with a security review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than a suggestion about splitting the change in two; leaving for @veorq.
|
||
sig, err := priv.Sign(crypto.Sha256(msg)) | ||
sig, err := ecdsa.SignCompact(priv, crypto.Sha256(msg), false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using https://pkg.go.dev/github.com/btcsuite/btcd/btcec/v2/ecdsa#Sign to gain a smaller diff. Then, a follow-up commit (maybe same PR) can replace Sign
with SignCompact
and delete serializeSig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just found that they no longer expose R
and S
as exported fields anymore(https://github.com/decred/dcrd/blob/dcrec/secp256k1/v4.1.0/dcrec/secp256k1/ecdsa/signature.go#L52) so logic in serializeSig
will no longer work.
Do you think we still need to break it into two commits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks. In that case, using SignCompact
is probably ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the minor changelog nit, I'd recommend considering the change mentioned by @elias-orijtech.
@wcsiu just checking in here. This seems like a valuable change for us to have, but it needs some minor updates. |
I agree with your suggestions. I will update the code according to them. I am having a busy week. I will try to finish it in the coming few days. Thank you. |
No rush 🙂 Just wanted to check in. Thanks very much! |
@thanethomson @elias-orijtech has agreed the fix is no longer necessary. I think the PR should now be ready. |
Co-authored-by: Thane Thomson <connect@thanethomson.com>
Similar to Tendermint's PR, tendermint/tendermint#9250 Note that crypto/ledger is not updated in this PR, because if its dependency on the R and S values being exposed by ParseDERSignature. Updates cosmos#13423 Signed-off-by: Elias Naur <elias@orijtech.com>
Similar to Tendermint's PR, tendermint/tendermint#9250 Note that crypto/ledger is not updated in this PR, because if its dependency on the R and S values being exposed by ParseDERSignature. Updates cosmos#13423 Signed-off-by: Elias Naur <elias@orijtech.com>
Similar to Tendermint's PR, tendermint/tendermint#9250 Note that crypto/ledger is not updated in this PR, because if its dependency on the R and S values being exposed by ParseDERSignature. Updates cosmos#13423 Signed-off-by: Elias Naur <elias@orijtech.com>
Similar to Tendermint's PR, tendermint/tendermint#9250 Note that crypto/ledger is not updated in this PR, because if its dependency on the R and S values being exposed by ParseDERSignature. Updates #13423 Signed-off-by: Elias Naur <elias@orijtech.com> Signed-off-by: Elias Naur <elias@orijtech.com> Co-authored-by: Marko <marbar3778@yahoo.com>
* secp256k1: upgrade to latest btcec v2 and btcutil * Update CHANGELOG_PENDING.md Co-authored-by: Thane Thomson <connect@thanethomson.com> * fix rebase Co-authored-by: Thane Thomson <connect@thanethomson.com>
…9250) (#9784) * update btcec to v2 and use the latest btcutil (#9250) * secp256k1: upgrade to latest btcec v2 and btcutil * Update CHANGELOG_PENDING.md Co-authored-by: Thane Thomson <connect@thanethomson.com> * fix rebase Co-authored-by: Thane Thomson <connect@thanethomson.com> * Minor spacing Co-authored-by: Wachiu Siu <5212960+wcsiu@users.noreply.github.com> Co-authored-by: Thane Thomson <connect@thanethomson.com>
* secp256k1: upgrade to latest btcec v2 and btcutil * Update CHANGELOG_PENDING.md Co-authored-by: Thane Thomson <connect@thanethomson.com> * fix rebase Co-authored-by: Thane Thomson <connect@thanethomson.com>
…9250) (#9787) * update btcec to v2 and use the latest btcutil (#9250) * secp256k1: upgrade to latest btcec v2 and btcutil * Update CHANGELOG_PENDING.md Co-authored-by: Thane Thomson <connect@thanethomson.com> * fix rebase Co-authored-by: Thane Thomson <connect@thanethomson.com> * go mod tidy Co-authored-by: Wachiu Siu <5212960+wcsiu@users.noreply.github.com> Co-authored-by: Thane Thomson <connect@thanethomson.com>
Similar to Tendermint's PR, tendermint/tendermint#9250 Note that crypto/ledger is not updated in this PR, because if its dependency on the R and S values being exposed by ParseDERSignature. Updates #13423 Signed-off-by: Elias Naur <elias@orijtech.com> Signed-off-by: Elias Naur <elias@orijtech.com> Co-authored-by: Marko <marbar3778@yahoo.com>
PR checklist
CHANGELOG_PENDING.md
updated, or no changelog entry neededdocs/
) and code comments, or nodocumentation updates needed
Closes #9249