Cold wallet signing over QR (Keystone / Quantus cold wallet app)

[n13]

Adds air-gapped signing to the CLI: import a Keystone 3 or Quantus cold wallet app account as a watch-only wallet, then use it with any extrinsic command — quantus send --from <cold-wallet>, quantus multisig approve --from <cold-wallet>, etc. The CLI shows the transaction as a ur:quantus-sign-request QR, you sign on the device, and it scans the animated signature UR back with the laptop camera. wallet import-cold scans the device's address QR (or takes --address).

Key design decisions

• Commands don't know whether a wallet is hot or cold. A WalletSigner enum (Hot(QuantumKeyPair) / Cold { name, address }) replaces raw keypairs at every submit call site, and the shared submit stage in cli::common branches to the QR flow when the signer is watch-only. Every extrinsic command gets cold signing through the one shared path — no per-command special-casing — and commands that submit several extrinsics (e.g. runtime update, tech-referenda submit-with-preimage) do one QR roundtrip per extrinsic. Wormhole is the one deliberate exception: it derives secrets from the wallet's mnemonic and submits unsigned extrinsics, so it refuses cold wallets like any other key-requiring path.
• Cold I/O flags are global. --cold-request-out, --cold-response-in <file|->, and --camera-index are global CLI flags installed once from main, so scripted/headless flows work with any command; builds without the default camera feature support only this path.
• Fee preflight works for cold wallets via a dummy-signature estimate — the fixed-length Dilithium signature means the estimate is as accurate as a real one, with no key material needed.
• Byte-identical to the existing protocol. Uses the same quantus_ur crate (git tag 1.6.0) the mobile app, cold wallet app, and Keystone firmware pin, so the CLI is a drop-in third participant — no device-side changes needed.
• The QR carries the raw unhashed signing payload, built manually from subxt's public ExtrinsicParamsEncoder traits. subxt's own signer_payload() blake2-hashes payloads >256 bytes, which would make them unparseable (and undisplayable) on the device.
• Nonce/era are captured once into a TxContext and reused verbatim for both the QR and the submitted extrinsic, with a runtime cross-check that the two constructions agree. The old hardware_mark_1 branch silently refetched the nonce between display and submit, invalidating signatures — that bug is structurally excluded here.
• Responses are verified before submission: length, pubkey→address (poseidon) binding, and the ML-DSA-87 signature itself. A response from the wrong device aborts hard (naming the offending address); an incomplete scan offers a rescan. Signed extrinsics are never rebuilt/resigned behind the user's back — no retry loop.
• Cold wallets reuse the existing wallet-file format via a serde-defaulted wallet_type field: old files read as hot, old CLI versions still parse cold files, and list/find paths needed no changes. All key-requiring paths refuse cold wallets before any password prompt.
• Camera is a default-on feature flag (nokhwa + rxing — the ZXing port's adaptive binarizer decodes the defocused frames a fixed-focus laptop camera produces at phone-scanning distance, where quirc-style decoders fail); the signing machinery is generic over any call payload, which is what lets the shared submit stage route every command through it.

Testing

• Golden byte-layout test pinning the payload to the field layout the cold-wallet-app/Keystone parsers expect, plus an equivalence test pinning it to subxt's canonical signer payload (both the raw ≤256 B and hashed >256 B cases), using the vendored metadata offline.
• Signature validation unit tests: round-trip, truncated scan, wrong signer, stale payload.
• UR round-trip tests including the always-multi-part 7219-byte response with shuffled frames.
• Live camera e2e against the Quantus cold wallet app on a phone: wallet import-cold scanned the address QR and send --from <cold> completed the full animated-UR signature roundtrip through the laptop camera, submitted, and was included in a block.
• Live e2e against a dev node using the new hidden developer cold-sign-sim command (plays the cold-wallet side with a local hot wallet, exchanging UR parts over files): transfer signed, submitted, and included in a block; fee preview matched the actual fee to the unit; wrong-signer response aborted as expected.
• ./clippy.sh clean, full test suite passing, cargo build --no-default-features green.

Not yet tested: physical camera scanning against a real Keystone (needs Heisenberg/Planck — Keystone firmware enforces a genesis allowlist that excludes dev nodes).

Breaking SDK change

Library transfer/multisig helpers now take WalletSigner instead of a raw keypair (LIBRARY_USAGE.md and the examples are updated) — call this out in the changelog when releasing.

[n13]

Verdict: Approve ✅

Reviewed the full diff locally (branch checked out at cbf0553), plus cargo check --all-targets, cargo clippy --all-targets (both clean), and the new tests (cold_signing 4/4, qr 8/8, wallet 35/35 pass).

This is a carefully built feature. The things that matter most for signing code are done right:

• Single-capture invariant: nonce/era/block context captured once into TxContext and reused verbatim for QR payload and submitted extrinsic, with a runtime cross-check (partial.signer_payload() == signable_payload(raw)) that hard-fails before submission if the two constructions ever drift. The old hardware_mark_1 nonce-refetch bug is structurally excluded.
• Response verification before submit: length, poseidon pubkey→address binding, and the ML-DSA-87 signature itself. Wrong-key responses abort hard and name the offending address; no silent rebuild/resign retry loop.
• Protocol fidelity: the golden-layout test pinning the raw payload byte-for-byte (call ‖ era ‖ nonce ‖ tip ‖ mode ‖ specV ‖ txV ‖ genesis ‖ blockHash ‖ metadataHash) against the cold-wallet parser layout, plus the test pinning our manual builder to subxt's canonical signer_payload() above/below the 256-byte hash threshold, is exactly the right way to lock this down.
• Wallet-file compat: serde-defaulted wallet_type is the correct migration — old files read as hot, old binaries ignore the new field, and the legacy-JSON test proves it. All key-requiring paths (load_keypair_from_wallet, export_mnemonic, decrypt) refuse cold wallets before any password prompt.
• Cold send correctly mirrors the hot path: same get_latest_block + .mortal(256) anchor, same effective_tip_amount/positive_tip_amount helpers, same result summary via the extracted print_send_result.

Minor, non-blocking nits:

1. handle_cold_send balance preflight excludes the fee (src/cli/send.rs ~700): the comment says "fee estimation needs a signer", but sign_and_submit_cold already estimates the fee with a zeroed fixed-length Dilithium signature. You could reuse that estimate to fail before the QR dance when balance < amount + tip + fee, instead of letting the chain reject an already-signed extrinsic. At minimum the comment is slightly contradicted by the estimator's existence.
2. Non-interactive session without --cold-request-out (cold_signing.rs ~283): when stdin is not a terminal and no request file is given, the request is never surfaced anywhere and the CLI just waits for a response that can't be produced. An early error like "non-interactive cold signing requires --cold-request-out" would fail faster.
3. scan_ur_from_stdin ignores the timeout (src/qr/scanner.rs:92): UrSource::StdinLines blocks until complete/EOF regardless of the timeout parameter. Fine in practice (Ctrl-C kills it), but the unused deadline is a small surprise in the API.
4. --cold-request-out / --cold-response-in / --camera-index are silently ignored for hot wallets — a one-line warning when they're passed with a hot --from would catch user confusion.

None of these block merge. Ship it. 🧊

[n13]

Merged current main into this branch and finished the leftover wiring so cold signing compiles and works as a complete CLI path again.

What was blocking:

• The branch was 12 commits behind and conflicting with main (ML-DSA-65/87, vesting, checkphrases, spec 143, exercise budget).
• The crypto types on main renamed Dilithium → Dilithium87. Cold signing (devices are ML-DSA-87 only) now uses those types.
• New vesting submit/claim paths went through load_keypair and would have refused a cold --from. They now use WalletSigner like every other extrinsic command.
• quantus_ur was still on 1.4.0 while the mobile/cold-wallet apps pin 1.6.0. Bumped; Keystone 1.4.0 encodings still decode.

Review nits from the earlier pass, now done:

• Non-interactive sessions without --cold-request-out fail immediately instead of waiting for a response that cannot be produced.
• --cold-request-out / --cold-response-in / --camera-index warn when used with a hot wallet.
• Stdin UR collection honors the timeout.

Still not done here: a live camera scan against a real Keystone / cold-wallet app. Devices only sign Planck/Heisenberg genesis + the whitelisted calls (transfers, reversible, and on the app also multisig). Dev-node e2e is still the file-based developer cold-sign-sim path.

Cold/QR/wallet unit tests pass locally (5 cold_signing, 8 qr, wallet suite).

[n13]

Every signed submit now goes through one fork in submit_transaction / submit_transaction_with_inclusion_block / submit_transaction_with_nonce:

• Hot WalletSigner → local Dilithium sign
• Cold WalletSigner → QR request/response (sign_and_submit_cold)

CLI commands already loaded wallets via load_signer_from_wallet. The remaining gap was the library helpers (transfer, batch_transfer, create_multisig, propose_*, approve_proposal, cancel_proposal) and a few examples that signed with QuantumKeyPair / raw subxt sign_and_submit. Those now take WalletSigner too, so a cold --from or a cold library signer cannot skip the QR path.

Unsigned wormhole/collect-rewards submits are unchanged (no key).

[n13]

Verdict: REQUEST_CHANGES

The core signing invariant is sound, but this head still has blocking lifecycle and validation defects.

1. Cold import can overwrite a concurrently-created wallet (src/wallet/mod.rs:271-291). create_cold_wallet performs a check-then-save with Keystore::save_wallet, whose rename intentionally replaces an existing destination. If another CLI process creates a hot wallet with the same name after the check, the cold import replaces that file and can permanently discard its encrypted keys. Every existing creation path takes lock_wallet_create and finishes with the no-replace save_new_wallet; reuse that path here and add the concurrent-creation regression.

2. The documented file transport consumes stale sessions as current ones (src/qr/scanner.rs:79-90, src/cli/cold_signing.rs:367-380, and the simulator request read around src/cli/cold_signing.rs:470-478). A complete response file from extrinsic N is returned immediately for extrinsic N+1, before the external signer can replace it; validation then reports BadSignature and aborts. The simulator has the symmetric problem with an old request file. This makes reused file paths and multi-extrinsic commands unreliable in the headless flow. Establish per-roundtrip freshness/consumption semantics (or unique session files) and cover two consecutive exchanges using the same configured paths.

3. Arbitrary non-ASCII QR/file input can panic the scanner (src/qr/scanner.rs:35-37). trimmed[..3] slices at a byte offset that need not be a UTF-8 boundary (for example, a decoded QR beginning with two multi-byte characters). This input is explicitly untrusted and should be ignored or returned as an error, not unwind the CLI; use a boundary-safe prefix check and add a regression.

4. The required Clippy gate fails at this head: SKIP_CIRCUIT_BUILD=1 cargo clippy --all-targets --locked -- -D warnings rejects src/qr/scanner.rs:229-230 (field_reassign_with_default) and src/qr/scanner.rs:396 (manual_is_multiple_of). GitHub's Analysis check is failing for the same reason.

Validation on exact base f0a6432 / head 44804ef: git diff --check and cargo +nightly fmt --all -- --check passed; SKIP_CIRCUIT_BUILD=1 cargo check --locked --no-default-features passed; the library run passed 274/275 tests, including all new cold-signing, QR, and wallet tests, with only the unrelated generated-bins test failing because SKIP_CIRCUIT_BUILD=1 intentionally omits those artifacts. No blocking issue was found in the nonce/era reuse, raw-payload/subxt equivalence check, signer-address binding, or pre-submission signature verification.

[n13]

Addressed all four blocking items from the review in 33b1f2b:

1. Cold import creation race — create_cold_wallet now acquires lock_wallet_create and saves through the atomic no-replace save_new_wallet (hard_link fails with AlreadyExists when the name is taken), matching every other creation path. The new test_cold_import_cannot_replace_concurrent_creation regression races 8 concurrent importers on one name (exactly one wins) and asserts an existing hot wallet's file — including its encrypted keys — survives an import attempt byte-for-byte.

2. Stale file-transport sessions — per-roundtrip consumption semantics: scan_ur_from_file now deletes the file after a successful complete read, which covers both directions (the CLI's response read and the simulator's request read), and sign_and_submit_cold removes any response file that predates its own request (a response existing before the request is handed out is stale by construction). test_scan_ur_from_file_consumes_file_between_sessions covers two consecutive exchanges over the same configured path: the first read consumes the file and the second waits for the fresh session instead of replaying the old one.

3. Non-ASCII scanner panic — is_ur_line compares raw bytes (as_bytes()[..3].eq_ignore_ascii_case(b"ur:")), so a decoded QR starting with multi-byte characters can no longer slice off a UTF-8 boundary. Regression with 2-, 3-, and 4-byte leading characters added.

4. Clippy gate — SKIP_CIRCUIT_BUILD=1 cargo clippy --all-targets --locked -- -D warnings is clean at this head: DecodeHints built with struct-update syntax, frames.is_multiple_of(15), plus an unused import in examples/basic_usage.rs the gate also caught.

cargo +nightly fmt --all -- --check passes and the library suite is 278/278.

[n13]

Verdict: APPROVE

No blocking findings remain at exact head 33b1f2b.

The four blockers from the previous review are resolved:

• Cold import now holds the per-name creation guard and finishes through save_new_wallet (src/wallet/mod.rs:272, src/wallet/mod.rs:290). The latter atomically links the new file without replacement, so a competing process cannot overwrite an existing hot wallet.
• File-based QR sessions now remove any response that predates the new request and consume a file after a successful complete decode (src/cli/cold_signing.rs:337, src/qr/scanner.rs:83). This gives reused paths fresh-session semantics for multi-extrinsic/headless flows.
• The UR prefix check is byte-safe for arbitrary UTF-8 scanner/file input (src/qr/scanner.rs:38).
• The all-target Clippy failures are fixed.

The signing-critical invariants remain intact: transaction context is captured once for the displayed and submitted payload, the constructed signer payload is cross-checked before submission, and the response is length-, signer-address-, and ML-DSA-87-signature-validated.

Validation on base f0a6432 / head 33b1f2b:

• git diff --check f0a6432...33b1f2b — passed
• cargo +nightly fmt --all -- --check — passed
• Exact regressions for concurrent cold import, consecutive file sessions, and non-ASCII UR input — 3/3 passed
• SKIP_CIRCUIT_BUILD=1 cargo clippy --all-targets --locked -- -D warnings — passed
• SKIP_CIRCUIT_BUILD=1 cargo check --locked --no-default-features — passed (one non-blocking camera-only dead-code warning)
• GitHub format, Linux/macOS build-and-test, Clippy/doc, security-audit, and examples checks — all passed at this head

No blocking findings.

[illuzen]

Reviewed PR #123 (cold-wallet-signing → main, draft, head 44804ef). Verdict: request changes — the signing core is solid, but a few lifecycle/scanner bugs should land before merge.

The earlier in-PR review is still accurate: those defects are still in this head. I also found one more scanner bug that review did not call out.

What is in good shape
The signing invariant is the right one. Nonce, era, and block hash are captured once into TxContext, the QR and the submitted extrinsic are built from that snapshot, and partial.signer_payload() == signable_payload(raw) hard-fails before submit if the two constructions drift.

Response checks before submit are also right: length, pubkey→address binding, then ML-DSA-87 verify. A wrong-key response aborts and names the address. There is no silent rebuild/resign loop.

WalletSigner plus the shared fork in submit_transaction / submit_transaction_with_inclusion_block / submit_transaction_with_nonce is the correct architecture. Commands stay hot/cold-agnostic; wormhole still refuses cold wallets because it needs a mnemonic.

Tests around payload layout, subxt equivalence, signature validation, and stale-prefix UR recovery are the right kind of lock-in for this protocol.

Blocking

1. Cold import can overwrite a concurrently created wallet

create_cold_wallet is the only creation path that skips lock_wallet_create and finishes with save_wallet (replace-on-rename) instead of save_new_wallet (hard-link, no replace):

mod.rs
Lines 268-291
pub fn create_cold_wallet(&self, name: &str, address: &str) -> Result {
// ...
if keystore.load_wallet(name)?.is_some() {
return Err(WalletError::AlreadyExists.into());
}
// ...
let encrypted_wallet = keystore::EncryptedWallet::new_cold(name, address.trim());
keystore.save_wallet(&encrypted_wallet)?;
If another process creates a hot wallet with the same name after the exists check, the cold import replaces that file and can discard encrypted keys. Reuse the existing create lock + save_new_wallet, and add the concurrent-creation regression the other create paths already have.

Also store the canonical SS58 (account_id.to_ss58check_with_version(...)), not address.trim(). load_wallet rejects non-canonical encodings, so a valid-but-non-canonical paste would create a wallet that cannot be loaded.

2. File transport treats a leftover complete UR as the current session

scan_ur_from_file returns as soon as the path already holds a complete set. A leftover response from extrinsic N is consumed as N+1; validation then reports BadSignature and aborts (not rescan-safe). The simulator has the same problem on the request file.

This breaks reused --cold-request-out / --cold-response-in paths and any multi-extrinsic command (runtime update, tech-referenda submit-with-preimage). Need per-roundtrip freshness or consume-on-read, plus a test of two consecutive exchanges on the same paths.

3. decode_any_suffix prefers the oldest complete stream, not the newest

scanner.rs
Lines 55-61
fn decode_any_suffix(parts: &[String]) -> Result<Option<Vec>> {
for start in 0..parts.len() {
if let Some(bytes) = decode_if_complete(&parts[start..])? {
return Ok(Some(bytes));
}
}
Ok(None)
}
The commit says dropping oldest parts recovers the newest stream. The loop returns the first complete suffix, which is the oldest. The existing test only covers an incomplete stale prefix (3 old parts + a full new set). If the camera captures a complete previous signature before the new animation starts — the case that commit describes — this returns the stale payload, verification becomes BadSignature, and the CLI aborts with no rescan.

Keep the last successful decode, or iterate newest-first. Extend the test to a complete stale stream followed by a complete new one.

4. Untrusted QR/file input can panic the scanner

scanner.rs
Lines 35-38
fn is_ur_line(line: &str) -> bool {
let trimmed = line.trim();
trimmed.len() > 3 && trimmed[..3].eq_ignore_ascii_case("ur:")
}
len() > 3 is bytes; [..3] is not a UTF-8 boundary. A decoded QR starting with two multi-byte characters (for example "áá…") panics. This input is untrusted. Use a boundary-safe prefix check (strip_prefix / get(..3)) and add a regression.

5. Clippy gate is red

SKIP_CIRCUIT_BUILD=1 cargo clippy --all-targets --locked -- -D warnings still fails on src/qr/scanner.rs: field_reassign_with_default around the DecodeHints assignment, and manual_is_multiple_of on frames % 15 == 0.

Non-blocking
PR body is stale: it still cites quantus_ur 1.4.0 and rqrr; the branch is on 1.6.0 and rxing.
--cold-request-out / --cold-response-in are hidden global flags. README documents them, but quantus --help does not. Consider un-hiding them or pointing at them from --camera-index help.
No live camera pass against a real Keystone / cold-wallet app yet. That is called out and is fine for a draft; I would not merge to a release without at least one Planck/Heisenberg hardware roundtrip.
Library transfer / multisig helpers now take WalletSigner. LIBRARY_USAGE.md and examples are updated; this is a breaking SDK change and should be called out in the changelog / PR summary.

[n13]

Re: the follow-up review — it was taken at head 44804ef; items 1, 2, 4 and 5 were already fixed in 33b1f2b (see the summary above). The two new findings are now addressed in 38b51ab:

3. decode_any_suffix preferred the oldest complete stream — confirmed, that was a real bug in the 33b1f2b fix: the loop returned the first complete suffix, i.e. the stale stream when both were complete in the capture. It now iterates shortest suffix first, so the newest complete stream always wins. The regression gained the complete-stale + complete-new case, asserting the new payload is returned even though the stale prefix also forms a complete set.

1 (addendum). Canonical SS58 — create_cold_wallet now persists account.to_ss58check_with_version(quantus_ss58_format()) instead of the pasted string, so a valid-but-non-canonical encoding can no longer create a wallet that load_wallet refuses to read back.

Non-blocking items:

• PR body refreshed: quantus_ur 1.6.0, rxing (with the why), live-camera status, and a breaking-SDK-change section for the WalletSigner signature change.
• --cold-request-out / --cold-response-in are no longer hidden from --help.
• Live hardware: the Quantus cold wallet app roundtrip (address import + full animated signature scan through the laptop camera, submitted and included) has now been done; a real Keystone pass on Heisenberg/Planck remains open before a release merge.

Gates at 38b51ab: SKIP_CIRCUIT_BUILD=1 cargo clippy --all-targets --locked -- -D warnings clean, cargo +nightly fmt --all -- --check clean, library suite 278/278.