Skip to content

gui: Require the recorded fingerprint before import - #28

Draft
BenWestgate wants to merge 8 commits into
gui-reference-v1from
26-verify-recovery-before-import
Draft

BenWestgate wants to merge 8 commits into
gui-reference-v1from
26-verify-recovery-before-import

Conversation

@BenWestgate

@BenWestgate BenWestgate commented Sep 22, 2026 •

Copy link
Copy Markdown
Owner

Fixes #26.

The library and CLI half is #57, with #81 strengthening ms32 create --existing by moving its independent wallet-record/no-record decision before any new share ceremony. This older GUI branch carries copies of _bitcoin_core.py, cli.py, their tests and Core tools only because its historical gui-reference-v1 base predates that library work; review those files in #57/#81. Here, review only the GUI restore boundary.

Integration note: this branch is reviewed evidence, not the final GUI integration candidate. The clean GUI line is #65 → #66 → #77 → #78. After the library/CLI and remaining foundation/security/API work is integrated into the settled reviewability-v1, rebase that clean GUI stack once and replay/squash only this PR’s reviewed GUI restore-authentication delta onto the resulting tip. Do not preserve this branch's duplicated library snapshot or exploratory Claude co-author history. Resolve #76 from actual rendered Tails evidence, then rerun automated and manual GUI qualification before the fresh adversarial review.

Scope: this PR is the GUI’s accident-safety gate. It prevents a wrong/mixed/miscorrected but checksum-valid recovery from changing Core before the operator identifies the intended wallet. A typed 32-bit fingerprint is not presented as protection against deliberate threshold-share replacement; the seed-keyed encrypted descriptor backup in #55 is the separate first-class malicious-tampering defense.

  • Restore: asks for the fingerprint from the wallet record without showing the recovered one. A mismatch stays on the page, says Bitcoin Core wasn’t changed, and lets the user retype.
  • Fresh setup: shows the newly created seed’s fingerprint once and requires I wrote it down. It does not authenticate against a pre-existing fingerprint or descriptor because there is no pre-existing wallet identity to prove.
  • I have no wallet record: shows the recovered fingerprint, the backup identifier result and the shared warning, then Go back or Restore anyway. This is deliberately a visual accident-safety fallback/authorization, not independent authentication evidence.
  • Verify before mutation on restore: before the GUI unlocks an encrypted restore destination, it rechecks the recorded fingerprint. If restore creates a new blank Core destination, the worker rechecks recorded identity immediately before create(). Fresh setup skips that restore-only check.
  • The old manual Check again callback on this branch preserved restore mode; gui: Refresh empty wallets automatically #66 removes that manual transition entirely in the final stack and uses serialized in-page polling instead.
  • The AST boundary tests distinguish fresh setup from restore and enforce the restore-only verify-before-create path.
  • wallet: Add checksummed recovery-record evidence #43 tracks checksummed wallet-record metadata and a canonical single-sig descriptor-checksum option; wallet: Encrypted descriptor backup keyed by the seed #55 tracks the encrypted full descriptor backup.

Validation on the reviewed historical head be8c243: 987 tests pass normally and under -O; focused GUI tests, Ruff, formatting, mypy and git diff --check pass. Final qualification occurs only after replay onto #65/#66/#77/#78 and includes the supported Tails guest-resolution checks.

@BenWestgate BenWestgate left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Looks good, evaluate whether the CLI should require this also.

And whether the user should be asked to type the fingerprint and reject on mismatch (to prevent loading the wrong wallet) or merely verify the displayed recovered fingerprint against the separately stored wallet record.

Comment thread docs/security/invariants.md Outdated
Comment thread docs/security/model.md Outdated
@BenWestgate BenWestgate added the gate: adversarial review Resolve, merge, or explicitly defer before the next full adversarial review. label Sep 24, 2026
@BenWestgate

BenWestgate commented Sep 24, 2026 •

Copy link
Copy Markdown
Owner Author

Superseded: I wrote this before seeing the concept NACK on #31. The current proposal is in #27: a typed fingerprint, enforced once in BitcoinCore.initialize() for both GUI and CLI. #28 should be reworked into that change.

BitcoinCore.initialize now takes a required expected_fingerprint and
checks it before any wallet is listed, created, unlocked or imported
into, so the GUI and CLI share one gate. The operator types the value
from the wallet record; a new wallet shows it once and asks for it back.
Without a record, the backup identifier must derive from the seed
(codex32 fingerprint or legacy Bails RIPEMD-160 identifier).

