Skip to content

wallet import: surface derivation mismatches when recovering a phrase - #184

Merged
illuzen merged 4 commits into
mainfrom
fix/wallet-import-recovery-scan
Oct 19, 2026
Merged

illuzen merged 4 commits into
mainfrom
fix/wallet-import-recovery-scan

Conversation

@illuzen

@illuzen illuzen commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

Fixes the "entered my phrase, got a wrong blank wallet" report.

The import defaults changed from ML-DSA-87 at …/0' (v1.x) to ML-DSA-65 at …/1' (v2.0+), so a phrase from an older wallet imports "successfully" onto a brand-new empty address with no indication anything is off. A user recovering funds cannot tell this apart from a bad phrase.

Changes

  • wallet import --scan: derives every historical default (ML-DSA-65 …/1', ML-DSA-87 …/0', master seed under each scheme), shows address and checkphrase for each, marks on-chain history when a node answers, and lets the user pick. Prints the explicit --scheme/--derivation-path flags to reproduce the choice. q, EOF, or bad input abort without touching the keystore; non-TTY stdin fails fast instead of blocking. Conflicts with --scheme/--derivation-path/--no-derivation at parse time.
  • Post-import history check: after a plain import, warn when the derived address has never been used on-chain and point at --scan / airdrop check.
  • Node never required: the probe is bounded by a 5s timeout; a missing node only drops the on-chain info. --offline skips the probe entirely for air-gapped machines.
  • Mnemonic prompt reworded to "recovery phrase (… not the wallet password)"; v1.x defaults documented in --scheme help and a new README section.

Testing

  • 98 wallet tests pass (5 new, including pinning the ML-DSA-87 candidate to the known qzm5QCox… vector and asserting all four candidates are distinct).
  • cargo clippy --workspace --all-targets --all-features --locked -- -D warnings clean; formatted with CI's pinned nightly-2026-08-31.
  • Picker exercised end-to-end through a pty: choosing 2 imports the pre-v2.0 ML-DSA-87 address; EOF and out-of-range input abort.

The CLI's import defaults changed from ML-DSA-87 at …/0' (v1.x) to
ML-DSA-65 at …/1' (v2.0+), so a phrase from an older wallet imports
"successfully" onto a brand-new empty address with no indication that
anything is off. A user recovering funds cannot distinguish this from a
bad phrase.

- `wallet import --scan` derives every historical default (both schemes,
  both HD paths, both master-seed variants), shows the address and
  checkphrase for each, marks on-chain history when a node answers, and
  lets the user pick. It prints the explicit flags to reproduce the
  choice. EOF or `q` aborts without touching the keystore.
- After a plain import, check the derived address on-chain and warn when
  it has never been used, pointing at --scan and `airdrop check`.
- The node is never required: the probe is bounded by a 5s timeout and a
  missing node only drops the on-chain column. `--offline` skips the
  probe entirely for air-gapped machines.
- Reword the mnemonic prompt to "recovery phrase" so it is not confused
  with the wallet password, and document the v1.x defaults in --scheme
  help and the README.
@illuzen illuzen added the bot-review Request automated review from review-bot label Oct 17, 2026

@n13 n13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer model: GPT-6.1 Sol

