fix: bump secp256k1 off yanked prerelease - #274
Conversation
|
Warning Review limit reachedNext included review available in 9 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
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. Comment |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana Review — PR #274 (fix: bump secp256k1 off yanked prerelease)
TL;DR
The MuSig2 nonce-generation change is correct: the right replacement was chosen and the security invariant is preserved. The deprecation renames are mechanical and semantically equivalent. Two items need follow-up before or just after merge; neither is a blocker given CI has been red for two weeks due to the yanked releases.
Protocol / Security: MuSig2 Nonce Generation
ark-core/src/batch.rs lines 192–210 (post-diff)
The replacement is correct:
| Old (0.32-beta.2) | New (0.33) | Verdict |
|---|---|---|
SessionSecretRand::assume_unique_per_nonce_gen(rng.gen()) |
SessionSecretRand::assume_uniformly_random(rng.gen()) |
Semantic equivalent — same constant-time zero check, same pass-through |
KeyAggCache::nonce_gen(…) |
KeyAggCache::nonce_gen_with_uniform_randomness(…) |
Same internal new_nonce_pair call, NULL seckey, both paths |
The 0.33 assume_unique_per_nonce_gen is intentionally a different function now (requires a secret-key XOR mixing step). Using it here would have been wrong. The PR correctly avoided that trap.
extra_rand moving from Option<[u8; 32]> to mandatory [u8; 32] is strictly better: entropy is always mixed in rather than sometimes skipped. The security property never depended on it (the session secret was already uniform), but "always some" is preferable to "half the time None".
The CryptoRng bound at generate_nonce_tree<R: Rng + CryptoRng> ensures the session secret satisfies the assume_uniformly_random precondition at every call site. The SecretNonce single-use wrapper (via Option::take) still enforces no-double-sign at the type level. ✓
Deprecation Renames
ark-core/src/batch.rs line 300 (post-diff)
Keypair::from_seckey_byte_array → from_secret_bytes: straightforward rename, identical behaviour.
ark-core/src/conversions.rs line 9 / ark-client/src/boltz.rs line 5223 (post-diff)
XOnlyPublicKey::serialize() → to_byte_array(): same 32-byte output, no semantic change. to_musig_pk in conversions.rs retains pk.serialize() from the bitcoin crate's PublicKey, which is unaffected — correct.
Issues
1. Public API carries musig types — semver bump needed (non-blocking given yanked state)
ark-core/src/batch.rs line 145 (post-diff):
pub struct NonceKps(HashMap<Txid, (Option<musig::SecretNonce>, musig::PublicNonce)>);pub fn aggregate_nonces(tree_tx_nonce_pks: TreeTxNoncePks) -> musig::AggregatedNonce { … }pub fn take_sk(&mut self, txid: &Txid) -> Option<musig::SecretNonce> { … }These three public items expose musig:: (i.e. secp256k1) types directly. Bumping musig from 0.32 to 0.33 changes those type identities, making this a semver-breaking change for any downstream crate that uses NonceKps or aggregate_nonces. The PR body acknowledges this. Because every 0.32-beta.* is yanked and nothing can build against the old shape anyway, the practical impact is zero today — but ark-core's version should be bumped (minor or patch as appropriate) before crates.io publication to avoid silent compatibility claims. Recommend a follow-up PR for the version bump.
2. sign_batch_tree_tx has no test coverage
ark-core/src/batch.rs line 300 (post-diff): the from_secret_bytes call site is in sign_batch_tree_tx, which has zero test coverage. The two new tests exercise generate_nonce_tree only. A test that drives generate_nonce_tree → sign_batch_tree_tx through a partial-sign round-trip would both cover the rename and guard the signing path. Not a merge blocker here given the emergency nature of the fix, but should be filed as a follow-up.
3. Wire compatibility is inference, not verified
The PR correctly notes that 66-byte AGGNONCE/PUBNONCE and 32-byte PART_SIG formats are unchanged, but this was not validated against a live arkd instance. Since E2E is Docker-gated this is a known limit. Worth a runtime handshake test once Docker is available.
4. secp256k1-sys 0.12.0 → 0.14.1
Two major version jumps in the sys crate means the vendored libsecp256k1 C code changed substantially. Not audited here. This is a widely-deployed upstream library, so the risk is low, but noting the gap.
Tests
The two new tests at ark-core/src/batch.rs lines 1200–1237 (post-diff):
nonce_tree_is_determined_by_the_rng: confirms nonces are a pure function of the RNG seed — guards against counter-based or clock-based nonce generation. ✓nonce_tree_varies_with_the_rng: confirms different seeds produce different nonces — catches the fully-deterministic counter mis-port. ✓
The PR description's stated limitation is accurate: the tests do not catch a counter mis-port that still threads RNG-derived extra_rand. That is an acceptable gap for this fix.
Test helper note: cosigner_kp(1) uses [1u8; 32] as a secret key, which is valid (non-zero, within the curve order). ✓
Cross-repo Impact
ark-core's public API does not re-export musig types at the crate root (ark-core/src/lib.rs), but the types leak through batch.rs public items as noted above. No other repos in the monorepo depend on ark-core as a Rust dependency directly (only rust-sdk workspace and ark-rs-flutter-example submodule). TS/Go/dotnet SDKs are unaffected.
Summary
| Area | Status |
|---|---|
| MuSig2 nonce replacement correctness | ✓ Correct |
| Deprecation renames | ✓ Correct |
| Test coverage (nonce path) | ✓ Added |
| Test coverage (sign path) | |
| Semver bump for ark-core | |
| Wire compatibility verified | |
| secp256k1-sys C code audit | ℹ️ Not done |
The published ark-client 0.10.1 depends on the yanked secp256k1 0.32.0-beta.2, which breaks fresh resolution in CI for the dotnet native crate. Upstream fixed it in arkade-os/rust-sdk#274 but has not released yet, so both the main workspace and client-sdk/rust-sdk take the same git revision. This drops the unmerged VTXO cache commit (arkade-os/rust-sdk#264) the main workspace was pinned to. Cargo.nix still needs regenerating, see #1098.
3 participants
CI is red on every PR right now, and has been since the previous
ci.ymlrun on 2026-08-18. All threesecp256k1 0.32.0-beta.*releases were yanked from crates.io when 0.33 shipped, andCargo.lockis gitignored (.gitignore:8), so every job re-resolves and dies before compiling anything:This bumps
secp256k1(aliasedmusig) to0.33inark-coreandark-clientand ports the call sites.A committed lockfile would also have made CI green — cargo honours a yanked version already present in a lockfile — but lockfiles are ignored by dependents, so anyone building
ark-core/ark-clientfrom crates.io would still be broken. Hence the bump.The nonce change, which is the part worth reviewing
0.33 reworked the MuSig2 nonce API, and one of the two available replacements is a trap:
KeyAggCache::nonce_genis now counter-based (nonrepeating_cnt: u64). Using it here would be wrong — there is no non-repeating counter that survives a process restart, and a repeated MuSig2 nonce leaks the secret key.KeyAggCache::nonce_gen_with_uniform_randomnesspreserves the existing randomness-derived behaviour, and is what this PR uses.The two renames line up exactly with the old API:
SessionSecretRand::assume_uniformly_random(0.33.1musig.rs:143) is byte-for-byte identical to the oldassume_unique_per_nonce_gen(0.32-betamusig.rs:81) — same constant-time zero check, same pass-through. It is the straight rename.assume_unique_per_nonce_genis a different function now: it mixesSHA256_tagged("MuSig/aux", inner) XOR skand needs a secret key, whichgenerate_nonce_treedoes not have.nonce_genand newnonce_gen_with_uniform_randomnessboth delegate to the samenew_nonce_pair(session_secrand, Some(cache), None /* NULL seckey */, pub_key, Some(msg), …).One behavioural delta, forced by the signature:
extra_randwent fromOption<[u8; 32]>— which rand 0.8 filled withNoneroughly half the time — to a mandatory[u8; 32]. Strictly more entropy; the security property never rested on it, sincesession_secrandwas already uniform and secret.Also fixes three deprecations the bump surfaces (
Keypair::from_seckey_byte_array→from_secret_bytes, and twoXOnlyPublicKey::serialize→to_byte_array), which-D warningsrequires.Test plan
generate_nonce_treehad no test coverage. Added two tests inark-core/src/batch.rs. Both were first checked against the pre-bump 0.32-beta build, so they characterise existing behaviour rather than the new code.nonce_tree_varies_with_the_rngfails against a deliberately injected counter-based mis-port: two different RNG seeds produced a byte-identical nonce, i.e. literal nonce reuse, caught.extra_rand. They guard the fully-deterministic case, not every counter shape.cargo test --workspace --exclude e2e-tests: 277 passed, 0 failed (baseline 275/0 — delta is exactly the two new tests).cargo clippy --all-targets --all-features -- -D warnings: clean, includinge2e-tests, so the Docker-gated call site type-checks too.rm Cargo.lock && cargo metadata: exit 0, no yanked versions, resolves 0.33.1. That command was exit 101 before this change.Not verified
secp256k1-sysmoves 0.12.0 → 0.14.1. The Rust wrapper was diffed; the vendored libsecp256k1 C code was not audited. No live arkd interop either — E2E is Docker-gated. Wire sizes are unchanged (AGGNONCE 66, PUBNONCE 66, PART_SIG 32), so the bytes on the wire keep their shape, but that is inference rather than a runtime handshake.Worth a maintainer call:
ark-core's public API exposes secp256k1 types, so this is technically breaking for consumers at 0.10.1. Mitigating factor is that every 0.32-beta is yanked, so nothing can build against the old shape anyway.