Repository navigation
api: Remove unused public entry points - #130
BenWestgate wants to merge 2 commits into
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".
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
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 |
|
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 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.
This branch has not been deployed
2 participants
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;Parsing, recovery, derivation, and re-sharing of existing
clsecrets remain supported.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) and the stale "private descriptors contain root xprv" design row. It is rebased onto #57 and #132; the package drops from 5,196 to 4,965 logical lines.
Fixes #63
Fixes #128
Supersedes #64
Generated with Claude Code; human review/authorship and integration remain required.