REQUEST_CHANGES — two recovery paths can still block unexpectedly. Reviewed head 92d3e5ffcdae805c518471783d6651374f23ac34 against base 413a6e23f7fc9ebbe8145d9cd887cd52a70cc933.

  1. [P2] Bound the complete optional node probe, including RPC reads. In src/cli/wallet.rs:547–565, only QuantusClient::new is inside the five-second timeout. chain_token opens another connection, and the four account lookups run sequentially outside that deadline. The same issue affects the plain-import check at lines 494–504. With a mock node that immediately completed initialization but withheld state_getStorage, plain import took 31.5 seconds; separately withholding token properties or storage left --scan without any addresses or selection prompt after eight seconds. Account requests retain the client's 30-second timeout, so multiple failures accumulate. Apply one overall deadline to all best-effort network work in both paths and show the locally derived candidates when it expires. Add coverage for a node that connects successfully and then stops answering.

  2. [P2] Reject non-TTY scans before collecting passwords or mnemonics. The terminal guard at src/cli/wallet.rs:537–543 is reached only after the import handler has run the password and mnemonic prompts. rpassword reads the controlling terminal, so wallet import --scan --offline --mnemonic-file <protected-file> </dev/null still waits for a password when no password source is supplied. I reproduced this with redirected stdin and a controlling PTY; it remained at the password prompt until killed. Move the scan terminal check to the start of import handling, before secret-file reads and prompting, and test without --allow-empty-password or an environment password. The current non-TTY test bypasses the earlier prompts.

Validation:

  • SKIP_CIRCUIT_BUILD=1 cargo test --locked --no-default-features --lib wallet: 108 passed.
  • SKIP_CIRCUIT_BUILD=1 cargo clippy --locked --all-targets --all-features -- -D warnings: passed.
  • CI-pinned cargo +nightly-2026-08-31 fmt --all -- --check and git diff --check: passed; headless CLI build passed.
  • PTY checks confirmed legacy candidate 2 imports the pinned address, and cancellation/invalid selection create no wallet.
  • GitHub build/test, analysis, and example jobs were still running at the last check. The security-audit job failed against the unchanged dependency lockfile; this is separate from the two PR findings above.

@n13 n13 removed the bot-review Request automated review from review-bot label Oct 17, 2026
Connect, account reads and token lookup now share one 10s deadline;
partial results are kept and --scan always shows every candidate.
--scan refuses non-interactive stdin before reading the password or
recovery phrase. Mock node can stall state_getStorage for tests.

Co-authored-by: Cursor <cursoragent@cursor.com>
@illuzen

illuzen commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both findings in bb53b2c.

Node probe wall-clock bound. Connect, the System::Account reads and the token lookup now run inside a single probe_on_chain with one 10s tokio::time::timeout. Partial results are kept: if the node connects but stalls, --scan still lists every candidate (marked unknown (no answer)) and the plain import prints a "node stopped answering" note instead of blocking. Against a mock node that accepts the connection and never answers state_getStorage, plain wallet import now finishes in ~11s (was ~31.5s). Tests: probe_on_chain_is_bounded_when_node_connects_then_stalls, wallet_import_completes_promptly_when_node_connects_then_stalls, plus healthy and unreachable cases.

--scan TTY check. ensure_scan_is_interactive() now runs at the top of the Import arm, before the password ceremony and before the recovery phrase is read. Test: wallet_import_scan_without_terminal_fails_before_any_prompt (nonexistent mnemonic file, no password env; asserts the TTY error is what comes back and no wallet is created).

The mock node in chain::client moved to a test_support module with a stall_storage flag so both test modules share it.

@illuzen illuzen added the bot-review Request automated review from review-bot label Oct 19, 2026
@illuzen
illuzen requested a review from n13 October 19, 2026 07:36

@n13 n13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer model: GPT-6.1 Sol

APPROVE — the previous blocking findings are resolved; no blocking findings remain at head bb53b2c9a89188aa47d526c5aa5efbe13c116689 against base 413a6e23f7fc9ebbe8145d9cd887cd52a70cc933.

Both plain import and scan now share one deadline covering connection, account reads, and token lookup, preserving partial results when a node stalls. The scan terminal guard runs before secret-file reads or password/mnemonic prompts. Historical candidates reuse the existing derivation and import paths.

Non-blocking feedback:

  • [P3] Describe current state rather than claiming complete history. The plain-import warning and scan label infer “never received funds or sent a transaction” from the current System::Account balance/nonce. A previously used account that was reaped also returns zero/default state. Prefer wording such as “No current balance or nonce found” in both paths and the corresponding README text; otherwise a user can be told their correct historical address was never used. Address selection and import still work.

