Tarak/go1.26 cgo only - #44
Open
tarakby wants to merge 11 commits into
Open
Conversation
tarakby
marked this pull request as ready for review
August 6, 2026 19:42
turbolent
reviewed
Aug 6, 2026
turbolent
left a comment
turbolent
left a comment
Member
There was a problem hiding this comment.
I reviewed the code from a Go-perspective, but I don't have the expertise to review the crypto changes
| } | ||
| } | ||
|
|
||
| func TestEllipticUnmarshalSecp256k1(t *testing.T) { |
Comment on lines
+172
to
+175
| pkBytes := make([]byte, 1+2*pLenSecp256k1) | ||
| pkBytes[0] = ecEncodingUncompressed | ||
| // pad x and y to the field size and concatenate them | ||
| padToSizeAndConcat(pkBytes[1:], x, y, pLenSecp256k1) |
Member
There was a problem hiding this comment.
This looks like it's duplicated in secp256k1DecodePublicKeyCompressed below. Maybe it can be factored out?
Member
|
Could you please improve the PR title? |
turbolent
reviewed
Aug 6, 2026
| } | ||
|
|
||
| // returns a publicKeyECDSAP256 from (bytes(x) || bytes(y)) bytes | ||
| func publicKeyECDSAP256(a *ecdsaContext, XYBytes []byte) (*pubKeyECDSAP256, error) { |
Member
There was a problem hiding this comment.
a *ecdsaContext is unused. Maybe remove it or use it instead of p256Instance below
| // normalize the signature to low S. | ||
| // This is required because the secp256k1 package does not accept high S signatures while the package allows them. | ||
| // Rejecting high S signatures would be a breaking change with prior versions. | ||
| newSig, validS := secp256k1Instance.signatureNormalizeS(sig) |
Member
There was a problem hiding this comment.
Should this maybe use pk.ecdsaContext?
| curveP *big.Int | ||
| // curve order | ||
| curveN *big.Int | ||
| // curve order minus 1 divided by 2 (used for signature malleability annalysis) |
Member
There was a problem hiding this comment.
Suggested change
| // curve order minus 1 divided by 2 (used for signature malleability annalysis) | |
| // curve order minus 1 divided by 2 (used for signature malleability analysis) |
| // (same slice is returned if S is already normalized) | ||
| // It assumes len(sig) == 2*nLen where nLen is the byte-length of the curve order. | ||
| // This is needed when the underlying signature verification requires S to be in the lower range (to avoid signature malleability). In this package, verification allows high S signatures to be accepted. | ||
| // The function checks that S is in the correct range [0, n-1] before normalizing it. If S is not in the correct range, the function returns a false boolean. (S will be checked against 0 in the verification function - check against N is inlcuded here) |
Member
There was a problem hiding this comment.
Suggested change
| // The function checks that S is in the correct range [0, n-1] before normalizing it. If S is not in the correct range, the function returns a false boolean. (S will be checked against 0 in the verification function - check against N is inlcuded here) | |
| // The function checks that S is in the correct range [0, n-1] before normalizing it. | |
| // If S is not in the correct range, the function returns a false boolean. | |
| // (S will be checked against 0 in the verification function - check against N is included here) |
| var _ PublicKey = (*pubKeyECDSASecp256k1)(nil) | ||
|
|
||
| func privateKeyECDSASecp256k1(a *ecdsaContext, dBytes []byte) *prKeyECDSASecp256k1 { | ||
| sk := &prKeyECDSASecp256k1{ |
Member
There was a problem hiding this comment.
Maybe validate, like privateKeyECDSAP256 does
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
closes #40
CGO_ENABLED=1, in the past version SECp256k1 did not use cgoThis PR also removes the non-cgo build mode, which is used to build the library without cgo (
CGO_ENABLED=0) while disabling BLS. Now that ECDSA secp256k1 also uses cgo, the non-cgo mode has very limited utility and is discontinued.