fix(keypairs): strip exact secp256k1 prefix, not all leading zeros (#290)#303
Open
satyakwok wants to merge 1 commit into
Open
fix(keypairs): strip exact secp256k1 prefix, not all leading zeros (#290)#303satyakwok wants to merge 1 commit into
satyakwok wants to merge 1 commit into
Conversation
…RPLF#290) `Secp256k1::sign` used `trim_start_matches(SECP256K1_PREFIX)` where `SECP256K1_PREFIX: char = '0'`. That strips *every* leading '0' character from the hex-encoded key, not just the 2-char "00" prefix the XRPL secp256k1 encoding actually uses. For a legitimate private key whose underlying 32 bytes start with 0x00 (~1-in-256 random keys; trivially constructible like scalar 1), the prefixed form is "0000…" — the over-strip removes both the prefix and the key's leading zeros, leaving a too-short string that `SecretKey::from_str` rejects. sign() then fails on a valid key. Replace `trim_start_matches` with `strip_prefix("00").unwrap_or(...)` so exactly the 2-char prefix is removed and the actual key bytes are preserved. The `SECP256K1_PREFIX: char = '0'` constant is left untouched (still `pub`) — only the consumer in `sign` changes. Adds `secp256k1_sign_handles_private_key_with_leading_zero_bytes`, which constructs the canonical 66-char form of scalar `1` ("00" + 62 zeros + "01") and verifies sign() succeeds on it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #290.
Summary
Secp256k1::signusedtrim_start_matches(SECP256K1_PREFIX)whereSECP256K1_PREFIXis defined as a singlechar'0'. That strips every leading'0'character from the hex-encoded key, not just the 2-char"00"prefix the XRPL secp256k1 encoding actually uses.For a legitimate private key whose underlying 32 bytes start with
0x00(~1-in-256 random keys; trivially constructible like scalar1), the prefixed form is"0000…"— the over-strip removes both the prefix and the key's leading zeros, leaving a too-short string thatSecretKey::from_strrejects.sign()then fails on a valid key.Replace
trim_start_matcheswithstrip_prefix("00").unwrap_or(...)so exactly the 2-char prefix is removed and the actual key bytes are preserved. TheSECP256K1_PREFIX: char = '0'public constant is left untouched (stillpub) — only the consumer insignchanges.Test
Adds
secp256k1_sign_handles_private_key_with_leading_zero_bytes, which constructs the canonical 66-char form of scalar1("00"+ 62 zeros +"01") and verifiessign()succeeds. Fails onmain(SecretKey::from_strerrors on the over-stripped string).Smoke gate
cargo build✓cargo test --lib core::keypairs::algorithms— 7 passed, 0 failed.