Validation:

  • SKIP_CIRCUIT_BUILD=1 cargo test --locked --no-default-features --lib wallet: 112 passed, including healthy/unreachable/stalled-node probes, bounded plain import, early non-TTY rejection, and historical derivation/import vectors.
  • Same test command with chain::client::tests: 6 passed.
  • SKIP_CIRCUIT_BUILD=1 cargo clippy --locked --all-targets --all-features -- -D warnings: passed.
  • Headless CLI build, CI-pinned cargo +nightly-2026-08-31 fmt --all -- --check, Taplo, and git diff --check: passed. Circuit generation was skipped for these wallet-focused checks.
  • Fresh PTY checks passed for default/legacy selection, cancellation, invalid input, and EOF; aborted selections wrote no wallet. Redirected stdin with a controlling terminal failed immediately before a password prompt or mnemonic-file access.

GitHub's Ubuntu build/tests, format, dependency cooldown, and Clippy/doc jobs passed. macOS build/tests and examples were still running at the final check. The security-audit job failed with 13 vulnerabilities in the existing dependency set; Cargo.toml, Cargo.lock, and the CI workflow are unchanged by this PR.

@n13 n13 removed the bot-review Request automated review from review-bot label Oct 19, 2026
… used

System::Account default state is also what a reaped account returns, so
the import warning, --scan label, --scan help and README no longer say
the address has no history.

Co-authored-by: Cursor <cursoragent@cursor.com>
@illuzen

illuzen commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Took the P3 in $(git rev-parse --short HEAD): the plain-import warning now reads "No current balance or nonce found for this address." (hint softened to "funds may be under a different derivation"), the --scan label is "no current balance or nonce", and the --scan help text and README section describe current balance/nonce rather than history. No behaviour change.

@illuzen illuzen added the bot-review Request automated review from review-bot label Oct 19, 2026
@illuzen
illuzen requested a review from n13 October 19, 2026 09:40

@n13 n13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer model: GPT-6.1 Sol

APPROVE — no blocking findings remain at head 33b25d34c81bb0cab263cb3fd5f9e3d061272d7a against base 413a6e23f7fc9ebbe8145d9cd887cd52a70cc933.

The latest commit resolves the remaining wording feedback in both import paths and the README: empty current account state is no longer presented as proof that an address was never used. The complete diff also preserves the earlier fixes: one overall 10-second deadline covers the optional node probe, and non-TTY scans fail before password prompts or mnemonic-file reads. Historical candidates reuse the existing derivation and import functions.

Validation at this head:

  • SKIP_CIRCUIT_BUILD=1 cargo test --locked --no-default-features --lib wallet: 112 passed, including historical vectors, healthy/unreachable/stalled nodes, bounded plain import, and early non-TTY rejection.
  • The same test command with chain::client::tests: 6 passed.
  • SKIP_CIRCUIT_BUILD=1 cargo clippy --locked --all-targets --all-features -- -D warnings: passed; headless CLI build passed.
  • CI-pinned cargo +nightly-2026-08-31 fmt --all -- --check, Taplo, and git diff --check: passed.
  • Fresh PTY checks passed for all four selections, cancellation, invalid input, and EOF. Each successful import matched its displayed address; aborted selections wrote no wallet. Redirected stdin with a controlling terminal and no password source failed immediately before secret access.

Circuit generation was skipped for these wallet-focused local checks. GitHub format, Clippy/doc, and security-audit jobs passed; Ubuntu/macOS build/tests and examples were still running at the final check. Both dependency-cooldown jobs failed because their merge-result checkout includes dependency updates already on main, compared against this PR's older base. This PR changes neither Cargo.toml nor Cargo.lock; see the cooldown log.

@n13 n13 removed the bot-review Request automated review from review-bot label Oct 19, 2026
@illuzen
illuzen merged commit fdd2224 into main Oct 19, 2026
7 checks passed
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.

2 participants