Fixes #26. Fixes #30.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@BenWestgate
BenWestgate force-pushed the 26-verify-recovery-before-import branch from 074793b to 0cc5437 Compare September 24, 2026 11:51
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@BenWestgate BenWestgate changed the title gui: Verify recovery before import wallet: Require the recorded fingerprint before import Sep 24, 2026
@BenWestgate

Copy link
Copy Markdown
Owner Author

I actually think writing and typing the fingerprint can be avoided by signing and encrypting the descriptor with keys derived from the master root.

The descriptor will only decrypt and signature only validate if the correct master root was recovered. Inside that descriptor will be the descriptor checksum and the fingerprint.

This is far more elegant as we kill two birds with one stone and reduce user burden. The extra dependency is gpg which is installed on Tails and debian by default and bip85 which already has a python reference implementation of which we need a tiny fraction of.

The fingerprint or seedid identifier does no authentication unless we are dealing with shares. Tamperer will just change it to match the seed he controls.

Another time we should be asked for our encrypted descriptor or our fingerprint is when no correction is found due to lack of checksum discrimination, we can push well beyond 13 / 15 erasures when we have a known fingerprint to check against.

@BenWestgate BenWestgate added area: security Security invariants, hardening, and security-sensitive boundaries. area: wallet/core Wallet integration and Bitcoin Core boundaries. labels Sep 24, 2026
@BenWestgate

Copy link
Copy Markdown
Owner Author

@claude review

Pass the recorded fingerprint through the real Bitcoin Core regtest and smoke harnesses after initialize() made identity verification mandatory.\n\nValidation: Ruff; mypy on both tools; 7 focused identity/CLI tests; git diff --check. The branch also passed 978 normal and 978 optimized tests before this tool-only fix.\n\nrefs #26
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

Restoring without a wallet record no longer requires a seed-derived
backup identifier. The operator sees the recovered fingerprint, whether
the identifier was made from the seed (codex32 fingerprint, Bails
RIPEMD-160, or its mid-2023 SHA-256 alpha, first three characters for
Bails), and a shared warning, then chooses. Split codex32 backups have
random identifiers and were otherwise unrecoverable without a record.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 819bd468a9

ℹ️ 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".

Comment thread src/codex32_gui/wallet_setup.py
Comment thread src/codex32/_bitcoin_core.py Outdated
Use the same recovery-identity row in the Bitcoin Core controls table
as the reviewability-v1 change, and keep only the window's specifics
in the graphical section. Trim the gate's docstrings to the library
size budget.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@BenWestgate BenWestgate changed the title wallet: Require the recorded fingerprint before import gui: Require the recorded fingerprint before import Sep 24, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 64e6b96d19

ℹ️ 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".

Comment thread src/codex32_gui/pages.py

Copy link
Copy Markdown
Owner Author

Yes—the CLI should enforce the same pre-import safety boundary; stacked PR #31 does so after #29. The user should not be required to retype the 32-bit fingerprint: it is useful diagnostic metadata, but typing it adds friction without making it a strong authentication check. PR #29 instead requires the separately stored 256-bit recovery commitment before wallet mutation.

Repeat the recovery identity check immediately before GUI unlock and wallet creation so a stale or inconsistent Core response cannot mutate wallet state first. Treat unavailable RIPEMD-160 as a legacy-rule miss and keep recordless identifier guidance accurate.\n\nSecurity: enforces the verify-before-mutate wallet invariant across GUI-only passphrase and creation paths.\n\nValidation: 986 pytest tests in normal and optimized modes; focused GUI/identifier tests; Ruff; format check; mypy; git diff --check.
Keep the wallet-record fingerprint gate on ms32 wallet restores, while ms32 create only requires the operator to record the new fingerprint. Fresh creation has no pre-existing wallet identity to authenticate.\n\nValidation: 886 tests in normal and optimized modes; Ruff; format check; mypy; git diff --check.
Fresh GUI setup now records the new fingerprint without treating it as evidence for a pre-existing wallet. Restore still verifies recorded identity immediately before creating a destination wallet.\n\nValidation: 987 tests in normal and optimized modes; focused GUI tests; Ruff; format check; mypy; git diff --check.

@BenWestgate BenWestgate left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

