Repository navigation
gui: Prepare records and restore from their date - #113
BenWestgate wants to merge 2 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
This branch has not been deployed
1 participant
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.