Repository navigation
bip93: Allow other human-readable parts (staging for #2040 successor) - #2
BenWestgate wants to merge 5 commits into
Conversation
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
0ec3358 to
87d096b
Compare
There was a problem hiding this comment.
This draft is good. My comments about covering low bits vs whole HRP just requires a researched response and argument for the best choice.
However dont update this PR as that would be out of scope to change the checksum limits. Do so in a separate PR if you decide to do so.
| def ms32_create_regular_checksum(data): | ||
| values = data | ||
| polymod = ms32_polymod(values + [0] * 13) ^ MS32_CONST | ||
| def codex32_create_regular_checksum(hrp, data): |
There was a problem hiding this comment.
I think the primitives should just take combined expanded messages so we dont pass hrp except to the selector.
| The share generation and secret recovery procedures below are the same for both checksum variants. | ||
|
|
||
| The functions in this section represent each codex32 string as a list of integers obtained by converting the data-part characters to their values using the bech32 character table from BIP-0173. | ||
| This representation omits the human-readable part, which is the same for the input strings and the result. |
There was a problem hiding this comment.
This seems kind of arbitrary since we dont omit the threshold and identifier, perhaps we should?
Interpolating the fixed identical characters wastes time and can only cause problems producing invalid outputs especially in the expanded HRP. Although it may be useful for advanced error correction so im fine leaving it as is.
|
|
||
| In the case that the user wishes to generate a fresh secret, the user generates random initial shares, as follows: | ||
| In the case that the user wishes to generate a fresh secret, the user generates random initial shares, as follows. | ||
| This requires an application that accepts every payload of the chosen length as a secret, as the master seed format does. |
There was a problem hiding this comment.
An application profile.
| In the case that the user wishes to generate a fresh secret, the user generates random initial shares, as follows: | ||
| In the case that the user wishes to generate a fresh secret, the user generates random initial shares, as follows. | ||
| This requires an application that accepts every payload of the chosen length as a secret, as the master seed format does. | ||
| An application that does not MAY use rejection sampling instead: retry with a fresh set of shares until the resulting secret qualifies. |
There was a problem hiding this comment.
Technically it can just retry the last initial random share. That gives better UX as they can write down and confirm each share before the next is generated, improving os csrng seeding.
| Longer strings mean more chances for transcription errors, so shorter strings are better. | ||
|
|
||
| If the prefix is damaged and a user is guessing that the data might be using this scheme, then the user can enter the available data explicitly using the suspected <code>MS1</code> prefix. | ||
| The checksum covers the expanded human-readable part, as in BIP-0173, and the expansion counts toward the checksum length limits. |
There was a problem hiding this comment.
BIP-0173 only covers the HRP low bits, while this standard covers the HRP high bits.
Although now that HRP is limited to 83 characters I am amenable to only require covering the low bits like BIP-0173. Which gives a fixed maximum string length for each checksum and is backwards compatible now.
My pubkey key expression encoding crosses checksums if key origin info is HRP prepended, although I suppose that's unavoidable as there's no maximum length of that info. But usual derivation paths fit in 83-char.
| If the prefix is damaged and a user is guessing that the data might be using this scheme, then the user can enter the available data explicitly using the suspected <code>MS1</code> prefix. | ||
| The checksum covers the expanded human-readable part, as in BIP-0173, and the expansion counts toward the checksum length limits. | ||
| Beyond those limits, some errors in the human-readable part cannot be distinguished from errors elsewhere in the string, so counting the expansion keeps the error detection guarantees for the entire string. | ||
| It also lets implementations select the checksum variant from the string alone, without knowing the application. |
There was a problem hiding this comment.
From the string shape alone.
| Beyond those limits, some errors in the human-readable part cannot be distinguished from errors elsewhere in the string, so counting the expansion keeps the error detection guarantees for the entire string. | ||
| It also lets implementations select the checksum variant from the string alone, without knowing the application. | ||
|
|
||
| If the human-readable part is damaged and a user is guessing that the data might be using this scheme, then the user can enter the available data explicitly using the suspected human-readable part. |
There was a problem hiding this comment.
Evaluate applications that accept multiple HRP in the same place whether this still leaves enough error detection guarantees to prevent crossing network or private/public boundaries.
If not we should suggest error correcting implementations use ?? for the damaged HRP so correction limits are tracked and not silently correcting user assumed errors.
To avoid making this complex we could write
a user is guessing that the data might be master seed data using this scheme, ... suspected MS1 prefix.
| * checksum: <code>jhsks4laxts8q</code> | ||
|
|
||
| The checksum covers the human-readable part. | ||
| With the human-readable part <code>ms</code>, the same header and payload form a 256-bit codex32-encoded master seed with a different checksum: <code>ms10peevst6cqh0wu7p5ssjyf4z4ez42ks9jlt3zneju9uuypr2hddak6tlqstxmpzl24l6e0d</code> |
There was a problem hiding this comment.
I dont like this explain why we need it.
| * <code>ms10fauxsXXXXXXXXXXXXXXXXXXXXXXXXXXuqxkk05lyf3x2</code> | ||
| * <code>ms10fauxsxxxxxxxxxxxxxxxxxxxxxxxxxxUQXKK05LYF3X2</code> | ||
|
|
||
| These examples have a human-readable part other than "ms" and the invalid expanded codeword lengths 94 and 95. They use the regular and long checksums, respectively. |
There was a problem hiding this comment.
Do we need 4 vectors to exercise both invalid expanded codeword lengths with both checksums?
| Versions before 0.2.1 are assigned retrospectively to significant revisions. | ||
|
|
||
| * '''0.3.0''' (2026-09-21): [https://github.com/bitcoin/bips/pull/2040 #2040] | ||
| ** Allow human-readable parts other than "ms"; the checksum covers them and counts their expansion toward its length limits. |
There was a problem hiding this comment.
Nit: This would be simpler if we just covered low bits, there would be no change to string length limits.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Specify the human-readable part as in BIP-0173 instead of requiring "ms", so other applications, such as the registered "cl", can use the codex32 format. Master seeds and their shares keep "ms". Pass the human-readable part to the checksum functions and cover its BIP-0173 expansion, which already counts toward the checksum length limits. Rename the ms32 functions and constants that now serve every human-readable part to codex32. Results for "ms" are unchanged. Require every share in a set to have the same human-readable part, state that the interpolation helpers do not check the set conditions, and limit fresh-secret generation to applications that accept every payload, with rejection sampling allowed for those that do not. Implementations should not correct the human-readable part unless its application specifies how. Update the existing invalid-vector classifications for generalized human- readable parts. Add vectors for a Core Lightning "cl" secret, an 11-character human-readable part requiring the long checksum, the 83-character maximum, the 94-95 expanded-length gap, an uppercase human- readable-part checksum, and an overlong human-readable part. Checked the checksum functions against the previous ms32 functions for "ms" across data-part lengths 0 through 1029, and checked interpolation is unchanged. Checked every new vector with the specification's Python code and python-codex32, and checked each invalid group fails for its stated reason. Link-format, README table, and whitespace checks pass. Refs: bitcoin#2040, bitcoin#2258, bitcoin#2285
e81574e to
e29a410
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
e29a410 to
80a27cf
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
State explicitly that the checksum's substitution and erasure correction guarantees apply to errors in the data part. This avoids implying the same correction guarantees for the human-readable part.
Record the human-readable part generalization as version 0.3.0. It is a backward-compatible extension, so BIP 3 calls for a minor version bump: strings with the human-readable part "ms" are unchanged. Preamble, README table, link-format, and whitespace checks pass. Refs: bitcoin#2320
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Staging PR within the fork so this can get reviewed in isolation before going to bitcoin/bips, per BIP-3's recommendation to work in public on a fork before opening against the main repository.
Base is
bip93-master-seed-refactor(bitcoin#2285's branch, assumed to merge unchanged). These 3 commits are the HRP-generalization part of the old bitcoin#2040, rewritten from scratch since that PR can no longer be reopened (its branch was rebased onto a sibling PR's head after closing, which GitHub does not allow reopening from). Diff: exactly 3 commits, nothing from bitcoin#2285 itself.@codex review