Repository navigation
api: Remove unused public entry points - #130
Conversation
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head security review: ACK 3133c8e96ee241ec1b3427e214a5fada5babc84f.
I reviewed both fixes and their surrounding API removals. Removing core_descriptors eliminates the mismatched-seed public path from #128 rather than trying to authenticate an API with no repository caller; the live Core initialization path still uses the root xprv and Core-native descriptor creation. Removing fresh Core Lightning generation leaves parsing, recovery, and CreationCeremony.from_secret() re-sharing of existing cl secrets covered. The CorrectionEdit change keeps observed and replacement accessible as fields while excluding both from dataclass repr, and the regression verifies restored characters are not rendered by a correction candidate.
The real Bitcoin Core fixture is green on this head. The full Python-package run is currently queued, so this review does not claim that matrix has completed. No correctness or security blocker found in the current diff. Human review/authorship and integration remain separate.
3133c8e to
7171b1b
Compare
BenWestgate
changed the title
BenWestgate
added
area: api
ChatGPT Codex Connector
|
@codex review |
![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: 7171b1b705
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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.
AI-assisted current-head review: ACK 2cbd279. #128 is fixed by removing the unauthenticated public-descriptor path entirely; the stale CL-generation docs are reconciled. Exact-head package and Core-fixture CI are green.
ae3fd14 to
71a792b
Compare
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review: ACK 71a792b for the code delta. Against current reviewability-v1, this remains the focused removal of the unused unauthenticated public-descriptor path and fresh Core Lightning generation surface; existing CL parsing/recovery/re-sharing and master_xprv remain. The stale CL-generation documentation is reconciled, and the current diff does not reintroduce the previously removed wallet path. Bitcoin Core fixture CI is green. The Python-package workflow is still running at the time of this review, so this ACK does not claim the full exact-head matrix has completed.
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71a792b188
ℹ️ 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.
Exact-head verification follow-up for 71a792b: the Python-package matrix and Bitcoin Core wallet-fixture workflow have both completed successfully. The current-head code ACK stands; no CI or review-thread blocker remains before responsible-human review.
71a792b to
0c5d3f0
Compare
|
@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 current-head review: ACK 0c5d3f0. The removal is internally consistent: unused descriptor/public-derivation and fresh CL-generation APIs leave package exports, implementation, documentation, tests, installed-wheel verification, and real-Core fixture together. Existing CL parse/recovery/re-share support remains, while Bitcoin Core owns EC-dependent descriptor derivation. Exact-head package and Core-fixture CI are green. No finding.
core_descriptors(private=False) asked a named Bitcoin Core wallet for its HD key and built public descriptors from it, but only type-checked the seed it was given, so a wallet holding a different seed returned that wallet's descriptors. Nothing calls it: ms32 wallet hands Core the root xprv with addhdkey and Core builds the descriptors itself with createwalletdescriptor, and Core's listdescriptors and exportwatchonlywallet already provide public descriptors. We are pre-1.0, so delete public API that no command or tool calls instead of maintaining it: - core_descriptors, the WalletPublicDeriver protocol, the descriptor checksum (DESCSUM) and record helpers, and the Core adapter's public_descriptors, gethdkeys and derivehdkey calls; - generate_core_lightning_secret and CreationCeremony.core_lightning, which created fresh Core Lightning secrets that no command creates, and the CL padding check only fresh CL generation reached. Parsing, recovering, deriving and re-sharing an existing cl secret are unchanged. The real-Core fixture and installed-wheel smoke test now check xprv and tprv serialization through master_xprv. The package exports 22 names. The installed package drops from 5,196 to 4,965 logical lines. Fixes #128 Refs #63 Claude-Session: https://claude.ai/code/session_01T233rKgZqE5wzDm3EVTHL1
Fresh Core Lightning generation was removed from the public API, but the architecture, capability table, identifier policy, and security model still described it as supported. Align those contracts with the remaining behavior: existing CL secrets may still be parsed, recovered, corrected, derived, and re-shared.\n\nRefs #128
Core Lightning imports a codex32 HSM secret only when its threshold is 0 and its index is S; lightningd rejects anything else with "This is only one share of codex32!". The cl profile still parsed shares and shared S strings, CreationCeremony.from_secret split CL secrets, and the generic façade recovered and derived CL shares, so every CL share this library made or accepted was unusable in Core Lightning. Reject a nonzero threshold in the cl profile, which closes CL recovery, derivation and splitting at the parse boundary. The profile hook now receives the whole header instead of only the share index. Ceremonies accept only MasterSeed, so their profile plumbing goes away. Tests that only needed a valid CL secret use the BIP-93 cl10peev vector; the CL sharing fixtures and tests are removed, and new tests cover the rejections. The docs no longer promise CL sharing. Package size goes from 4,965 to 4,959 logical lines. Claude-Session: https://claude.ai/code/session_01T233rKgZqE5wzDm3EVTHL1
The identifier policy said a random identifier does not make a weak supplied seed safe. Nothing suggests it would; the sentence only invites the question. Claude-Session: https://claude.ai/code/session_01T233rKgZqE5wzDm3EVTHL1
0c5d3f0 to
c8bedfd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8bedfdbd8
ℹ️ 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".
The v1 rewrite dropped the license headers that master carries in bech32.py and checksums.py. bech32.py still derives convertbits from Pieter Wuille's segwit_addr.py, and checksums.py keeps his polymod arithmetic and Leon Olsson Curr and Pearlwort Sneed's codex32 generators. MIT and BSD-3-Clause both require the notice to travel with the code, and CONTRIBUTING.md requires the original author's header on derived work. Restore master's headers, except that checksums.py now credits the polymod arithmetic, since it no longer holds Bech32/Bech32m constants. Each credit also names its upstream source, sipa/bech32's ref/python/segwit_addr.py or BIP-93, as CONTRIBUTING.md asks. Comments do not count toward the package line budget. Claude-Session: https://claude.ai/code/session_01T233rKgZqE5wzDm3EVTHL1
The one-shot signature in api.md omitted generate_master_seed's fingerprint argument. A fresh call that leaves identifier unset raises ValueError unless that callable is supplied, so the documented defaults could not generate a seed. Show the argument and its contract. Claude-Session: https://claude.ai/code/session_01T233rKgZqE5wzDm3EVTHL1
1 participant
core_descriptors(secret, private=False, integration=..., wallet=...)derived public descriptors from whichever seed the named Bitcoin Core wallet held while only type-checking the suppliedsecret. Nothing in the repository calls that path:ms32 walletgives Core the root xprv and Core creates the account-0 descriptors itself.Remove that unused pre-1.0 API rather than maintaining an unauthenticated seed/wallet pairing:
core_descriptors,WalletPublicDeriver, descriptor checksum/record helpers, and the Core adapter's public-derivation calls;Core Lightning secrets cannot be shared: Core Lightning imports a codex32 HSM secret only at threshold 0 and rejects anything else ("This is only one share of codex32!"). So the
clprofile now rejects a nonzero threshold, which removes CL recovery, share derivation, and splitting;CreationCeremonyaccepts onlyMasterSeed. Unsharedclsecrets can still be parsed, checked, and corrected. The CL sharing fixtures, tests, and documentation are removed, with new tests for the rejections. A focused security review found no remaining path that builds or accepts a CL share.master_xprvremains the wallet API. The real-Core fixture and installed-wheel smoke test usemaster_xprvfor xprv/tprv serialization.The correction-rendering fix previously stacked here is split into focused #132, so this PR now fixes only #128 and supersedes the narrower API cleanup in #64. The head also removes the stale CL-generation claims identified by Codex (that review thread is resolved), the stale "private descriptors contain root xprv" design row, and the identifier-policy remark about weak seeds. It is rebased onto #57 and #132; the package drops from 5,196 to 4,959 logical lines.
Fixes #63
Fixes #128
Supersedes #64
Generated with Claude Code; human review/authorship and integration remain required.