Repository navigation
python-codex32 reference impl - #74
BenWestgate wants to merge 4 commits into
Conversation
From: https://github.com/BenWestgate/python-codex32 Published on PyPI: https://pypi.org/project/codex32/
|
I vote to merge this with a cursory review; later when I improve rust-codex32, I will steal all the test vectors from this implementation, and presumably if they pass on both implementations, they're both correct :P. cursory review ACK 4857e18 -- checked that this is shaped like it does codex32 stuff, that there's nothing obviously malicious, no filesystem or network access, etc. cc @roconnor-blockstream can you let me know if I should hold off on merging this? Fine to ping me out-of-band. |
|
I nACK 4857e18 because it allows encoding shares from padded bytes. When the padding is not uniform, independent random it can leak up to Comment explaining the problem: 4857e18#r198130738 The rust impl allows this too, and an issue was opened where a user of our libraries was confused he could not recover the same secret bytes from derived share bytes. 
The best solution is refuse to encode or decode shares from/to bytes. Especially now that bitcoin/bips#2258 defines "S" decoding to be application specific. I have some BIP0173 style test vectors in the bitcoin/bips#2040 (comment) that are worth stealing as well. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Rewrite the BIP93 encoding, checksum, sharing and profile layers of the v0.6.1 code (byte-identical to 4857e18, ACKed in BlockstreamResearch/codex32#74) as frozen, typed values with no third-party dependency. - gf32/checksums/bech32: immutable checksum specs, GF(32) tables and strict single-case Bech32 parsing; drop the unused segwit_addr module. - bip93: Header/Share/Secret values, parse_codex32, derive_share and recover_secret. There is no public encoder from raw (padded) bytes to a share; shares come only from interpolation. - profiles: ms (BIP32 master seed), cl (Core Lightning hsm_secret, unshared only) and bip39 entropy. - Tooling: drop mypy.ini, requirements.txt and the pylint job; add ruff, strict mypy and a hash-pinned setuptools build dependency. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
Rewrite the BIP93 encoding, checksum, sharing and profile layers of the v0.6.1 code (byte-identical to 4857e18, ACKed in BlockstreamResearch/codex32#74) as frozen, typed values with no third-party dependency. - gf32/checksums/bech32: immutable checksum specs, GF(32) tables and strict single-case Bech32 parsing; drop the unused segwit_addr module. - bip93: Header/Share/Secret values, parse_codex32, derive_share and recover_secret. There is no public encoder from raw (padded) bytes to a share; shares come only from interpolation. - profiles: ms (BIP32 master seed), cl (Core Lightning hsm_secret, unshared only) and bip39 entropy. - Tooling: drop mypy.ini, requirements.txt and the pylint job; add ruff, strict mypy and a hash-pinned setuptools build dependency. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
Rewrite the BIP93 encoding, checksum, sharing and profile layers of the v0.6.1 code (byte-identical to 4857e18, ACKed in BlockstreamResearch/codex32#74) as frozen, typed values with no third-party dependency. - gf32/checksums/bech32: immutable checksum specs, GF(32) tables and strict single-case Bech32 parsing; drop the unused segwit_addr module. - bip93: Header/Share/Secret values, parse_codex32, derive_share and recover_secret. There is no public encoder from raw (padded) bytes to a share; shares come only from interpolation. - profiles: ms (BIP32 master seed), cl (Core Lightning hsm_secret, unshared only) and bip39 entropy. - Tooling: drop mypy.ini, requirements.txt and the pylint job; add ruff, strict mypy and a hash-pinned setuptools build dependency. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
|
I have done a cursory review of 4857e18. Want me to merge it? |
Shares are now made only by interpolation. encode() and from_seed() reject share indices other than 's', and decode() returns no seed bytes or padding for shares. Codex32String keeps the string it was given instead of re-encoding from bytes, so attributes are no longer writable aliases. Drop code a reference implementation should not carry: - the bip32 dependency and the fingerprint default identifier (BIP-93 leaves the identifier to the user; coincurve also fails to build on Python 3.13) - segwit_addr.py, Bech32/Bech32m constants and their tests - unused HRP_CODES, IDX_ORDER and IdNotLength4 Claude-Session: https://claude.ai/code/session_01PYQtm2qPkBYqZ9WuNvhv51
- "ms" strings must have an encoded length of 48, 54, 61, 67, 74 or 127, and master seeds must be 16, 20, 24, 28, 32 or 64 bytes. - Select the checksum by expanded codeword length (HRP expansion plus data part): regular up to 93, long from 96 to 1023; 94 and 95 are invalid. - Limit the HRP to 83 characters. - Add Test vectors 6-8 from BIP-93 master and 9-11 plus the new invalid vectors from bips#2320. Drop the long-HRP boundary vectors that encoded the old data-length checksum selection. Claude-Session: https://claude.ai/code/session_01PYQtm2qPkBYqZ9WuNvhv51
|
I updated this PR to resolve my NACK and keep it matching BIP93 master and bitcoin/bips#2320.
Less than 45 lines added so this is a quick cursory re-review. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
codex32_decode now checks only what every codex32 string has: the checksum and the header (digit threshold, share index "s" when k=0). ms32_verify adds the BIP-93 "ms" length rule, and Codex32String uses it for "ms" strings, so generic strings with other HRPs no longer need byte-aligned payloads. encode() and decode() only handle codex32-encoded master seeds, so they drop their hrp argument. Codex32String stores the HRP, header and payload characters and gets bytes from data(), which only works for "ms" secrets. The seed-size check and InvalidSeedLength are gone since the "ms" lengths already imply the seed sizes. Claude-Session: https://claude.ai/code/session_01PYQtm2qPkBYqZ9WuNvhv51
|
Generalizing HRP needed one more change: It is now correct and up-to-date with master and open BIP93 PRs. |
Rewrite the BIP93 encoding, checksum, sharing and profile layers of the v0.6.1 code (byte-identical to 4857e18, ACKed in BlockstreamResearch/codex32#74) as frozen, typed values with no third-party dependency. - gf32/checksums/bech32: immutable checksum specs, GF(32) tables and strict single-case Bech32 parsing; drop the unused segwit_addr module. - bip93: Header/Share/Secret values, parse_codex32, derive_share and recover_secret. There is no public encoder from raw (padded) bytes to a share; shares come only from interpolation. - profiles: ms (BIP32 master seed), cl (Core Lightning hsm_secret, unshared only) and bip39 entropy. - Tooling: drop mypy.ini, requirements.txt and the pylint job; add ruff, strict mypy and a hash-pinned setuptools build dependency. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
apoelstra/rust-codex32#2 (comment):
I have wrote this python implementation that covers more tests than the rust one does. It also does the approach above and relies on general checksum and bech32 modules. So it also passes Bech32/Bech32m tests.
It includes new test vectors for bitcoin/bips#2040 and bitcoin/bips#2077.
From: https://github.com/BenWestgate/python-codex32
Published on PyPI: https://pypi.org/project/codex32/