Repository navigation
share: Show the master fingerprint after the threshold - #100
BenWestgate wants to merge 9 commits into
Conversation
a7efaae to
054e8d9
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
Every correction plan returned its target set, that same set as primary, an empty reduced set, and a true timed flag. Only the targets and primary set were consumed. Derive primary from targets at the call site and remove the other fields. The search engine also accepted reduced without reading it, so remove that argument and update its test and benchmark callers. Search order and capture accounting remain unchanged. Refs #46.
The preceding all-isinstance check rejects every non-share, so the list-comprehension predicate in recovery could never discard an item. Pass the validated list directly, using a type cast to express the established invariant to mypy. Recovery still copies and validates the sequence internally. Refs #46.
Without bitcoin-cli every ms32 command said "Install a reviewed bitcoin-cli before creating a backup", even commands that create nothing, and the codex32 hint didn't say what it leaves out. Say that Bitcoin Core 32 or newer must run with RPC enabled and that an unsynced regtest or signet node is enough for practice. Then say what the command uses Core for: the master fingerprint and correction ranking for secret, share and correct (with the codex32 fallback and what it omits), or giving Core the master key for create and wallet. ms32 correct now connects before its search instead of after it, so a missing Core no longer costs up to ten seconds of discarded work. Closes #84 Claude-Session: https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
Connecting before the search made two problems easier to hit. With damaged input piped to `ms32 correct` and two Core networks running, the network prompt read the exhausted pipe forever. With stderr redirected, "Using Bitcoin Core on ..." and a blank line came before `interactive confirmation required`, which the security model says must be the only message. Ask for a network only when stdin is a terminal; a pipe now gets "More than one local Bitcoin Core network is running." Print Core's messages only when stderr is a terminal. The gate test's fake now reports like the real one, so it catches the extra output. Claude-Session: https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
`ms32 secret` shows the master fingerprint with the recovered secret, but `ms32 share` never did, so the operator couldn't compare it with the wallet record before writing a new card. Print it above the derived share on a terminal, whether every input is a share or one of them is the secret. `--plain` and redirected output stay unchanged. Closes #86 Claude-Session: https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
eb37865 to
8aaf0fa
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
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-generated review (Codex), posted at the maintainer's request.
Not ACKing a3a27b0 yet: this raises the installed-code budget from 5,200 to 5,250 without the explicit authorization AGENTS.md requires. The fingerprint-after-threshold behavior itself looks sound.
BenWestgate
added
area: cli
a3a27b0 to
8aaf0fa
Compare
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-generated current-head follow-up review (Codex), posted at the maintainer's request.
ACK 8aaf0facce2d now that the unrelated <5250 cap commit has been removed.
The change displays the master fingerprint only on the interactive ms32 share path after a threshold-capable basis is available. --plain and redirected output remain unchanged, and the derivation/recovery validation that establishes the seed is unchanged. No security or correctness blocker found in this focused diff.
The PR is stacked on #105 and preserves <5200; run final CI on the actual integrated #105 + #100 tip before freeze. Responsible-human authorship/review policy still applies.
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.
Codex current-head review at 13fef0f: no findings. Since the prior ACK, the only new commit brings in #105’s already exact-head-reviewed behavior-preserving cleanup; the fingerprint-after-threshold behavior is unchanged and remains confined to interactive output.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated current-head stack review (Codex), posted at the maintainer's request.
Code ACK 13fef0f39ec9 for the actual #105 + #100 head. #105 is now an ancestor of this branch; comparing #105 (361feb7) to the head yields only the focused cli.py and test_cli.py fingerprint-after-threshold changes. The unrelated <5250 cap change is absent.
The behavior remains scoped to interactive ms32 share: the fingerprint is available only after a threshold-capable basis is recovered, while --plain and redirected output remain unchanged.
Fresh current-head Python-package run #752 is still queued at the time of this review. Treat this as an exact-head code ACK with CI pending. Responsible-human authorship/review still applies before integration.
This comment has been minimized.
This comment has been minimized.
|
V1 disposition (Codex, 2026-10-04): defer this post-audit enhancement from the v1 candidate. The focused behavior is reviewed and the current branch is cleanly stacked after #99, but the feature is not required by any validated DeepSeek/GLM/Kimi/consolidated audit finding. Attempting to carry #100 and #101 together exposed an actual overlap conflict in the late CLI stack. Resolve that after v1 rather than expanding the frozen-candidate review scope. Keep this PR open for post-v1; its current-head code ACK remains useful. Human authorship/review is still required when integrated. |
BenWestgate
removed
the
gate: adversarial review
3847389 to
27f4cc6
Compare
This branch has not been deployed
2 participants
Requested by Ben · project thread
Problem
ms32 secretshows the recovered master fingerprint, butms32 sharedid not, so an operator deriving a new card could not compare the recovered seed with the separate wallet record before writing that card.Change
On an interactive terminal,
ms32 shareprintsMaster fingerprint: XXXXXXXXabove the derived share whether the threshold was entered as ordinary shares or includedS.--plainand redirected output stay unchanged. The fingerprint is derived only after a threshold-capable basis is present.Review stack
This post-v1 branch currently contains #99 as an ancestor after stack-maintenance PR #123. The unrelated
<5250cap commit was removed and its old tip is preserved onarchive/100-pre-restack-20261004.The focused #100 delta remains the reviewed
cli.py/test_cli.pyfingerprint-after-threshold change. It is intentionally outside the v1 candidate; rebase it onto the eventual post-v1 runtime before integration.Validation
The focused behavior has a Codex ACK. Prior validation passed 926 tests plus Ruff and strict mypy. Existing share/recovery validation still requires a threshold-capable compatible basis before a fingerprint can be recovered; redirected and
--plainoutput behavior is covered by tests.Closes #86.
AI-assisted stack maintenance and review. Responsible-human review/authorship policy still applies before integration.