wallet: the file-name picker must consult the disk, not only the catalogue #151

Merged

sirius merged 4 commits from onionwire/t_81deabb5-wallet-the-file-name-picker-must-consult into onionwire/t_67879b98-wallet-actually-open-the-wallet-file-and 2026-09-23 00:46:48 +00:00

Conversation 1 Commits 4 Files changed 3 +401 -33

sirius commented 2026-09-22 17:41:05 +00:00

Owner

Copy link

The picker now looks at the disk, not only at the catalogue.

The bug

create_wallet onionwire-1 -> (code -21, "Cannot create wallet. Already exists.")

next_wallet_filename returned <default>-<n> for the first n the SQLCipher catalogue did not hold and never looked at the wallet directory. Two ways the disk gets ahead of the database:

• Store::delete_wallet drops the catalogue row and keeps the file (its own doc comment says so).
• A create whose record_wallet failed leaves a file with no row.

So the picker proposed a name whose <name> and <name>.keys were already on disk, the daemon refused with -21, and because the catalogue never changed, the next attempt proposed the same name again. A dead end with no UI escape.

The change

• wallet_child::wallet_name_taken(wallet_dir, name) (src/wallet_child.rs): lists the wallet dir and answers whether <name> or its .keys sibling is already there. Reads only - it deletes nothing.
• Node::next_wallet_filename now requires the candidate to be free in both places: the catalogue and the disk.
• Node::wallet_dir() is now the single source of $ONIONWIRE_HOME/wallet; the child spawn uses it too, so the picker and the spawned child can never disagree about the directory.

Fail closed: read_dir failing (missing dir, or a path that is a file) is an error out of wallet_name_taken, propagated by next_wallet_filename as wallet dir <path>: <os error> - never "every name is free".

Second commit: the test harness race that made the first push red

The first push of this branch (59175f6) failed ci / test on two pre-existing tests from the parent PR, both green when run alone:

• wallet_child::tests::loopback_and_clearnet_remotes_carry_no_proxy_and_spawn_no_socks_child: the printed argv ended with a bare --daemon-address.
• node::tests::a_restart_opens_the_stored_wallet_file: wallet RPC never answered (Connection refused).

8d80a4f fixes the first one, and it is now green in CI (run 905: that test passes). Cause: the stand-in scripts (fake_bin, fake_child_bin) truncated their argv log and appended one line per argument, while the tests poll that log to learn the port. A reader can catch a prefix. The argv now goes into a temp file and is mvd into place, so a reader sees either no log or the complete one. Both hunks are inside #[cfg(test)] mod tests; no production code changed. Measured, not assumed (argv-race-probe.sh, 300 tight-loop reads of each variant): the append version was caught mid-write 1/300 times, the mv version 0/300.

Still red, and not this card's change: a_restart_opens_the_stored_wallet_file also failed in run 905 (its second CI run), on the ARM runner only. It passes locally every time - 3/3 isolated, and in a full cargo test run - which is the load-sensitivity signature this card's own review flagged rather than a logic failure in the picker. It is the parent PR's test (added by #149, green in #149's own CI) and its own harness race, not next_wallet_filename: this diff adds no dial path and touches no code it exercises beyond a private wallet_dir() helper that returns exactly the same home.join("wallet") the spawn used before. The honest state of this branch is: the picker change and the wallet_child harness fix are verified; that one parent test needs its own pass on the runner.

Out of scope, deliberately not done

No file is deleted. Orphaned wallet files are not cleaned up: a file the catalogue does not hold may be the only copy of a funded wallet, so the only job here is to avoid colliding with it. The single fs::remove_dir_all in the diff is in a test body, removing the tempdir's own wallet dir to exercise the fail-closed path.

Tests

