generation: Validate roots before re-sharing - #126
BenWestgate wants to merge 2 commits into
Conversation
A parsed Bitcoin secret validates the codex32 format but need not form a valid BIP32 root. Reject that state at the public from_secret ceremony boundary before identifier randomness or recovery-card output, using the existing stdlib-only validator. The supplied-byte path was already covered by PR #16. Retain format parsing and Core Lightning behavior. The header-collision comparison is kept on one formatted line in the same function; its behavior is unchanged and the package remains at 5,198 logical review lines. The 69 focused creation/API tests pass normally and with optimization; Ruff, strict mypy, build/twine, and whitespace checks pass. The full normal suite passes 953 tests; the optimized full suite is running. AI-assisted fix requested by the maintainer; fixes #125.
BenWestgate
added
bug
![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: f47cb9721a
ℹ️ 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".
Clarify that a format-valid parsed MasterSeed does not by itself establish the BIP32-root precondition for creation. Document the from_secret check before identifier selection or entropy and distinguish Core Lightning. Matches the already-tested behavior of f47cb97; no runtime change. AI-assisted response to the PR #126 review; refs #125.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head follow-up review: ACK 8cd584e02c33aad0043762215def5452a9c00f1d.
The runtime check is at the correct public creation boundary: an existing MasterSeed must form a valid BIP32 root before threshold/header processing can select a random identifier or emit shares, while Core Lightning remains unaffected. The regression proves the invalid Bitcoin path stops before entropy selection. The follow-up documentation accurately distinguishes format-valid parsing from the creation precondition.
GitHub's current-head Python-package run completed successfully. No correctness or security blocker found. Human review and signed integration remain.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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 8cd584e: no findings. Existing Bitcoin MasterSeed inputs now fail BIP32-root validation before identifier selection or entropy; Core Lightning remains intentionally exempt.
|
@codex review Please review current head |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This branch has not been deployed
1 participant
CreationCeremony.from_secret()could emit Bitcoin recovery cards withoutchecking whether the encoded seed formed a valid BIP32 root. #16 fixed supplied
raw bytes but left this existing-secret path from Kimi F7 / consolidated M8.
Check the root before identifier randomness or share generation. Keep codex32
parsing as format validation and preserve Core Lightning behavior. The regression
forces the rare invalid-root outcome and proves it stops before any entropy call.
The same-function header comparison is compacted without changing its behavior;
the installed package remains below
<5200at 5,198 logical lines.Validation: 69 focused tests and all 953 tests pass both normally and under
python -O. Ruff, strict mypy,build/twine, and
git diff --checkpass; the BSD notice is present in both builtartifacts. Review after #53, whose API changes are the base of this focused diff.
Fixes #125. Refs #20, #16. AI assistance was used for this maintainer-requested
change. Human review and signed integration remain with the maintainer.