feat: remove btcd dependency #1201
Conversation
WalkthroughThe codebase transitions its secp256k1 cryptographic implementation from Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Fuzz Test
participant Curve1 as go-ethereum secp256k1
participant Curve2 as Decred secp256k1
Test->>Curve1: Derive points P1, P2 via base mult (dataP1, dataP2)
Test->>Curve2: Derive points Q1, Q2 via base mult (dataP1, dataP2)
Test->>Curve1: Add P1 + P2 -> (x1, y1)
Test->>Curve2: Add Q1 + Q2 -> (x2, y2)
Test->>Test: Compare (x1, y1) with (x2, y2)
alt Not equal
Test->>Test: Panic and print error
end
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
go.mod (1)
13-13:⚠️ Potential issueThe btcd dependency was not removed.
While the Decred secp256k1 dependency was added at line 20, the
github.com/btcsuite/btcd v0.20.1-betadependency at line 13 is still present. This contradicts the PR objective of removing the btcd dependency.Apply this diff to complete the btcd removal:
- github.com/btcsuite/btcd v0.20.1-betaAlso applies to: 20-20
🧹 Nitpick comments (1)
tests/fuzzers/secp256k1/secp_test.go (1)
51-51: Update error message to reflect the actual library being used.The panic message incorrectly references "btcd" when the code is using the Decred secp256k1 implementation.
- panic(fmt.Sprintf("Addition failed: geth: %s %s btcd: %s %s", resAX, resAY, resBX, resBY)) + panic(fmt.Sprintf("Addition failed: geth: %s %s dcred: %s %s", resAX, resAY, resBX, resBY))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
crypto/signature_nocgo.go(3 hunks)go.mod(1 hunks)tests/fuzzers/secp256k1/secp_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: check
tests/fuzzers/secp256k1/secp_test.go
[failure] 38-38:
fuzz already declared through import of package fuzz ("github.com/google/gofuzz")
[failure] 32-32:
Fuzz redeclared in this block
🪛 GitHub Actions: CI
tests/fuzzers/secp256k1/secp_test.go
[error] 32-32: Fuzz redeclared in this block
🪛 Gitleaks (8.26.0)
crypto/signature_nocgo.go
85-85: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
crypto/signature_nocgo.go (5)
17-18: Good addition of platform support.The expanded build tags to include
wasip1andtinygoimprove platform compatibility.
78-97: Secure implementation with proper key handling.The signature generation correctly validates the curve, safely converts the private key with overflow checks, and properly zeros the key material after use.
🧰 Tools
🪛 Gitleaks (8.26.0)
85-85: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
118-122: Important security enhancement for signature malleability protection.Excellent addition of the malleability check using
IsOverHalfOrder(). This ensures compatibility with libsecp256k1's behavior and prevents signature malleability attacks.
158-193:✅ Verification successful
Verify the btCurve wrapper maintains compatibility with the original implementation.
The custom
btCurvewrapper correctly implements marshaling/unmarshaling for uncompressed points. However, we should verify that this matches the exact behavior of the original btcec implementation to ensure backward compatibility.Run this script to verify the wrapper implementation:
🏁 Script executed:
#!/bin/bash # Description: Search for any existing usage of Marshal/Unmarshal methods to ensure compatibility # Check for existing Marshal/Unmarshal usage patterns rg -A 5 "\.Marshal\(" --type go rg -A 5 "\.Unmarshal\(" --type go # Look for any elliptic curve point serialization patterns ast-grep --pattern 'elliptic.Marshal($$$)' ast-grep --pattern 'elliptic.Unmarshal($$$)'Length of output: 69777
I want to inspect the cgo‐backed implementation of
Marshal/Unmarshalto ensure they produce the same uncompressed format. Let’s pull in the*_cgo.gofiles undercrypto/and look for the secp256k1 serialize/parse calls:
🏁 Script executed:
#!/bin/bash # Locate any Go files under crypto/ that mention cgo in their name CGO_FILES=$(find crypto -type f -name "*cgo*.go") echo "Found cgo files:" echo "$CGO_FILES" # For each, dump the top and search for serialize/parse for f in $CGO_FILES; do echo echo "=== $f ===" sed -n '1,200p' "$f" echo "----- looking for serialization functions -----" grep -n "serialize" "$f" || true grep -n "parse" "$f" || true doneLength of output: 21117
Compatibility Verified: btCurve Marshal/Unmarshal Matches Default Behavior
The custom btCurve.Marshal and Unmarshal implementations are identical to Go’s crypto/elliptic.Marshal and Unmarshal for uncompressed points:
- Both use a 0x04 prefix and fixed 1+2*byteLen length.
- Both pad big‐endian coordinates to
byteLenbytes.- Neither performs additional curve‐point validity checks (matching the stdlib).
No changes required.
159-161:✅ Verification successful
Verify API compatibility of the new EllipticCurve return type.
The
S256()function now returns a custombtCurvetype instead of the original curve type. While this implements the same interface, we should verify that all callers are compatible with this change.
🏁 Script executed:
#!/bin/bash # Description: Check usage of S256() function and verify type compatibility # Find all calls to S256() rg "S256\(\)" --type go -B 2 -A 2 # Look for any type assertions or comparisons with S256() ast-grep --pattern 'S256() == $$$' ast-grep --pattern '$$$.(type) { $$$ case $$$S256()$$$: $$$ }' # Check for any direct field access on the curve rg "S256\(\)\." --type goLength of output: 11050
I’d like to confirm how
EllipticCurveandbtCurveare defined to ensureMarshal(and any other methods) remain available on the new wrapper.
🏁 Script executed:
#!/bin/bash # Find EllipticCurve interface definition rg "type EllipticCurve" -A5 -B5 # Find btCurve type definition rg "type btCurve" -A5 -B5Length of output: 630
To ensure our
btCurvewrapper fully satisfies the required interface, let’s locate theEllipticCurvedefinition and its methods:
🏁 Script executed:
#!/bin/bash # Find where EllipticCurve is defined rg "type EllipticCurve interface" -A5 -B5 # List all references to EllipticCurve to catch any alias or import rg "EllipticCurve" -nLength of output: 522
API compatibility of the
S256()wrapper confirmedI’ve verified that:
- No callers perform type assertions or comparisons against the concrete curve type.
- The only direct method invocation (
S256().Marshal(…)) still works, sincebtCurveimplements both the embeddedsecp256k1.KoblitzCurvemethods and any additionalEllipticCurvemethods (e.g.Marshal/Unmarshal).- The
prv.Curve != S256()check continues to succeed becausesecp256k1.S256()always returns the same pointer, so two calls tobtCurve{secp256k1.S256()}compare equal.There are no remaining compatibility concerns—no fixes required.
| func Fuzz(f *testing.F) { | ||
| f.Fuzz(func(t *testing.T, a, b []byte) { | ||
| fuzz(a, b) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Resolve naming conflicts with existing fuzz package.
The function names Fuzz and fuzz conflict with identifiers from the github.com/google/gofuzz package that appears to be imported elsewhere in this package. This is causing compilation failures.
Rename the functions to avoid conflicts:
-func Fuzz(f *testing.F) {
+func FuzzSecp256k1(f *testing.F) {
f.Fuzz(func(t *testing.T, a, b []byte) {
- fuzz(a, b)
+ fuzzPointAddition(a, b)
})
}
-func fuzz(dataP1, dataP2 []byte) {
+func fuzzPointAddition(dataP1, dataP2 []byte) {Update the TestFuzzer call accordingly:
func TestFuzzer(t *testing.T) {
a, b := "00000000N0000000/R0000000000000000", "0U0000S0000000mkhP000000000000000U"
- fuzz([]byte(a), []byte(b))
+ fuzzPointAddition([]byte(a), []byte(b))
}Also applies to: 38-53
🧰 Tools
🪛 GitHub Check: check
[failure] 32-32:
Fuzz redeclared in this block
🪛 GitHub Actions: CI
[error] 32-32: Fuzz redeclared in this block
🤖 Prompt for AI Agents
In tests/fuzzers/secp256k1/secp_test.go around lines 32 to 53, the function
names Fuzz and fuzz conflict with identifiers from the imported
github.com/google/gofuzz package, causing compilation errors. Rename the outer
Fuzz function to a unique name like RunFuzz or SecpFuzz, and rename the inner
fuzz function to a distinct name such as runFuzz or fuzzTest. Update all calls
accordingly to use the new function names to avoid naming conflicts.
|
@yiweichi is this PR still active? |
Yes, we will need this PR for Arweave sender, cause the btcd conflicts with an Arweave dependency. |
1. Purpose or design rationale of this PR
...
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit