Skip to content

cli: Verify recovery before wallet import - #31

Closed
BenWestgate wants to merge 1 commit into
27-strong-recovery-commitmentfrom
30-verify-cli-recovery-identity
Closed

BenWestgate wants to merge 1 commit into
27-strong-recovery-commitmentfrom
30-verify-cli-recovery-identity

Conversation

@BenWestgate

@BenWestgate BenWestgate commented Sep 22, 2026 •

Copy link
Copy Markdown
Owner

Closes #30.

  • derive the recovered wallet identity before CLI wallet mutation
  • require the separately stored 256-bit recovery commitment for restore and existing-seed initialization
  • centralize commitment formatting/matching so CLI and GUI enforce one rule
  • print the recovery commitment with wallet-record details after successful initialization
  • add ms32 wallet --enroll so legacy records can obtain the commitment from the established Core wallet's public root identity without reading recovery cards or mutating Core
  • keep the BIP32 fingerprint diagnostic only; there is no fingerprint-only recovery bypass

Validation: 977 tests pass normally and under python -O; Ruff, format, mypy, and git diff --check pass.

Stacked on #29.

@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: 448d477646

ℹ️ 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/cli.py Outdated
@BenWestgate
BenWestgate force-pushed the 30-verify-cli-recovery-identity branch from 448d477 to 56c7656 Compare September 22, 2026 19:47

@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: 56c765618c

ℹ️ 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/cli.py
@BenWestgate
BenWestgate force-pushed the 30-verify-cli-recovery-identity branch from 56c7656 to a3f5765 Compare September 22, 2026 19:54
@BenWestgate
BenWestgate force-pushed the 30-verify-cli-recovery-identity branch from a3f5765 to b8748ce Compare September 22, 2026 20:09
@BenWestgate BenWestgate added the gate: adversarial review Resolve, merge, or explicitly defer before the next full adversarial review. label Sep 24, 2026
@BenWestgate

Copy link
Copy Markdown
Owner Author

cNACK separately stored 256-bit recovery commitment for restore and existing-seed initialization, unless it is printed this is too much to write, the user will make errors and it won't confirm the data anyhow.

Perhaps we should us our codex32 or bech32 checksum to checksum the fingerprint? Then at least we know if the user wrote and typed it correctly if we need to do a non-glance comparison? This seems overengineered see my comments on #29.

However we solve this, I think the GUI and CLI should do it the same way, even if it's drawing a unique shape and asking the user for an exact match (there's projects for this). Even if it's drawing a mini QR code and scanning it (preferable to writing, possibly, estimate the time comparison). If the user is already recovering by keyboard it's better to not require trust in the camera, but if they've already scanned some shares, there's nothing wrong with also including a tiny QR to encode the fingerprint on the wallet identity sheet.

I believe this PR is a duplicate of #30? Yes/No?

@BenWestgate
BenWestgate marked this pull request as draft September 24, 2026 10:49
@BenWestgate

Copy link
Copy Markdown
Owner Author

#31 is the implementation of issue #30, not a duplicate PR. The broader verify-before-mutate concern overlaps #26, but #28 covers the GUI and #31 covers the CLI. I agree with the concept NACK on a separately handwritten 256-bit commitment, so this PR is now draft pending a simpler shared design.

@BenWestgate

Copy link
Copy Markdown
Owner Author

Closing: #28 is being reworked into one library-level gate for both GUI and CLI, which also fixes #30. --enroll is no longer needed.

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

Labels

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