Repository navigation
gui: Prepare records and restore from their date - #113
BenWestgate wants to merge 4 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7181c7b70e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d53a4a426
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BenWestgate
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1863df760
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d65adc189
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review (Codex), posted at the maintainer's request.
Not ACKing 175cf4c yet: the GUI budget is documented consistently at 2,050 now, but its required explicit maintainer authorization is still missing.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head review at 175cf4c: one existing P2 remains. The GUI budget is now documented as 2,050 lines, but the repository contract requires explicit maintainer authorization for a budget increase and none is recorded.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review of 175cf4c: one blocking documentation/security-contract issue. The new Before you start text says the separate wallet record “proves later that cards you restore are this wallet.” That overstates the settled threat model: the 32-bit master fingerprint is a human-scale accident-safety check for wrong/mixed cards and transcription/correction mistakes; it is not malicious-tampering authentication. Please change this to wording such as “helps confirm later that the restored cards match your recorded wallet before import,” while keeping the stronger authenticated descriptor-backup work separate. I found no other blocker in the bundled-form launcher, package-data boundary, handwriting key, or pre-seed checklist flow.
207c0f4 to
e43102f
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
This comment has been minimized.
This comment has been minimized.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI current-head review: ACK bd49e52 for this branch's scope. The checklist occurs before secret generation, only bundled blank forms are handed to the desktop, GTK <4.10 takes the explicit path fallback, package data includes both forms, and CODEX32_FORMS_DIR cleanly supplies a confinement-readable copy. The remaining Tails integration dependency is already explicit: Bails must copy the forms into a Tor-Browser-readable directory and set that variable when it repins. No additional code finding here.
|
cNACK put a dot in each 6, there's no evidence this is a standard way to disambiguate these letters, we can simply say to make G's horizontal bar obvious and open, while to close the loops of 6 and 9 |
|
Done in d08bbbe. The dot is gone. The window, the printable recovery card and Generated by Claude Code |
|
please read CONTRIBUTING.md and squash this into atomic easy to review commits before human review. |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review: Concept ACK d08bbbe. The form preflight precedes seed generation; the corrected handwriting key is consistent. The #139 changes gate dated restore on Core's prune/AssumeUTXO state before import and keep new wallets loaded on startup, with focused tests. Not ready for human review until the requested atomic commit cleanup; also test the complete restore and form-opening flow on supported Tails.
Introduce a pre-creation checklist, packaged printable forms and fallback paths for older GTK and Tails. Include the final handwriting conventions for easily confused codex32 symbols and update GUI tests, security documentation, and the approved reviewability budget. Refs BenWestgate/Bails#314
d08bbbe to
aea9da6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aea9da63e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if core._target(name) is None or any(type(w) is not str or "could not be updated" in w for w in notes): | ||
| raise BitcoinCoreError("Bitcoin Core did not create an empty wallet that it loads at every start.") |
There was a problem hiding this comment.
When createwallet succeeds but reports that load_on_startup could not be saved, this raises only after Core has already created and loaded the blank wallet. Core's createwallet contract returns the created wallet name together with any warnings, so the wallet remains eligible to _wallets; after choosing Start again or Restore my wallet, the operator can select and fill it even though it will not load after a restart. On a pruned node that wallet can then fall behind the retained blocks, defeating the protection this option was added to provide. Keep this wallet out of subsequent selection, or otherwise ensure it cannot be filled until startup persistence is confirmed.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
report Core's warning is sufficient.
| if mismatch: | ||
| return _fingerprint_page(view, core, secret, timestamp, restoring=restoring, problem=mismatch) | ||
| _wallets(view, core, secret, timestamp, expected, restoring=restoring) | ||
| _wallets(view, core, secret, start, expected, restoring=restoring) |
There was a problem hiding this comment.
When restoration uses a nonblank creation date, this line passes start—one day before that date—to importdescriptors; Core documents that the rescan is based on the earliest supplied timestamp. The resulting _RESTORED screen nevertheless tells the operator that Core is scanning “from the beginning.” A late or mistyped approximate date can therefore omit older transactions while the completion screen falsely implies that all chain history was checked; describe the selected dated scan instead, or retain timestamp zero if promising a genesis scan.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
describe the selected dated scan instead
| kept = t if isinstance(b, dict) and type(t := b.get("time")) is int and t >= genesis else 0 | ||
| if start < kept + 86400 or not kept: | ||
| since = f" from before {time.strftime('%Y-%m-%d', time.gmtime(kept))}" if kept else "" |
There was a problem hiding this comment.
If the untrusted getblockstats response contains a very large integer such as 10**20 for time, it passes the current lower-bound check and reaches time.gmtime(kept), which raises OverflowError on supported platforms instead of the promised BitcoinCoreError. The CLI does not catch that exception and can terminate with a traceback during wallet initialization, while the GUI reports only its generic unexpected-failure message. Apply a sane upper bound to the retained-block timestamp, or translate failures from the date conversion into BitcoinCoreError.
AGENTS.md reference: AGENTS.md:L78-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Apply a sane upper bound to the retained-block timestamp
aea9da6 to
042fcf1
Compare
BenWestgate
changed the title
Ask for the seed's approximate first-use date on restore and reject scans a pruned or validating AssumeUTXO node cannot complete before creating or importing a wallet. Validate retained-block timestamps before date formatting and describe the actual dated scan on the finished page. Have Core persist load_on_startup at creation. Return all creation warnings for display to the operator instead of inventing a separate loading or quarantine policy. Preserve structural RPC response checks and add regressions for invalid metadata and visible warnings. Validation: Core 32 regtest with physically pruned genesis recovers a confirmed synthetic payment from the recorded date, refuses timestamp zero before mutation, and reloads restored wallets from settings.json after restart. GUI and Core boundary tests pass under Xvfb; Ruff and mypy pass. Refs BenWestgate/Bails#314
042fcf1 to
b045908
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b045908d2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| kept = b.get("time") if isinstance(b, dict) else None | ||
| if type(kept) is not int or not genesis <= kept < time.time() + 7200: | ||
| raise BitcoinCoreError("Bitcoin Core did not say which blocks it still keeps.") | ||
| if start < kept + 86400: |
There was a problem hiding this comment.
When the first retained block's raw timestamp lags a pruned predecessor's timestamp by more than roughly one day—a reproducible regtest case after a mock-time gap and valid under consensus rules—this comparison can approve a date even though Core will start the rescan in pruned history. RescanFromTime selects the first block using cumulative GetBlockTimeMax, not the prune-height block's raw time; importdescriptors can therefore fail after modifying the destination wallet, defeating this preflight. Base the gate on the same cumulative maximum or verify that Core's actual starting block is retained.
AGENTS.md reference: AGENTS.md:L78-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Base the gate on the same cumulative maximum
There was a problem hiding this comment.
Core doesn't expose the cumulative maximum over RPC. GetBlockTimeMax is only reachable through FindEarliestAtLeast, and the only RPC that calls that is pruneblockchain, which deletes blocks. getblockheader and getblockstats return only raw time and mediantime. Raw times aren't monotonic, so computing the maximum ourselves means reading every header below the prune height. That's one bitcoin-cli call per block, hundreds of thousands on a pruned mainnet node.
The exact signal we do get is Core's own per-descriptor "Rescan failed … pruning" error from importdescriptors. Today that surfaces as the generic "did not import every private descriptor". I'm proposing to keep the cheap preflight as a first filter, and to turn that error into a clear "this pruned node lacks blocks from your date" failure that unloads the wallet the restore just created. The alternatives are to keep the one-day margin as is, or to walk all the headers. I'm asking which you want in the project thread, so this reply doesn't stand in for that choice.
Generated by Claude Code
There was a problem hiding this comment.
Done in 59d7b3c, following the option you picked. The cheap preflight stays. When importdescriptors reports "Rescan failed", initialize now runs unloadwallet <name> false, so the wallet isn't loaded on startup, and raises a message saying the wallet is missing history and was closed. That replaces the generic import failure. A test covers it. The library is at 5,019 lines, inside the 5,025 budget. This is one commit on top, ready to squash into "wallet: Rescan from recorded creation date".
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59d7b3cb6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """Create one blank descriptor wallet and return Core's warnings for the operator.""" | ||
| # Let Core persist startup loading so the wallet follows new blocks. | ||
| arguments = [f"wallet_name={_wallet_name(name)}", "disable_private_keys=false", "blank=true"] | ||
| arguments.append("load_on_startup=true") |
There was a problem hiding this comment.
When restoring on a pruned node into an eligible wallet that was loaded transiently (for example, with loadwallet name false), this persistence flag never runs: _wallet_page sends existing choices directly to _import, so the dated restore can succeed but the wallet disappears after the next Core restart and can fall behind blocks that are subsequently pruned. Bitcoin Core defines false as removing the wallet from its persistent startup list in the loadwallet contract. Ensure selected existing destinations are persistent, or reject/warn for pruned restores, rather than protecting only wallets created here. This differs from the already-discussed createwallet-warning case because this path makes no createwallet call and produces no persistence warning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This isn't something this PR introduced: the existing-wallet path has never set load_on_startup. Core has no RPC that changes that flag on a wallet that's already loaded. The only way is unloadwallet followed by loadwallet name true, which would close a wallet the operator opened themselves. Bails also already reloads every wallet with load_on_startup on each Core start (Bails #315, load-wallets). I'm leaving this for Ben to decide rather than widening this PR.
Generated by Claude Code
59d7b3c to
b63b112
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b63b112ba9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b63b112 to
aaff90f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaff90fb33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Core starts a dated rescan at the first block whose highest time so far reaches the date. No RPC reports that cumulative time, so the pruning check reads the oldest kept block's own time, and a pruned block that ran more than a day ahead can still pass it. Core then imports the keys, reports "Rescan failed" for each descriptor, and leaves a wallet that is missing history. When that happens, ask Core to unload the wallet and drop it from startup, and say why instead of the generic import failure. The message tells the operator not to use the wallet and to close it whenever it is open, since the unload can fail or be undone at the next start. It advises a later date only when Core blames pruning; an aborted scan needs the same date again. The GUI's relock treats a wallet that a successful listing no longer shows as locked, so it does not hide that message, and still asks for a check if Core stops answering. Shorten the blank wallet prompts and the createwallet check to stay inside the library and GUI line budgets. Refs BenWestgate/Bails#314
Pressing Ctrl-C at the first-use date prompt of create --existing reached the generic create handler, which tells the operator to void every card from the incomplete creation. Nothing has been made at that point, and the only cards in play are the operator's existing, valid backup. Report a plain interruption there instead. Refs BenWestgate/Bails#314
aaff90f to
6c64411
Compare
This branch has not been deployed
2 participants
Restore asks for the wallet's recorded creation date and rescans from one day before it, instead of always requesting timestamp zero. A pruned node refuses an unavailable history before wallet creation/import; a node that retains the needed blocks can recover the wallet's transactions. This carries the maintainer-reviewed #139 changes.
The history remains two atomic commits:
load_on_startup=trueat wallet creation.The restore commit also fixes the final page's misleading genesis-scan claim and validates retained-block timestamps before formatting them. Core creation warnings are displayed verbatim on the finished page, as requested by the maintainer, rather than discarding them or adding a local quarantine/loading policy. Malformed RPC response shapes are still refused.
Validation at
b045908d2ab16220899022624875c08be986e076:python -O.python -O. Python 3.14 is additional host evidence, not a newly supported runtime.pruneheight=510); dated restore recovers a confirmed synthetic 1.25 BTC payment; timestamp zero is rejected before destination mutation; restart loads original/restored wallets from Core'ssettings.json, with no startup hook.Merge target is
bails-v1-pin, notmaster. Bails must repin to this reviewed GUI revision and expose its bundled blank forms through Tor Browser's permitted folder; the accompanying Bails integration PR does both. The broader library release is not a dependency.Supported-Tails desktop testing has not been performed here. Before rollout, exercise both form buttons and the dated restore on supported Tails with Core 32. This description and code were prepared with AI assistance at the maintainer's explicit request; final human review remains required.
Refs BenWestgate/Bails#314.