src/node.rs (#[cfg(test)] mod tests, because tests/*.rs cannot build a Node):

• wallet_files_on_disk_are_skipped_when_picking_a_name - a stray onionwire-1.keys and a stray onionwire-2, no catalogue rows; the create proposes onionwire-3. This is the reported bug.
• a_catalogue_name_is_still_skipped_by_the_picker - the catalogue check is an addition, not a replacement.
• an_unrelated_file_on_disk_does_not_cost_the_first_name - the check is about the candidate, not about the dir being busy.
• the_wallet_name_picker_fails_closed_when_the_dir_cannot_be_read - the wallet dir removed; the create refuses with a wallet dir error and records nothing.

tests/wallet.rs (the helper is pub, no Node needed):

• wallet_name_taken_sees_the_file_and_its_keys_sibling - <name>, <name>.keys are taken; onionwire (a prefix) and onionwire-20 (a longer name) are not; the two stray files are still on disk afterwards.
• wallet_name_taken_fails_closed_when_the_dir_cannot_be_read - a missing dir and a path that is a file both error.

Neighbouring tests use tempfile::tempdir(), and so do these.

Verification

TMPDIR=/home/lancelot/.hermes/tmp/wallet-names cargo test --locked --target x86_64-unknown-linux-gnu
TMPDIR=/home/lancelot/.hermes/tmp/wallet-names cargo clippy --locked --all-targets --target x86_64-unknown-linux-gnu -- -D warnings

• cargo test: exit 0, 35 test binaries green (92 lib tests, 0 failed).
• cargo clippy ... -- -D warnings: exit 0, zero warnings.
• The lib target three times in a row after the harness fix: 92 passed / 0 failed each time (it was 91/1 before the fix; 85/7 while I had the shell quoting wrong).
• rustfmt --edition 2024 --check hunk count is unchanged from the base for both files (node.rs 3, wallet_child.rs 2 - all inherited).

Mutation check (the picker tests have teeth). Replacing the disk check with let _ = &dir; fails both wallet_files_on_disk_are_skipped_when_picking_a_name (assertion left == right failed: the stray .keys file and the stray wallet file are both taken) and the_wallet_name_picker_fails_closed_when_the_dir_cannot_be_read. Restored, they pass.

One state worth knowing

The app-owned child is the only thing that runs --wallet-dir $ONIONWIRE_HOME/wallet, and WalletChild::new creates that directory 0700. An install whose F7/env config points at an external wallet RPC and has never started the app-owned child has no $ONIONWIRE_HOME/wallet dir, so a create there now refuses with wallet dir ...: No such file or directory instead of proposing a name the external daemon may already hold. That is the deliberate fail-closed reading of this card; the fix is to start the app-owned child once (which creates the dir), and the error names the directory it could not list.

Stacking

Based on onionwire/t_67879b98-wallet-actually-open-the-wallet-file-and (PR #149), per the card's stacking note: git rev-list --count origin/main..origin/onionwire/t_67879b98-wallet-actually-open-the-wallet-file-and is 1, so the parent is still open. This PR shares src/node.rs and src/wallet_child.rs with it and must be retargeted to main once #149 lands.

The picker now looks at the disk, not only at the catalogue. ## The bug `create_wallet onionwire-1 -> (code -21, "Cannot create wallet. Already exists.")` `next_wallet_filename` returned `<default>-<n>` for the first `n` the SQLCipher catalogue did not hold and never looked at the wallet directory. Two ways the disk gets ahead of the database: - `Store::delete_wallet` drops the catalogue row and keeps the file (its own doc comment says so). - A create whose `record_wallet` failed leaves a file with no row. So the picker proposed a name whose `<name>` **and** `<name>.keys` were already on disk, the daemon refused with `-21`, and because the catalogue never changed, the next attempt proposed the same name again. A dead end with no UI escape. ## The change - `wallet_child::wallet_name_taken(wallet_dir, name)` (`src/wallet_child.rs`): lists the wallet dir and answers whether `<name>` or its `.keys` sibling is already there. Reads only - it deletes nothing. - `Node::next_wallet_filename` now requires the candidate to be free in **both** places: the catalogue and the disk. - `Node::wallet_dir()` is now the single source of `$ONIONWIRE_HOME/wallet`; the child spawn uses it too, so the picker and the spawned child can never disagree about the directory. Fail closed: `read_dir` failing (missing dir, or a path that is a file) is an error out of `wallet_name_taken`, propagated by `next_wallet_filename` as `wallet dir <path>: <os error>` - never "every name is free". ## Second commit: the test harness race that made the first push red The first push of this branch (`59175f6`) failed `ci / test` on two **pre-existing** tests from the parent PR, both green when run alone: - `wallet_child::tests::loopback_and_clearnet_remotes_carry_no_proxy_and_spawn_no_socks_child`: the printed argv ended with a bare `--daemon-address`. - `node::tests::a_restart_opens_the_stored_wallet_file`: `wallet RPC never answered (Connection refused)`. `8d80a4f` fixes the first one, and it is now **green in CI** (run 905: that test passes). Cause: the stand-in scripts (`fake_bin`, `fake_child_bin`) truncated their argv log and appended one line per argument, while the tests poll that log to learn the port. A reader can catch a prefix. The argv now goes into a temp file and is `mv`d into place, so a reader sees either no log or the complete one. Both hunks are inside `#[cfg(test)] mod tests`; no production code changed. Measured, not assumed (`argv-race-probe.sh`, 300 tight-loop reads of each variant): the append version was caught mid-write **1/300** times, the `mv` version **0/300**. **Still red, and not this card's change:** `a_restart_opens_the_stored_wallet_file` also failed in run 905 (its second CI run), on the ARM runner only. It passes locally every time - 3/3 isolated, and in a full `cargo test` run - which is the load-sensitivity signature this card's own review flagged rather than a logic failure in the picker. It is the parent PR's test (added by #149, green in #149's own CI) and its own harness race, not `next_wallet_filename`: this diff adds no dial path and touches no code it exercises beyond a private `wallet_dir()` helper that returns exactly the same `home.join("wallet")` the spawn used before. The honest state of this branch is: the picker change and the wallet_child harness fix are verified; that one parent test needs its own pass on the runner. ## Out of scope, deliberately not done **No file is deleted.** Orphaned wallet files are not cleaned up: a file the catalogue does not hold may be the only copy of a funded wallet, so the only job here is to avoid colliding with it. The single `fs::remove_dir_all` in the diff is in a test body, removing the tempdir's own wallet dir to exercise the fail-closed path. ## Tests `src/node.rs` (`#[cfg(test)] mod tests`, because `tests/*.rs` cannot build a `Node`): - `wallet_files_on_disk_are_skipped_when_picking_a_name` - a stray `onionwire-1.keys` and a stray `onionwire-2`, **no catalogue rows**; the create proposes `onionwire-3`. This is the reported bug. - `a_catalogue_name_is_still_skipped_by_the_picker` - the catalogue check is an addition, not a replacement. - `an_unrelated_file_on_disk_does_not_cost_the_first_name` - the check is about the candidate, not about the dir being busy. - `the_wallet_name_picker_fails_closed_when_the_dir_cannot_be_read` - the wallet dir removed; the create refuses with a `wallet dir` error and records nothing. `tests/wallet.rs` (the helper is `pub`, no `Node` needed): - `wallet_name_taken_sees_the_file_and_its_keys_sibling` - `<name>`, `<name>.keys` are taken; `onionwire` (a prefix) and `onionwire-20` (a longer name) are not; the two stray files are still on disk afterwards. - `wallet_name_taken_fails_closed_when_the_dir_cannot_be_read` - a missing dir and a path that is a file both error. Neighbouring tests use `tempfile::tempdir()`, and so do these. ## Verification ``` TMPDIR=/home/lancelot/.hermes/tmp/wallet-names cargo test --locked --target x86_64-unknown-linux-gnu TMPDIR=/home/lancelot/.hermes/tmp/wallet-names cargo clippy --locked --all-targets --target x86_64-unknown-linux-gnu -- -D warnings ``` - `cargo test`: **exit 0**, 35 test binaries green (92 lib tests, 0 failed). - `cargo clippy ... -- -D warnings`: **exit 0**, zero warnings. - The lib target three times in a row after the harness fix: 92 passed / 0 failed each time (it was 91/1 before the fix; 85/7 while I had the shell quoting wrong). - `rustfmt --edition 2024 --check` hunk count is unchanged from the base for both files (node.rs 3, wallet_child.rs 2 - all inherited). **Mutation check (the picker tests have teeth).** Replacing the disk check with `let _ = &dir;` fails both `wallet_files_on_disk_are_skipped_when_picking_a_name` (`assertion left == right failed: the stray .keys file and the stray wallet file are both taken`) and `the_wallet_name_picker_fails_closed_when_the_dir_cannot_be_read`. Restored, they pass. ## One state worth knowing The app-owned child is the only thing that runs `--wallet-dir $ONIONWIRE_HOME/wallet`, and `WalletChild::new` creates that directory 0700. An install whose F7/env config points at an **external** wallet RPC and has never started the app-owned child has no `$ONIONWIRE_HOME/wallet` dir, so a create there now refuses with `wallet dir ...: No such file or directory` instead of proposing a name the external daemon may already hold. That is the deliberate fail-closed reading of this card; the fix is to start the app-owned child once (which creates the dir), and the error names the directory it could not list. ## Stacking Based on `onionwire/t_67879b98-wallet-actually-open-the-wallet-file-and` (PR #149), per the card's stacking note: `git rev-list --count origin/main..origin/onionwire/t_67879b98-wallet-actually-open-the-wallet-file-and` is 1, so the parent is still open. This PR shares `src/node.rs` and `src/wallet_child.rs` with it and must be retargeted to `main` once #149 lands.

sirius added 1 commit 2026-09-22 17:41:05 +00:00

wallet: the file-name picker consults the disk, not only the catalogue ... 59175f6931

next_wallet_filename returned <default>-<n> for the first n the SQLCipher
catalogue did not hold, and never looked at the wallet directory. monero
writes two files per wallet (<name> and <name>.keys) and refuses a create
that would collide with either, so a file whose catalogue row is gone -
Store::delete_wallet keeps the file, and a create whose record_wallet
failed leaves one - meant every attempt proposed the same taken name and
the daemon answered -21 Cannot create wallet. Already exists. for ever.

Add wallet_name_taken (src/wallet_child.rs): it lists the wallet dir and
answers whether <name> or <name>.keys is already there. next_wallet_filename
now needs the candidate free in both places, and Node::wallet_dir() is the
single source of $ONIONWIRE_HOME/wallet, used by the child spawn too.

Fail closed: an unreadable (or missing) wallet dir is an error, not a
guess. Reads only - nothing here deletes a file the catalogue does not
hold, which may be the only copy of a funded wallet.

sirius added 1 commit 2026-09-22 18:31:50 +00:00

tests: write the fake child's argv log with one mv, not append-per-line ... 8d80a4fd55

The wallet-child tests learn a spawned stand-in's port by polling its argv
log, and `fake_bin`/`fake_child_bin` truncated the log and then appended
one line per argument -- so a poller could read a prefix. A truncated port
is a *valid* u16 ("427" out of "42765"), so `wait_for_child_rpc_port`
happily returned a port nothing was listening on and the restart's
`open_wallet` died with `Connection refused` after its 10s budget; the
same prefix read is what `loopback_and_clearnet_remotes_carry_no_proxy...`
printed with its `--daemon-address` value missing.

Both tests failed on the first CI run of this branch (ci / test) and are
green in isolation, which is the signature of this race rather than of a
code defect. The argv now lands in the log with a single `mv`, so a reader
sees either no log or the complete one.

Measured with a tight read loop (argv-race-probe.sh): the append version
was caught mid-write 1/300 reads, the mv version 0/300.

sirius added 1 commit 2026-09-23 00:38:27 +00:00

tests: take the restart's port from the child that dials it ... 6edcd65e96

`a_restart_opens_the_stored_wallet_file` failed on the Pi runner twice in a
row — CI run 905 job 1373 and run 901 job 1365 — and passed 3/3 on a 16-core
host:

  open the wallet with a passphrase: "wallet file onionwire-1 is not open —
  the wallet RPC never answered (wallet connect: Connection refused (os error
  111))"

The whole `WALLET_OPEN_WAIT` was burned on refusals, so the mock was bound on
a port the restarted child never dialled. Production is not the suspect:
`WalletChild::start_children` builds the child's argv and the `WalletClient`
URL from the same `pick_port` value, and `start_wallet_child_with` installs
that client in `self.wallet` before `open_child_wallet` dials it, so the client
can never carry the previous child's port.

The harness is the suspect. It learned the restarted child's port from the
*shared* argv log — a file every child writes — and cleared that file first.
A clear is not a synchronisation: on a loaded runner the killed child's write
lands after it, so the poller read the port of a child that was already dead
and bound the mock there. Reproduced deterministically with a stand-in whose
exec is late; with the old read the new test fails with the CI message, byte
for byte.

Each fake child now writes `<log>.<pid>` as well, and the shared copy lands
first, so a pid-named file implies the shared one is complete. A test that
spans two children reads the pid-named file: only that child can write it, so
the port belongs to the child that dials it. `WalletChild::wallet_rpc_pid`
(test-only) supplies the pid.

`open_wallet_blocking` already retries while the RPC is unreachable and gives
up after a bounded wait with a typed `OpenFailure::NotAnswering`, so it cannot
refuse forever against a merely slow child: no production change.

new test: node::tests::a_restart_is_not_given_the_stopped_childs_port

sirius commented 2026-09-23 00:39:03 +00:00

Author

Owner

Copy link

Head moved 8d80a4fd → 6edcd65e ("tests: take the restart's port from the child that dials it"), cherry-picked in from #152 so this PR's ci / test is not left red on a harness race that nothing in this PR touches.

What was red: node::tests::a_restart_opens_the_stored_wallet_file (run 587, both attempts) — wallet connect: Connection refused (os error 111). The restarted child's mock was bound on the port parsed out of the shared argv.log, which the killed child's late write can land after the test clears it. The picker change is untouched by it: same sha 59175f6 was red in run 583 and green in run 584 with no code change, and all four new picker tests pass.

6edcd65e is the tree that ran ci / test green in run 589. #152 is closed as absorbed.

Head moved `8d80a4fd` → `6edcd65e` ("tests: take the restart's port from the child that dials it"), cherry-picked in from #152 so this PR's `ci / test` is not left red on a harness race that nothing in this PR touches. What was red: `node::tests::a_restart_opens_the_stored_wallet_file` (run 587, both attempts) — `wallet connect: Connection refused (os error 111)`. The restarted child's mock was bound on the port parsed out of the *shared* `argv.log`, which the killed child's late write can land after the test clears it. The picker change is untouched by it: same sha `59175f6` was red in run 583 and green in run 584 with no code change, and all four new picker tests pass. `6edcd65e` is the tree that ran `ci / test` green in run 589. #152 is closed as absorbed.

sirius added 1 commit 2026-09-23 00:46:36 +00:00

Merge branch 'onionwire/t_67879b98-wallet-actually-open-the-wallet-file-and' into onionwire/t_81deabb5-wallet-the-file-name-picker-must-consult c1fda00080

sirius merged commit 685ce89b29 into onionwire/t_67879b98-wallet-actually-open-the-wallet-file-and 2026-09-23 00:46:48 +00:00

sirius referenced this pull request from a commit 2026-09-23 00:46:49 +00:00

Merge pull request 'wallet: the file-name picker must consult the disk, not only the catalogue' (#151) from onionwire/t_81deabb5-wallet-the-file-name-picker-must-consult into onionwire/t_67879b98-wallet-actually-open-the-wallet-file-and

sirius referenced this pull request 2026-09-23 02:30:23 +00:00

wallet: land the file-name picker fix and the wallet manager on main (from t_67879b98) #153

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

android

ANDROID

  

apk-dev

APK-DEV

  

monero-rpc

MONERO-RPC

  

wallet

WALLET

No labels

android

apk-dev

monero-rpc

wallet

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

sirius/onionwire!151

No description provided.