AI-generated review (Claude), posted at the maintainer's request. I wrote 0cc5437, 819bd46 and 64e6b96, so this is partly self-review.

Concept ACK c5f8993.

  • e1180b7..c5f8993 look right: verify now runs before unlock/create in the GUI, and a missing RIPEMD-160 falls through to the SHA-256 rule.
  • Same question as #57: create --existing skips the gate (restore=False).
  • #57's 6da1f2a removed the identifier check; this PR keeps it. Pick one so the library/CLI halves match.
  • 0cc5437, 819bd46 and 64e6b96 have Co-Authored-By: Claude trailers, which AI_POLICY.md forbids. Squash-merge or reword.

Apply the restore gate before importing an existing seed, whether it
is confirmed unchanged or re-shared. Keep fresh setup as record-only
confirmation and share the library identity diagnostics with the CLI.

@BenWestgate BenWestgate left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

AI-generated review, posted at the maintainer's request.

ACK be8c243. Existing-seed creation now uses the same restore gate as wallet recovery, including after re-sharing. The shared CLI blob matches #57 and the full Python package matrix is green.

Copy link
Copy Markdown
Owner Author

Release-gate verification at current head be8c243: the GUI restore path re-verifies recorded identity before unlocking an encrypted destination and again immediately before creating a blank restore destination; the corresponding review regressions cover both mutation boundaries. Fresh setup deliberately skips restore authentication because there is no pre-existing wallet identity, and existing-seed initialization now uses the restore gate. Workflow run 36284079882 completed successfully. Review shared Core/CLI code in #57; this PR’s human review can focus on the GUI/window flow.

Copy link
Copy Markdown
Owner Author

One non-code release-gate item remains despite the current code ACK: this branch history still contains AI co-author trailers (including 0cc5437, 819bd46, and 64e6b96). docs/developer/AI_POLICY.md requires a human author and says agents must not be authors/co-authors. Before merge, rewrite/squash those commits under the responsible human author while preserving the current be8c243 tree, then rerun the package workflow on the rewritten head.

Copy link
Copy Markdown
Owner Author

Release-gate history check: the functional review is now ACKed at be8c243, but the earlier AI_POLICY merge condition still remains in the commit graph. 0cc5437, 819bd46, and 64e6b96 still carry Co-Authored-By: Claude ... trailers. Do not merge this PR with commit-preserving merge/rebase as-is. Either rewrite those commits under the responsible human author before final review, or use a maintainer-authored squash commit when the GUI branch is rebuilt on the integrated reviewability-v1 + #57 line. The latter fits the already-required GUI re-integration and avoids preserving the duplicated shared-library snapshot.

BenWestgate added a commit that referenced this pull request Sep 27, 2026
Compress the gate docstring without changing behavior. This leaves the combined #10 + #28 GUI at 1,997 non-comment lines, below the enforced 2,000-line budget.

Copy link
Copy Markdown
Owner Author

Agent release-gate review of the historical GUI evidence at exact head be8c243: the GUI-specific restore path asks for independent fingerprint evidence before wallet selection, re-verifies it immediately before unlocking an encrypted destination, and re-verifies before creating a new restore destination. The explicit no-record path discloses fingerprint/identifier evidence and requires affirmative restore authorization; fresh creation does not pretend to authenticate a nonexistent prior wallet. Focused tests cover mismatch-before-unlock, and exact-head run 36284079882 succeeded. The GUI behavior is suitable to replay, but this PR itself remains not mergeable as the final candidate: its branch duplicates library/CLI work now owned by #57 and contains historical AI co-author trailers. Replay/squash only the reviewed GUI delta onto #65 → #66 after refreshed #57 is integrated.

@BenWestgate

Copy link
Copy Markdown
Owner Author

GUI replay map for the final clean stack:

This map is derived from exact historical head be8c243 and current clean GUI tip 38373b3. The historical AI-coauthor/duplicated-library commits remain intentionally excluded. Final replay still waits for #42 → refreshed #57 → #46 → #80 → #81 and the remaining foundation work to settle, then gets one GUI qualification pass on the integrated tip.

@BenWestgate
BenWestgate marked this pull request as draft October 1, 2026 06:02

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: gui Graphical user interface behavior. area: security Security invariants, hardening, and security-sensitive boundaries. area: wallet/core Wallet integration and Bitcoin Core boundaries. enhancement New feature or request gate: adversarial review Resolve, merge, or explicitly defer before the next full adversarial review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant