Repository navigation
wallet: Require the recorded fingerprint before import - #57
Conversation
BenWestgate
added
area: cli
![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: 5eaf535bb7
ℹ️ 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".
|
Contract clarification applied in b50321a: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6da1f2a416
ℹ️ 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 (Claude), posted at the maintainer's request. I wrote 5eaf535, so this is partly self-review.
Not ACKing 6da1f2a.
- 6da1f2a removes
identifier_origin/identifier_note, which the no-record path was built to show. If that's intended,invariants.md:16-18andmodel.md:232still promise it (agree with the Codex P2). If not, revert it. Either way #57 now differs from #28, which keeps them. - Policy: 6da1f2a is authored by
Codex Preflight <codex-preflight@localhost>, and AI_POLICY.md forbids agent authors. Its message also contains a literal\n\nand has no area prefix. 5eaf535 has aCo-Authored-By: Claudetrailer, which the same policy forbids. Squash-merge or reword. - Q:
create --existingimports an existing seed (timestamp 0) withrestore=False, so it skips the fingerprint gate. Should it passrestore=True? The Enter-if-none path keeps it usable.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review, posted at the maintainer's request.
Concept ACK 70a188a. The no-record identifier evidence is restored and matches #28 again.
One correctness item remains: ms32 create --existing supplies an existing seed but still reaches _initialize_wallet(..., restore=False), so it can import without the wallet-record gate. Treat --existing as a restore for wallet initialization.
|
Release-gate verification at current head |
|
One non-code release-gate item still remains despite the code ACK: the current PR history still contains |
|
Release-gate history check: the functional fix is ACKed at |
795ccdd to
53cd58b
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
53cd58b to
dcc0d41
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Release-gate history follow-up: the earlier commit-policy blocker is now resolved. Current head |
|
Security fix-verification refresh at current head |
dcc0d41 to
e59ac57
Compare
Gate restore and existing-seed wallet initialization on the independently recorded BIP32 master fingerprint before any Bitcoin Core wallet mutation. Keep the correction path from disclosing or reusing a fingerprint derived from the candidate being authenticated. Fixes #30.
054e8d9 to
115f2c2
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
`ms32 wallet` and `ms32 create --existing` hid the recovered master fingerprint while confirming a correction (#57), so a wrong correction was caught only after the operator accepted it and typed the record. Ask for the record first: before the shares in `ms32 wallet` and before the seed in `ms32 create --existing`. A correction that completes the secret then says whether it matches the record, without showing the fingerprint, and the record picks between equally likely corrections. The final identity check, the retry on mismatch and the Enter path for no record work as before; without a record nothing is shown until the recordless gate. Ctrl-C at the moved prompt still says the existing cards are valid. Closes #91 Claude-Session: https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head release-gate re-review: ACK 115f2c2 as the restore-authentication stack unit.
The current diff still enforces the key invariant: BitcoinCore.initialize() calls verify_identity() before wallet selection or mutation; ms32 wallet and ms32 create --existing suppress recovered-fingerprint disclosure until the independent record/no-record decision; declining the recordless path terminates the attempt. All existing inline findings are resolved. Exact-head Python-package run 658 and Bitcoin Core wallet-fixture run 32 both succeeded.
Known downstream requirement: #80 must remain in the frozen stack because it corrects the no-record wording when RIPEMD-160 is unavailable. That does not weaken this PR's verify-before-mutate behavior. No new blocker found in this current diff; human review remains required before integration.
A tester asked why the fingerprint stays off the cards, why restore asks for it, how long a string is, which share indices create uses, and whether letter case matters. Answer each in a short section. The restore answer describes the typed-fingerprint step from #57, so this sits on that branch. Closes #90 Claude-Session: https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Documentation only review for now.
Will code review after the documentation is good enough.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head review at 115f2c2: documentation cleanup remains. The guide still duplicates ms32 wallet validation with ms32 check, does not explain that an offline rescan needs pre-synchronized chain history, and still pins the maintained Core offline-signing tutorial to v32.0rc1 instead of master.
Clarify the wallet-record fingerprint wording, remove the redundant pre-check, require chain history before offline rescans, and follow the maintained Core offline-signing tutorial. Refs #57.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review, posted at the maintainer's request.
ACK de569f9. The docs-only follow-up addresses the remaining guide feedback: clearer record wording, no duplicate pre-check, explicit offline-rescan prerequisite, and the maintained Core tutorial link. Exact-head package and wallet-fixture CI pass. No new blocker found.
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
LGTM after the requested changes
Make the recovery guide explicitly identify the trusted offline signer when loading the blank descriptor wallet. Replace the hardware-wallet example in the no-record warning with a watch-only wallet, which is the intended independent fingerprint reference.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review Please review the current head only. The latest commit is a narrow follow-up to two resolved reviewer comments: it clarifies the trusted offline signer wording and replaces the hardware-wallet example in the no-record warning with a watch-only wallet. Focus on correctness/security blockers; avoid style churn. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head review at 554e3e8: no findings. The only delta since the prior ACK is recovery guidance wording: it identifies the trusted offline signer explicitly and uses a watch-only wallet/descriptor backup as the independent fingerprint reference. Exact-head Python-package and Bitcoin Core wallet-fixture workflows are green, and all inline review threads are resolved.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
good enough. We will do another pass on guide.md for the strang "before filling" and "before entering" sentences.
|
@codex review Please review current head |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
1 participant
Fixes #30. Library/CLI half of #26; GUI half is tracked separately.
This adds the restore-time wallet identity gate before Bitcoin Core mutation.
BitcoinCore.initialize()accepts an expected fingerprint and checks it before wallet selection, unlock, creation or import.ms32 walletrequires the fingerprint from the wallet record; mismatch retries without changing Core.ms32 create --existinguses the same restore gate and does not expose the recovered fingerprint through correction or rendering before the independent record/no-record decision;create --existingrecord/no-record decision earlier, immediately after parsing the existing seed and before any new share ceremony or output;ms32 createonly records the new fingerprint; it has no pre-existing wallet identity to authenticate;parse_fingerprint()accepts 8 hex digits in any case or spacing.#43 tracks checksummed wallet-record fields; #55 tracks the separately planned encrypted full-descriptor backup; #56 tracks identifier-assisted correction ranking. The current release gate is accident safety, not malicious-share-tampering resistance.
Review shape
Current head
554e3e8is the reviewed restore commit115f2c2plus two documentation-only follow-ups. The restore patch has the same stable patch-id as the previously reviewed054e8d9,37eef4d, anda7efaaereplays; the follow-ups remove the redundant pre-check, document pre-synchronized chain history for offline rescans, follow Core's maintained offline-signing tutorial, identify the trusted offline signer explicitly, and use a watch-only wallet or descriptor backup as the independent fingerprint reference.The current diff stays within the maintainer-authorized
<5200library budget. Downstream #105 removes reviewed dead paths before the later Core/recovery follow-ups are integrated.Security verification
BitcoinCore.initialize()callsverify_identity()before_select()or any wallet RPC/mutation;test_identity_mismatch_stops_before_any_wallet_callpasses and asserts the RPC call log stays empty on mismatch;test_no_record_is_the_operators_choice_and_checks_nothingpasses, preserving the explicit fallback contract;ms32 walletandms32 create --existing.Validation
Current-head GitHub Python-package run 843 and Bitcoin Core wallet-fixture run 47 both succeeded. The unchanged reviewed patch was also validated on its immediately preceding replay with 40 focused identity/fingerprint/existing/timestamp regressions, 926 tests normally and under
python -O, strict mypy, Ruff check/format, correction-constant verification, all 57 frozen differential cases, andgit diff --check.Codex reviewed the restore code at
115f2c2; its later documentation findings are addressed byde569f9and554e3e8. Codex then reviewed554e3e8with no findings. All inline threads are resolved. #80 remains required downstream because it corrects no-record wording when RIPEMD-160 is unavailable.Human review/integration order from here is #57 → #105 → #99 → #80 → #81 → #95. #42/#45/#46 and #7/#51 are already in
reviewability-v1.AI assistance was used for authorized mechanical restacks and review follow-ups. The focused source commit retains Ben Westgate as its author and still requires responsible human review before integration.