fix(key-wallet-manager): enable key-wallet/getrandom for mnemonic generation - #945
fix(key-wallet-manager): enable key-wallet/getrandom for mnemonic generation#945QuantumExplorer wants to merge 3 commits into
Conversation
…eration key-wallet-manager declared key-wallet with `default-features = false`, which drops `getrandom`. Without that feature `Mnemonic::generate` compiles to its `#[cfg(not(feature = "getrandom"))]` stub, which always returns an error — so `create_wallet_with_random_mnemonic` could never succeed for an external consumer of the crate. The breakage was invisible in CI: workspace builds and the crate's own dev-dependency on key-wallet (default features on) both unify `getrandom` back on, so the test suite passed while a standalone consumer got a dead API. Verified against a crate outside the workspace — it printed "Mnemonic generation requires getrandom feature" before this change and generates a 24-word phrase after. key-wallet-ffi depends on key-wallet with default features, so iOS/Swift was never affected. This was fail-closed — it returned an error, never weak entropy. Also add `generate_draws_full_width_entropy`, a regression guard for the weak-seed-phrase bug class (Milk Sad / CVE-2023-39910, Trust Wallet / CVE-2023-31290). For all five word counts it asserts the recovered entropy is the full BIP-39 width, round-trips to the same phrase, never collides across draws, and that every bit position takes both values — a stuck bit being what a narrowed entropy source leaves behind. The test covers the truncation half of that bug class only. A full-width buffer filled from a seeded PRNG is not statistically detectable at any practical sample size; that half is held by construction, and the test doc comment names the invariant so review can protect it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change enables the ChangesMnemonic entropy support
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@key-wallet/src/mnemonic.rs`:
- Around line 406-421: Rename the test and its documentation around the affected
mnemonic test cases to describe only zero-padding or stuck-bit regression
coverage, removing claims that it verifies full-width entropy or detects
deterministic expansion. Preserve the assertions for round-tripping, collisions,
and varying bit positions, and keep the OS-CSPRNG guarantee stated separately as
a construction requirement of Mnemonic::generate using direct getrandom.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 75484a21-68a8-4132-a125-ca8c25981bbd
📒 Files selected for processing (2)
key-wallet-manager/Cargo.tomlkey-wallet/src/mnemonic.rs
… test The test doc comment said it guarded against a full-width entropy buffer whose bits are "zero-padding or a deterministic expansion". The deterministic-expansion half is wrong: a generator that expands a 32- or 48-bit seed through a hash or PRNG produces freely varying bits and passes every assertion in the test. That claim also contradicted the comment's own closing paragraph, which already conceded exactly that limitation. Rename `generate_draws_full_width_entropy` -> `generate_entropy_has_expected_width_and_no_stuck_bits` and rewrite the doc comment to state only what the assertions establish: correct BIP-39 width, entropy round-trips to the same phrase, no collisions, no stuck bits. The seeded-PRNG case is now called out explicitly as undetectable at this sample size and held by construction instead, so a reader knows it is `Mnemonic::generate` calling `getrandom` directly that review has to protect. Also reword the stuck-bit assertion message, which drew the same overly broad conclusion about the entropy source. No behavior change; assertions are untouched. Reported by CodeRabbit on #945. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #945 +/- ##
==========================================
+ Coverage 74.76% 75.19% +0.42%
==========================================
Files 328 328
Lines 76593 78218 +1625
==========================================
+ Hits 57267 58816 +1549
- Misses 19326 19402 +76
|
…ssage
The message passed `u8::from(count > 0)` as a trailing argument to
recover which value the bit was stuck at. Rust evaluates assert! message
arguments only on the panic path, so that line could never execute while
the test passes — it was the sole uncovered line in the patch (llvm-cov
line 470, flagged by Codecov at 96% patch coverage).
Fold the value into the format string as a captured `{count}` instead.
The count carries the same information more directly (0 ones = stuck at
0, SAMPLES ones = stuck at 1) while showing the actual distribution, and
drops a needlessly clever conversion. Patch coverage goes to 100%;
verified with `cargo llvm-cov --lib -p key-wallet`, which shows line 470
as the only uncovered line before and none in the test region after.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Why
Prompted by an audit of seed-phrase entropy across the repo, checking for the bug class behind Milk Sad (CVE-2023-39910, Libbitcoin
bx seed—mt19937seeded with 32-bittime(NULL)) and Trust Wallet's browser extension (CVE-2023-31290, 32-bit seed).The audit came back clean.
Mnemonic::generateis the single chokepoint every random wallet in the repo routes through, and it is correct on both halves of that bug class:getrandom::getrandomgoes straight to the OS CSPRNG (getentropyon Apple,getrandom(2)on Linux,BCryptGenRandomon Windows). No seeded PRNG anywhere in the path.getrandomv0.2.17 resolves with only itsdefaultfeature — nocustom,js, orrdrandbackend, and nothing callsregister_custom_getrandom!.Supporting checks:
Seed::random()fills all 64 bytes fromgetrandom;Wallet::new_random,WalletManager::create_wallet_with_random_mnemonic, and the FFImnemonic_generate*entry points all delegate toMnemonic::generate; no time-based seeding outside benchmarks; the onlyStdRng::seed_from_u64calls are test helpers. BIP38's 4-byteowner_saltis spec-mandated and drawn from a CSPRNG, not a seed.Verified empirically as well as by reading — 20,000 generations per word count through the public API gave zero collisions, all 256 values at every byte position, and χ² on bit balance landing at the degrees of freedom (e.g. 120.2 on 128 dof for 12-word).
What changed
1.
key-wallet-managercould never generate a mnemonic (the one real defect)key-wallet-managerdeclaredkey-walletwithdefault-features = false, which dropsgetrandom. Without it,Mnemonic::generatecompiles to its#[cfg(not(feature = "getrandom"))]stub — one that always returns an error — socreate_wallet_with_random_mnemoniccould never succeed.It was invisible in CI because workspace builds and the crate's own dev-dependency on
key-wallet(default features on) both unifygetrandomback on: the full test suite passed while an external consumer got a dead API. Confirmed with a crate built outside the workspace, which printedMnemonic generation requires getrandom featurebefore the change andOK: generated 24 wordsafter.This was fail-closed — an error, never weak entropy. Not a Milk Sad-class vulnerability.
key-wallet-ffidepends onkey-walletwith default features, so iOS/Swift was never affected.2. Regression guard:
generate_entropy_has_expected_width_and_no_stuck_bitsFor all five word counts, asserts the recovered entropy is the full BIP-39 width, round-trips to the same phrase, never collides across draws, and that every bit position takes both values. 128 samples makes a false positive a ~2^-63 event; runs in well under a second.
This catches zero-padded and stuck-bit entropy only. It does not establish how much real entropy those bits carry — see below.
For reviewers
generatefills its buffer directly fromgetrandom) and only review can protect it; the test's doc comment says so explicitly. Thanks to CodeRabbit for catching that the original name and doc comment overclaimed this.Mnemonic::generatecurrently exists as a runtime-erroring stub whengetrandomis off. Making it compile-time-absent would turn this whole class of misconfiguration into a build failure instead of a runtime surprise, but that's a breaking API change forno_stdconsumers who don't need generation.Testing
cargo test -p key-wallet --lib mnemonic— 47 passedcargo check -p key-wallet-manager -p key-wallet-ffi— cleancargo clippy -p key-wallet --lib --all-featuresandcargo fmt --check -p key-wallet— clean🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests