Repository navigation
docs: Trim offline signing to what Core's tutorial lacks - #93
BenWestgate wants to merge 1 commit into
Conversation
BenWestgate
added
the
documentation
Claude
BenWestgate
added
gate: adversarial 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-generated release-gate review, posted at the maintainer’s request.
ACK d4641c4. I independently exercised the documented Bitcoin Core 32.0rc2 RPC sequence on regtest: exporting listdescriptors, importing the filtered descriptor array into a blank private-key-disabled wallet, comparing the next receiving address, creating a watch-only send PSBT, signing it with walletprocesspsbt, and broadcasting the resulting hex with sendrawtransaction. The descriptor QR payload was 1,976 bytes; address comparison succeeded; the watch-only send was incomplete with a PSBT; offline processing completed with hex; broadcast succeeded.
The shell boundaries are also appropriate: descriptor JSON is passed as an RPC stdin argument rather than shell-evaluated, and PSBT/raw transaction values are quoted. The existing rc1 documentation URL is a release-documentation follow-up, not a correctness blocker while stable v32.0 does not yet exist and the release monitor is active.
No correctness or security blocker found. This commit is Claude-authored, so it still needs responsible human review/rewrite as required by the project’s authorship policy before integration.
|
Is the exportwatchonlywallet too large for qr even after it is compressed? |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head re-review: ACK 6f6c86f.
I re-reviewed the four follow-ups added after the earlier d4641c4 ACK. The descriptor payload is compressed well under the QR limit; the receive path now verifies each online-generated address against the locked offline signer and gives out the checked address directly from the offline screen where possible; Tails/Bails-compatible python3 replaces the unavailable jq; and the signed-transaction path fails closed when the PSBT is incomplete. The guide keeps only public descriptors, PSBTs, checked addresses and signed transactions crossing the QR boundary. There are no inline review threads, and exact-head Python-package run 694 succeeded.
No correctness or security blocker found. These Claude-authored documentation commits still require responsible-human rewrite/squash before integration.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
cNACK
I don't know if this project really needs to tell users how to do offline signing?
At best reduce this to only what info isn't already in the offline-signing-tutorial
At worst remove it and send this work downstream to github.com/BenWestgate/Bails
BenWestgate
changed the title
|
AI-assisted response: addressed at current head |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head review at 2035cb7: no findings. The maintainer cNACK is reflected in the final docs-only diff: QR/offline-signing mechanics are removed, Core’s maintained tutorial owns that workflow, and codex32 documents only its setup boundary.
![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: 2035cb7fc1
ℹ️ 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".
Preserve the requested Tails Cloner setup and permanently offline signer while identifying its boot USB separately from the transfer medium used by the maintained Bitcoin Core tutorial. This removes the ambiguity that would prevent watch-only and PSBT transfer. Checked the Tails backup instructions and Bitcoin Core offline-signing tutorial; git diff --check passes. AI-assisted response to PR #93 review feedback; refs #92 and #111.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13089b3f40
ℹ️ 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".
Stop Bitcoin Core before copying persistent wallet databases and unlock the cloned Persistent Storage before starting the offline Tails session. These are the source shutdown and target boot prerequisites from the maintained CipherStick/Tails procedures. Address the two current PR #93 correctness findings. Documentation-only; git diff --check passes. AI-assisted follow-up requested by the maintainer; refs #92.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be0056d625
ℹ️ 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".
Define CipherStick as a Tails USB with Core, codex32, and Persistent Storage already set up. Make clear that cloning reuses that prepared setup instead of introducing an unexplained prerequisite for every Tails reader. Documentation-only follow-up to PR #93 review; git diff --check passes. AI assistance used for the maintainer-requested change; refs #92.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbf4daa976
ℹ️ 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".
Retain the safety instruction to inspect destinations, amounts, and fees on the offline signer before signing. The maintained Core tutorial supplies the mechanics but does not explicitly direct that comparison; its online host must not be trusted to summarize a payment. This does not restore the bespoke QR workflow rejected by the maintainer. Checked the upstream tutorial and the documentation-only diff; no application behavior changes. AI-assisted review follow-up; refs #92. Review-comment: #93 (comment)
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head follow-up review: ACK 2fa7d8ac1ad28282da50ce6705856ea1288e934b.
The maintainer cNACK is now reflected in the final scope: Bitcoin Core's maintained tutorial owns watch-only setup and PSBT mechanics; codex32 keeps only its Tails/offline setup boundary and one safety instruction the current upstream tutorial does not state explicitly—verify every destination, amount, and fee on the offline signer before signing. The boot USB and PSBT/public-wallet transfer medium are clearly separated.
Current-head Python-package CI completed successfully. No remaining correctness or security blocker found in this documentation diff. Human review/authorship and signed integration remain.
2fa7d8a to
e3a4b41
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.
Codex current-head review: content ACK e3a4b4102abdd5e3185d0720c501c82301a7379c.
The human-authored rewrite preserves the reviewed final scope in one documentation commit: it defines the prepared Tails/offline-signer boot USB, requires Core shutdown before cloning Persistent Storage, unlocks storage with Networking Disabled, keeps the signer and boot USB offline, separates transfer media, delegates watch-only/PSBT mechanics to Bitcoin Core's maintained tutorial, and retains the offline destination/amount/fee verification step. The rejected bespoke QR workflow remains removed.
No correctness or security blocker found in the current diff. The exact-head Python-package run is presently queued in the repository-wide Actions backlog, so this review does not claim that run is complete.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
just one section to mostly revert and cherry pick small improvement and this is GTG
|
cNACK the commit description is going to go too far for this library. It's very salvagable, just one paragraph mostly reverted except my cherry picks. Then it'll be great. |
|
@codex review Please review current head |
|
Addressed the maintainer cNACK in |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review Please review exact current head |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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.
Codex current-head review at 1dcee71: one finding.
Require the signer to have the chain history needed for a rescan before it is air-gapped, and avoid a redundant standalone card check before ms32 wallet performs recovery validation. Refs #93.
This comment has been minimized.
This comment has been minimized.
|
@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-assisted current-head review, posted at the maintainer's request.
ACK ba3f00a. The only new delta clarifies the offline rescan prerequisite and removes the redundant standalone card check. Exact-head package CI passes. No new blocker found.
ba3f00a to
23d909f
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. |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review: ACK 23d909f. The offline-signing guidance stays deliberately narrow: prepare any chain history needed for an existing-wallet rescan before permanent air-gapping, then keep every network path disconnected while recovery/signing material is present; delegate watch-only/PSBT mechanics to Bitcoin Core's maintained tutorial; keep the boot medium distinct from transfer media; and verify decoded destinations, amounts, and fees on the offline signer. The removed bespoke QR instructions reduce stale/security-sensitive duplication. Exact-head Python-package CI is green and all review threads are resolved.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
looks good besides this. Perhaps we should mention what bails just did about making a stateless wallet that's forgotten on shutdown? If you think that is in scope, I don't think it works the same way on windows or every linux distro this software runs on.
066d00f to
1c05006
Compare
The offline-signing section pointed at Bitcoin Core's tutorial but still referred to "the QR tools below" and kept a QR troubleshooting section, with no step that used them. Carrying descriptors, PSBTs and addresses by QR is a Tails wallet workflow, so it now lives in Bails (Bails#312, Bails#320) instead of this guide. Keep only what Core's maintained tutorial does not already say for a codex32 user: create an empty encrypted descriptor wallet with private keys enabled on the offline signer and run ms32 wallet; keep that computer permanently disconnected from every network as soon as Bitcoin Core and codex32 are installed; and do not use the offline boot USB stick as the transfer drive. Signing a PSBT needs no rescan, so the signer needs no chain history. Link the tutorial on master in both places so the link follows Core's maintained copy. Keep one safety step the tutorial does not state: verify every destination, amount and fee on the offline signer against the intended payment before signing, and do not trust the online host's summary. Removing the QR steps also removes their jq dependency, which Tails does not ship. Fixes #92 Closes #111 Claude-Session: https://claude.ai/code/session_01T233rKgZqE5wzDm3EVTHL1
1c05006 to
53a82b3
Compare
This branch has not been deployed
1 participant
Keep the offline-signing section narrow: create/restore the signer on a permanently disconnected computer, delegate the watch-only/PSBT workflow to Bitcoin Core's maintained tutorial on
master, and retain the codex32-specific instruction to verify every destination, amount, and fee on the offline signer before signing. An offline bootable USB stick is explicitly not the transfer drive.Current head
ba3f00akeeps the narrow1dcee71handoff and adds the latest review fix: an offline signer that may rescan an existing wallet must already have the required chain history before it is permanently air-gapped. The recovery steps also avoid a redundant standalonems32 check. All inline review threads are resolved. The prior Codex review covered1dcee71; the exact-head re-review request forba3f00awas refused by the current GitHub Codex code-review quota.Exact-head Python-package run 825 succeeds across the full current matrix. The immediately preceding human-authored head
e3a4b410also completed exact-head run 786 successfully. The final follow-up commit is explicitly agent-authored and unsigned; responsible-human review/rewrite-squash and signed integration remain required.Closes #92. Closes #111.