Merge rust-bitcoin/rust-bitcoin#6955: key_expression: preserve master-key invariants in Xpub Arbitrary
What the developer wrote
Authored by Andrew Poelstra
Merge rust-bitcoin/rust-bitcoin#6955: key_expression: preserve master-key invariants in Xpub Arbitrary
cf90d73d43935a7efea1c92807f46f82cfde17ce key_expression: preserve master-key invariants in Xpub Arbitrary (myetcd)
Pull request description:
`Xpub::arbitrary` samples the parent fingerprint and child number independently of depth. When depth is zero, it can generate a master Xpub that is rejected by `Xpub::decode` after encoding.
Generate depth first and set the parent fingerprint and child number to zero for master keys, matching the existing `Xpriv::arbitrary` implementation. Non-master keys continue to generate both fields from the input.
Add deterministic regression tests for master and non-master Xpubs, including binary encoding/decoding round trips. The master-key regression test fails before the fix.
Validation:
- `cargo test -p bitcoin-key-expression --features arbitrary`
- `cargo test -p bitcoin-key-expression --all-features`
- `cargo test -p bitcoin-key-expression --no-default-features --features alloc,arbitrary`
- `cargo rbmt -p bitcoin-key-expression fmt --check`
- `just lint` — successful, with existing removed-lint and dependency-check warnings.
ACKs for top commit:
apoelstra:
ACK cf90d73d43935a7efea1c92807f46f82cfde17ce; successfully ran local tests
Tree-SHA512: 2df6949bce9202723545b5e767c1f0ae2fb269a3c065cfd678072c6c03aae5977f2b4d9cb0a519e9a985cef1253defa09150370a8fc742a90fa9e81b222f0c89
What changed, and why it matters
This change fixes a bug in test-only code that generates random fake Bitcoin extended public keys (xpubs). Previously, when generating a master xpub (depth 0), the code could pick random values for the parent fingerprint and child number, which violates the BIP32 rule that master keys must have those fields set to zero. That caused some generated xpubs to fail a round-trip encode/decode check. The fix makes depth-zero xpubs always use zero for those two fields, matching how private master keys were already generated. It only affects fuzzing/property tests, not normal wallet operations.
No urgent action needed for production deployments; the change is test-only. Reviewers should verify that the new regression tests pass under all feature combinations and that no other Arbitrary implementations in the crate have similar invariant issues.
Security signals we found
BIP32 master-key invariant violation in generated test data
Encode/decode round-trip failure for generated master xpubs
Fix aligns Xpub::arbitrary with existing Xpriv::arbitrary behavior
Only reachable under the arbitrary feature (fuzzing/property testing)
Evidence from the diff
The patch modifies the Arbitrary implementation for Xpub in key_expression/src/bip32.rs. Previously, Xpub::arbitrary sampled depth, parent_fingerprint, and child_number independently, so depth==0 xpubs could have non-zero parent_fingerprint or child_number. BIP32 requires these to be zero for master keys, and Xpub::decode enforces that, causing round-trip failures for generated master xpubs. The fix samples depth first, then forces parent_fingerprint = Fingerprint::default() and child_number = ChildNumber::ZERO_NORMAL when depth == 0, matching the existing Xpriv::arbitrary behavior. Two deterministic regression tests are added.
Changed components
key_expression/src/bip32.rsXpub::arbitrary implementationbitcoin-key-expression crate (arbitrary feature)Inspect captured patch +35 / −3
### key_expression/src/bip32.rs
@@ -1674,11 +1674,17 @@ impl<'a> Arbitrary<'a> for ChildNumber {
#[cfg(feature = "arbitrary")]
impl<'a> Arbitrary<'a> for Xpub {
fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> {
+ let depth = u.arbitrary()?;
+ let (parent_fingerprint, child_number) = match depth {
+ 0 => (Fingerprint::default(), ChildNumber::ZERO_NORMAL),
+ _ => (u.arbitrary()?, u.arbitrary()?),
+ };
+
Ok(Self {
network: u.arbitrary()?,
- depth: u.arbitrary()?,
- parent_fingerprint: u.arbitrary()?,
- child_number: u.arbitrary()?,
+ depth,
+ parent_fingerprint,
+ child_number,
public_key: u.arbitrary()?,
chain_code: u.arbitrary()?,
})
@@ -1714,6 +1720,32 @@ mod tests {
use super::*;
+ #[cfg(feature = "arbitrary")]
+ #[test]
+ fn arbitrary_master_xpub() {
+ let mut data = [1; 128];
+ // Use zero for both depth and network, leaving nonzero input for the other fields.
+ data[..2].fill(0);
+ let xpub = Xpub::arbitrary(&mut Unstructured::new(&data)).unwrap();
+
+ assert_eq!(xpub.depth, 0);
+ assert_eq!(xpub.parent_fingerprint, Fingerprint::default());
+ assert_eq!(xpub.child_number, ChildNumber::ZERO_NORMAL);
+ assert_eq!(Xpub::decode(&xpub.encode()).unwrap(), xpub);
+ }
+
+ #[cfg(feature = "arbitrary")]
+ #[test]
+ fn arbitrary_non_master_xpub() {
+ let data = [1; 128];
+ let xpub = Xpub::arbitrary(&mut Unstructured::new(&data)).unwrap();
+
+ assert_eq!(xpub.depth, 1);
+ assert_eq!(xpub.parent_fingerprint, Fingerprint::from_byte_array([1; 4]));
+ assert_eq!(xpub.child_number, ChildNumber::from_raw(0x0101_0101));
+ assert_eq!(Xpub::decode(&xpub.encode()).unwrap(), xpub);
+ }
+
#[test]
fn parse_derivation_path_invalid_format() {
for path in ["n/0'/0", "4/m/5", "0h/0x"] {Why this scored 25/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.