Skip to content

Conversation

@dai1975
Copy link
Collaborator

@dai1975 dai1975 commented Jun 12, 2025

Fix S256() curve compare fails in docker enviroments.

https://github.com/ethereum/go-ethereum/blob/master/crypto/signature_nocgo.go#L82

@dai1975 dai1975 requested review from Copilot and siburu June 12, 2025 10:26

This comment was marked as outdated.

@dai1975 dai1975 force-pushed the fix/S256-mismatch branch from 291f57e to 8c1e10a Compare June 12, 2025 10:27
@dai1975 dai1975 requested a review from Copilot June 12, 2025 10:28

This comment was marked as outdated.

@siburu
Copy link
Contributor

siburu commented Jun 12, 2025

@dai1975 Thank you, but why did you notice this issue?
It seems that any relayer built with nocgo (default) suffers this issue.
Then, why haven't test cases such as yui-relayer-build's tm2eth encountered with this issue until now?

Ah, I resolved it myself.
On typical Linux environments, go env returns CGO_ENABLED=1 by default.
In other words, nocgo is not the default setting, and so this issue has not surfaced.

Comment on lines 131 to 133
// The ecdsaPrivKey.Curve points to the dcrd.secp256k1.S256() instance,
// but in some environments (e.g., Docker containers) the comparison with gethcrypto.S256() may fail.
// To ensure consistency, we replace the Curve with gethcrypto.S256().'
Copy link
Contributor

@siburu siburu Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation is OK, but probably, this issue description is not accurate.
Since dcrd.secp256k1.S256() and gethcrypto.S256() are different objects, pointer comparison between them ALWAYS fails.

The real issue is implementation difference between signatrure_nocgo.go and signature_cgo.go in the go-ethereum/crypto package.
The former implementation requires prv.Curve == gethcrypto.S256() but the latter doesn't.
In typical environments, the latter implementation is chosen to build relayers, and so this issue doesn't pop up.
In some environments, like Docker containers with no C compilers installed, the former implementation is used.

Could you fix the comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks I did not understand real cause.
I've rewritten that comment.

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>
@dai1975 dai1975 force-pushed the fix/S256-mismatch branch from 8c1e10a to ce80e1f Compare June 13, 2025 01:33
@dai1975 dai1975 requested a review from Copilot June 13, 2025 01:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses an issue with comparing the S256() curve in docker environments by forcing the use of gethcrypto.S256() when necessary.

  • Replaces direct return of btcecPrivKey.ToECDSA() with an intermediate assignment.
  • Adds a conditional to reassign ecdsaPrivKey.Curve to gethcrypto.S256() if it does not match.
Comments suppressed due to low confidence (1)

pkg/wallet/wallet.go:134

  • Comparing the curve by pointer equality after checking the name may be brittle if the underlying implementation changes. Consider caching the gethcrypto.S256() value in a local variable to improve clarity and ensure consistent comparisons.
if ecdsaPrivKey.Curve.Params().Name == "secp256k1" && ecdsaPrivKey.Curve != gethcrypto.S256() {

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@dai1975 dai1975 requested a review from siburu June 13, 2025 01:41
Copy link
Contributor

@siburu siburu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dai1975 LGTM!

@dai1975 dai1975 merged commit 1c89c5a into main Jun 13, 2025
1 check passed
@dai1975 dai1975 deleted the fix/S256-mismatch branch June 13, 2025 02:55
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.

3 participants