Skip to content

Replace deprecated elliptic key encoding#4167

Open
mswilkison wants to merge 3 commits into
mainfrom
fix/secp256k1-deprecated-elliptic
Open

Replace deprecated elliptic key encoding#4167
mswilkison wants to merge 3 commits into
mainfrom
fix/secp256k1-deprecated-elliptic

Conversation

@mswilkison

@mswilkison mswilkison commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a shared secp256k1 public-key codec backed by the existing btcec dependency
  • replace all deprecated elliptic.Marshal and elliptic.Unmarshal calls while preserving the 65-byte uncompressed SEC 1 encoding
  • reject compressed, hybrid, wrong-prefix, and off-curve encodings and propagate malformed persisted wallet-key errors

The affected keys use secp256k1, which is not supported by crypto/ecdh, so this uses the repository's existing btcec implementation.

Testing

  • go test ./pkg/crypto/secp256k1 ./pkg/bitcoin ./pkg/chain/ethereum ./pkg/protocol/inactivity ./pkg/tbtc ./pkg/tbtc/internal/test -count=1
  • go test ./pkg/tecdsa/dkg -run 'Test(TssFinalize|TssRoundTwo_IncomingMessageMissing|TssRoundTwo_SymmetricKeyMissing)$' -count=1
  • Staticcheck 2025.1.1 with -checks=SA1019 ./...: 0 deprecated elliptic findings; 58 pre-existing generated-protobuf findings remain

A full local pkg/tecdsa/dkg run hit the two timing-sensitive round-one deadline tests listed above under concurrent load; both passed when rerun in isolation with the changed finalization path.

Fixes #3832

Summary by CodeRabbit

  • New Features

    • Added standardized secp256k1 public-key encoding and decoding.
    • Added validation for malformed and invalid public keys.
  • Bug Fixes

    • Improved error handling when wallet public keys cannot be decoded.
    • Ensured consistent public-key serialization across Bitcoin, TBTC, and signing workflows.
  • Tests

    • Added coverage for valid round trips, malformed keys, curve validation, and invalid coordinates.

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 72eb8e79-f4d8-4690-87ab-4776614cf011

📥 Commits

Reviewing files that changed from the base of the PR and between 038b7ce and 1321725.

📒 Files selected for processing (16)
  • pkg/bitcoin/script_test.go
  • pkg/bitcoin/transaction_test.go
  • pkg/chain/ethereum/tbtc.go
  • pkg/crypto/secp256k1/public_key.go
  • pkg/crypto/secp256k1/public_key_test.go
  • pkg/protocol/inactivity/member_test.go
  • pkg/tbtc/chain_test.go
  • pkg/tbtc/coordination_test.go
  • pkg/tbtc/heartbeat_test.go
  • pkg/tbtc/internal/test/marshaling.go
  • pkg/tbtc/marshaling.go
  • pkg/tbtc/marshaling_test.go
  • pkg/tbtc/registry.go
  • pkg/tbtc/wallet.go
  • pkg/tecdsa/dkg/protocol_test.go
  • pkg/tecdsa/dkg/result.go

📝 Walkthrough

Walkthrough

The change adds validated secp256k1 public-key marshal/unmarshal helpers and replaces deprecated elliptic serialization across TBTC, Bitcoin, Ethereum, DKG, and related test code. Invalid key decoding now propagates explicit errors in signer and scenario unmarshalling.

Changes

secp256k1 serialization migration

Layer / File(s) Summary
Validated secp256k1 helpers
pkg/crypto/secp256k1/*
Adds validated uncompressed public-key marshaling and unmarshaling with roundtrip, malformed-input, relabeled-curve, and invalid-key tests.
Production serialization integration
pkg/chain/ethereum/tbtc.go, pkg/tbtc/{marshaling.go,registry.go,wallet.go}, pkg/tecdsa/dkg/result.go
Replaces elliptic serialization in hash construction, wallet storage keys, wallet formatting, signer serialization, and DKG public-key bytes.
TBTC parsing and error handling
pkg/tbtc/internal/test/marshaling.go, pkg/tbtc/marshaling_test.go
Uses validated decoding for scenario and signer wallet keys and tests malformed-key error reporting.
Test key migration
pkg/bitcoin/*_test.go, pkg/protocol/inactivity/member_test.go, pkg/tbtc/*_test.go, pkg/tecdsa/dkg/protocol_test.go
Updates test helpers and fixtures to use shared secp256k1 encoding and decoding functions.

Estimated code review effort: 4 (Complex) | ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 13.04% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: replacing deprecated elliptic key encoding.
Linked Issues check ✅ Passed The PR addresses #3832 by replacing the listed elliptic.Marshal/Unmarshal usages with secp256k1-based helpers across all affected files.
Out of Scope Changes check ✅ Passed The new codec and tests are directly supporting the key-encoding migration and no unrelated changes are indicated.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/secp256k1-deprecated-elliptic

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mswilkison

Copy link
Copy Markdown
Contributor Author

CI note: I reran the non-required client-integration-test after its initial failure. The rerun failed for the same shared external-infrastructure reason as PR #4165: all 30 failures are fulcrum_tcp cases receiving dial tcp 193.30.123.70:50001: connect: connection refused; every other package passed. This endpoint is unrelated to the secp256k1 codec change, and the matching result on an unrelated PR confirms this is not a regression here. Rerun job: https://github.com/threshold-network/keep-core/actions/runs/29534290770/job/87752372472

The Marshal guard compared curve names, which assumes every secp256k1
implementation labels itself the way btcec does. Compare the field
prime and group order instead so keys held with any secp256k1
implementation marshal correctly, and cover the relabeled-curve case
with a test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AxCWMXfUdFygLaGaS2SPhg
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.

Lint: Fix deprecated elliptic.Marshal/Unmarshal usage (SA1019 warnings)

1 participant