From 0e5e021b690c5dccea18b44e33137c888164023b Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 21 Apr 2025 22:19:30 +0000 Subject: [PATCH 1/7] bip32: change several cryptographically unreachable paths to expects These paths cannot be reached. In general, key derivation cannot fail for cryptographic reasons. --- bitcoin/src/bip32.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index b335b09b3..b1bf0bc18 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -610,7 +610,8 @@ impl Xpriv { child_number: ChildNumber::ZERO_NORMAL, private_key: secp256k1::SecretKey::from_byte_array( hmac.as_byte_array().split_array::<32, 32>().0, - )?, + ) + .expect("cryptographically unreachable"), chain_code: ChainCode::from_hmac(hmac), }) } @@ -828,7 +829,8 @@ impl Xpub { let hmac = engine.finalize(); let private_key = secp256k1::SecretKey::from_byte_array( hmac.as_byte_array().split_array::<32, 32>().0, - )?; + ) + .expect("cryptographically unreachable"); let chain_code = ChainCode::from_hmac(hmac); Ok((private_key, chain_code)) } @@ -842,7 +844,8 @@ impl Xpub { i: ChildNumber, ) -> Result { let (sk, chain_code) = self.ckd_pub_tweak(i)?; - let tweaked = self.public_key.add_exp_tweak(secp, &sk.into())?; + let tweaked = + self.public_key.add_exp_tweak(secp, &sk.into()).expect("cryptographically unreachable"); Ok(Xpub { network: self.network, From 32d96f6c337290c2624886bd5cc30a7264c361d1 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 21 Apr 2025 22:33:28 +0000 Subject: [PATCH 2/7] bip32: make Xpriv::new_master be infallible The only error path for this is cryptographically unreachable and was removed in a previous commit. --- bitcoin/examples/bip32.rs | 2 +- bitcoin/src/bip32.rs | 8 ++++---- bitcoin/src/psbt/mod.rs | 2 +- bitcoin/tests/bip_174.rs | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/bitcoin/examples/bip32.rs b/bitcoin/examples/bip32.rs index 9df5ef673..6cc56697e 100644 --- a/bitcoin/examples/bip32.rs +++ b/bitcoin/examples/bip32.rs @@ -33,7 +33,7 @@ fn main() { let secp = Secp256k1::preallocated_new(buf.as_mut_slice()).unwrap(); // calculate root key from seed - let root = Xpriv::new_master(NetworkKind::Main, &seed).unwrap(); + let root = Xpriv::new_master(NetworkKind::Main, &seed); println!("Root key: {}", root); // derive child xpub diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index b1bf0bc18..b19c55c98 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -598,12 +598,12 @@ impl From for Error { impl Xpriv { /// Constructs a new master key from a seed value - pub fn new_master(network: impl Into, seed: &[u8]) -> Result { + pub fn new_master(network: impl Into, seed: &[u8]) -> Xpriv { let mut engine = HmacEngine::::new(b"Bitcoin seed"); engine.input(seed); let hmac = engine.finalize(); - Ok(Xpriv { + Xpriv { network: network.into(), depth: 0, parent_fingerprint: Default::default(), @@ -613,7 +613,7 @@ impl Xpriv { ) .expect("cryptographically unreachable"), chain_code: ChainCode::from_hmac(hmac), - }) + } } /// Constructs a new ECDSA compressed private key matching internal secret key representation. @@ -1111,7 +1111,7 @@ mod tests { expected_sk: &str, expected_pk: &str, ) { - let mut sk = Xpriv::new_master(network, seed).unwrap(); + let mut sk = Xpriv::new_master(network, seed); let mut pk = Xpub::from_xpriv(secp, &sk); // Check derivation convenience method for Xpriv diff --git a/bitcoin/src/psbt/mod.rs b/bitcoin/src/psbt/mod.rs index 6f0ce3fc4..d648dfac3 100644 --- a/bitcoin/src/psbt/mod.rs +++ b/bitcoin/src/psbt/mod.rs @@ -1439,7 +1439,7 @@ mod tests { let mut hd_keypaths: BTreeMap = Default::default(); - let mut sk: Xpriv = Xpriv::new_master(NetworkKind::Main, &seed).unwrap(); + let mut sk: Xpriv = Xpriv::new_master(NetworkKind::Main, &seed); let fprint = sk.fingerprint(secp); diff --git a/bitcoin/tests/bip_174.rs b/bitcoin/tests/bip_174.rs index 495658658..c9ecef166 100644 --- a/bitcoin/tests/bip_174.rs +++ b/bitcoin/tests/bip_174.rs @@ -120,7 +120,7 @@ fn build_extended_private_key() -> Xpriv { let xpriv = extended_private_key.parse::().unwrap(); let sk = PrivateKey::from_wif(seed).unwrap(); - let seeded = Xpriv::new_master(NetworkKind::Test, &sk.inner.secret_bytes()).unwrap(); + let seeded = Xpriv::new_master(NetworkKind::Test, &sk.inner.secret_bytes()); assert_eq!(xpriv, seeded); xpriv From f0a237c001da87348a7a36dc1ddd2b307d6c6b5f Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 21 Apr 2025 22:25:26 +0000 Subject: [PATCH 3/7] bip32: split out DerivationError from the main error enum This is a breaking change to the API of the deprecated `derive_pub` function, but it's in a way that's unlikely to break consumers (who are probably just .expect'ing the result) so we will retain the deprecated function. --- bitcoin/src/bip32.rs | 54 +++++++++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index b19c55c98..57d77973b 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -497,8 +497,6 @@ pub type KeySource = (Fingerprint, DerivationPath); #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub enum Error { - /// A pk->pk derivation was attempted on a hardened key - CannotDeriveFromHardenedKey, /// A secp256k1 error occurred Secp256k1(secp256k1::Error), /// A child number was provided that was out of range @@ -536,8 +534,6 @@ impl fmt::Display for Error { use Error::*; match *self { - CannotDeriveFromHardenedKey => - f.write_str("cannot derive hardened key from public key"), Secp256k1(ref e) => write_err!(f, "secp256k1 error"; e), InvalidChildNumber(ref n) => write!(f, "child number {} is invalid (not within [0, 2^31 - 1])", n), @@ -570,8 +566,7 @@ impl std::error::Error for Error { Base58(ref e) => Some(e), Hex(ref e) => Some(e), InvalidBase58PayloadLength(ref e) => Some(e), - CannotDeriveFromHardenedKey - | InvalidChildNumber(_) + InvalidChildNumber(_) | InvalidChildNumberFormat | InvalidDerivationPathFormat | UnknownVersion(_) @@ -596,6 +591,33 @@ impl From for Error { fn from(e: InvalidBase58PayloadLengthError) -> Error { Self::InvalidBase58PayloadLength(e) } } +/// A BIP32 error +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] +pub enum DerivationError { + /// Attempted to derive a hardened child from an xpub. + /// + /// You can only derive hardened children from xprivs. + CannotDeriveHardenedChild, + /// Attempted to derive a child of depth 256 or higher. + /// + /// There is no way to encode such xkeys. + MaximumDepthExceeded, +} + +#[cfg(feature = "std")] +impl std::error::Error for DerivationError {} + +impl fmt::Display for DerivationError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match *self { + Self::CannotDeriveHardenedChild => + f.write_str("cannot derive hardened child of public key"), + Self::MaximumDepthExceeded => f.write_str("cannot derive child of depth 256 or higher"), + } + } +} + impl Xpriv { /// Constructs a new master key from a seed value pub fn new_master(network: impl Into, seed: &[u8]) -> Xpriv { @@ -795,7 +817,7 @@ impl Xpub { &self, secp: &Secp256k1, path: &P, - ) -> Result { + ) -> Result { self.derive_xpub(secp, path) } @@ -806,7 +828,7 @@ impl Xpub { &self, secp: &Secp256k1, path: &P, - ) -> Result { + ) -> Result { let mut pk: Xpub = *self; for cnum in path.as_ref() { pk = pk.ckd_pub(secp, *cnum)? @@ -818,9 +840,9 @@ impl Xpub { pub fn ckd_pub_tweak( &self, i: ChildNumber, - ) -> Result<(secp256k1::SecretKey, ChainCode), Error> { + ) -> Result<(secp256k1::SecretKey, ChainCode), DerivationError> { match i { - ChildNumber::Hardened { .. } => Err(Error::CannotDeriveFromHardenedKey), + ChildNumber::Hardened { .. } => Err(DerivationError::CannotDeriveHardenedChild), ChildNumber::Normal { index: n } => { let mut engine = HmacEngine::::new(&self.chain_code[..]); engine.input(&self.public_key.serialize()[..]); @@ -842,7 +864,7 @@ impl Xpub { &self, secp: &Secp256k1, i: ChildNumber, - ) -> Result { + ) -> Result { let (sk, chain_code) = self.ckd_pub_tweak(i)?; let tweaked = self.public_key.add_exp_tweak(secp, &sk.into()).expect("cryptographically unreachable"); @@ -1120,7 +1142,10 @@ mod tests { // Check derivation convenience method for Xpub, should error // appropriately if any ChildNumber is hardened if path.0.iter().any(|cnum| cnum.is_hardened()) { - assert_eq!(pk.derive_xpub(secp, &path), Err(Error::CannotDeriveFromHardenedKey)); + assert_eq!( + pk.derive_xpub(secp, &path), + Err(DerivationError::CannotDeriveHardenedChild) + ); } else { assert_eq!(&pk.derive_xpub(secp, &path).unwrap().to_string()[..], expected_pk); } @@ -1135,7 +1160,10 @@ mod tests { assert_eq!(pk, pk2); } Hardened { .. } => { - assert_eq!(pk.ckd_pub(secp, num), Err(Error::CannotDeriveFromHardenedKey)); + assert_eq!( + pk.ckd_pub(secp, num), + Err(DerivationError::CannotDeriveHardenedChild) + ); pk = Xpub::from_xpriv(secp, &sk); } } From a891fb9b748064531729e310786f37cc9fce4444 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 21 Apr 2025 22:30:35 +0000 Subject: [PATCH 4/7] bip32: remove unused error variants --- bitcoin/src/bip32.rs | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 57d77973b..5dd747fbf 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -503,18 +503,12 @@ pub enum Error { InvalidChildNumber(u32), /// Invalid childnumber format. InvalidChildNumberFormat, - /// Invalid derivation path format. - InvalidDerivationPathFormat, /// Unknown version magic bytes UnknownVersion([u8; 4]), /// Encoded extended key data has wrong length WrongExtendedKeyLength(usize), /// Base58 encoding error Base58(base58::Error), - /// Hexadecimal decoding error - Hex(hex::HexToArrayError), - /// `PublicKey` hex should be 66 or 130 digits long. - InvalidPublicKeyHexLength(usize), /// Base58 decoded data was an invalid length. InvalidBase58PayloadLength(InvalidBase58PayloadLengthError), /// Invalid private key prefix (byte 45 must be 0) @@ -538,14 +532,10 @@ impl fmt::Display for Error { InvalidChildNumber(ref n) => write!(f, "child number {} is invalid (not within [0, 2^31 - 1])", n), InvalidChildNumberFormat => f.write_str("invalid child number format"), - InvalidDerivationPathFormat => f.write_str("invalid derivation path format"), UnknownVersion(ref bytes) => write!(f, "unknown version magic bytes: {:?}", bytes), WrongExtendedKeyLength(ref len) => write!(f, "encoded extended key data has wrong length {}", len), Base58(ref e) => write_err!(f, "base58 encoding error"; e), - Hex(ref e) => write_err!(f, "Hexadecimal decoding error"; e), - InvalidPublicKeyHexLength(got) => - write!(f, "PublicKey hex should be 66 or 130 digits long, got: {}", got), InvalidBase58PayloadLength(ref e) => write_err!(f, "base58 payload"; e), InvalidPrivateKeyPrefix => f.write_str("invalid private key prefix, byte 45 must be 0 as required by BIP-32"), @@ -564,14 +554,11 @@ impl std::error::Error for Error { match *self { Secp256k1(ref e) => Some(e), Base58(ref e) => Some(e), - Hex(ref e) => Some(e), InvalidBase58PayloadLength(ref e) => Some(e), InvalidChildNumber(_) | InvalidChildNumberFormat - | InvalidDerivationPathFormat | UnknownVersion(_) - | WrongExtendedKeyLength(_) - | InvalidPublicKeyHexLength(_) => None, + | WrongExtendedKeyLength(_) => None, InvalidPrivateKeyPrefix => None, NonZeroParentFingerprintForMasterKey => None, NonZeroChildNumberForMasterKey => None, From a66ad97fb6bcafcc5a9cd8cea5e722716dcca55a Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 21 Apr 2025 22:48:50 +0000 Subject: [PATCH 5/7] bip32: split InvalidChildNumber and InvalidChildNumberFormat out of error Currently in this module we have a distinction between an "index" which is a number between [0, 2^31 - 1] which indexes into the set of normal or hardened child numbers. We also have a child *number*, which within the library is an opaque type, but can be freely converted into/out of a u32 in the consensus encoding which uses the MSB as a normal/hardened flag and the other bits to encode the index. Probably we want to change ChildNumber::From to some sort of from_consensus_u32 method, but that's out of scope for this PR. What is *in scope*, though is fixing the error types. In the existing code we have three problems: * Our error type for a bad index is called InvalidChildNumber, rather than InvalidIndex, and the error message reflects this, which is wrong and confusing. (Some with InvalidChildNumberFormat.) * The InvalidChildNumberFormat is always constructed from a ParseIntError from stdlib, but we always throw that away rather than preserving it. * These two error variants only appear when parsing child numbers, or derivation paths which are lists of child numbers, but they are part of the main error enum. --- bitcoin/src/bip32.rs | 132 +++++++++++++++++++++++++++++++------------ 1 file changed, 97 insertions(+), 35 deletions(-) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 5dd747fbf..1a32428f4 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -148,11 +148,11 @@ impl ChildNumber { /// [0, 2^31 - 1]. /// /// [`Normal`]: #variant.Normal - pub fn from_normal_idx(index: u32) -> Result { + pub fn from_normal_idx(index: u32) -> Result { if index & (1 << 31) == 0 { Ok(ChildNumber::Normal { index }) } else { - Err(Error::InvalidChildNumber(index)) + Err(IndexOutOfRangeError { index }) } } @@ -160,11 +160,11 @@ impl ChildNumber { /// [0, 2^31 - 1]. /// /// [`Hardened`]: #variant.Hardened - pub fn from_hardened_idx(index: u32) -> Result { + pub fn from_hardened_idx(index: u32) -> Result { if index & (1 << 31) == 0 { Ok(ChildNumber::Hardened { index }) } else { - Err(Error::InvalidChildNumber(index)) + Err(IndexOutOfRangeError { index }) } } @@ -184,7 +184,7 @@ impl ChildNumber { } /// Returns the child number that is a single increment from this one. - pub fn increment(self) -> Result { + pub fn increment(self) -> Result { match self { ChildNumber::Normal { index: idx } => ChildNumber::from_normal_idx(idx + 1), ChildNumber::Hardened { index: idx } => ChildNumber::from_hardened_idx(idx + 1), @@ -225,16 +225,18 @@ impl fmt::Display for ChildNumber { } impl FromStr for ChildNumber { - type Err = Error; + type Err = ParseChildNumberError; - fn from_str(inp: &str) -> Result { + fn from_str(inp: &str) -> Result { let is_hardened = inp.chars().last().map_or(false, |l| l == '\'' || l == 'h'); Ok(if is_hardened { ChildNumber::from_hardened_idx( - inp[0..inp.len() - 1].parse().map_err(|_| Error::InvalidChildNumberFormat)?, - )? + inp[0..inp.len() - 1].parse().map_err(ParseChildNumberError::ParseInt)?, + ) + .map_err(ParseChildNumberError::IndexOutOfRange)? } else { - ChildNumber::from_normal_idx(inp.parse().map_err(|_| Error::InvalidChildNumberFormat)?)? + ChildNumber::from_normal_idx(inp.parse().map_err(ParseChildNumberError::ParseInt)?) + .map_err(ParseChildNumberError::IndexOutOfRange)? }) } } @@ -267,7 +269,7 @@ impl serde::Serialize for ChildNumber { /// derivation path pub trait IntoDerivationPath { /// Converts a given type into a [`DerivationPath`] with possible error - fn into_derivation_path(self) -> Result; + fn into_derivation_path(self) -> Result; } /// A BIP-32 derivation path. @@ -295,15 +297,17 @@ impl IntoDerivationPath for T where T: Into, { - fn into_derivation_path(self) -> Result { Ok(self.into()) } + fn into_derivation_path(self) -> Result { + Ok(self.into()) + } } impl IntoDerivationPath for String { - fn into_derivation_path(self) -> Result { self.parse() } + fn into_derivation_path(self) -> Result { self.parse() } } impl IntoDerivationPath for &'_ str { - fn into_derivation_path(self) -> Result { self.parse() } + fn into_derivation_path(self) -> Result { self.parse() } } impl From> for DerivationPath { @@ -338,9 +342,9 @@ impl AsRef<[ChildNumber]> for DerivationPath { } impl FromStr for DerivationPath { - type Err = Error; + type Err = ParseChildNumberError; - fn from_str(path: &str) -> Result { + fn from_str(path: &str) -> Result { if path.is_empty() || path == "m" || path == "m/" { return Ok(vec![].into()); } @@ -348,7 +352,7 @@ impl FromStr for DerivationPath { let path = path.strip_prefix("m/").unwrap_or(path); let parts = path.split('/'); - let ret: Result, Error> = parts.map(str::parse).collect(); + let ret: Result, _> = parts.map(str::parse).collect(); Ok(DerivationPath(ret?)) } } @@ -499,10 +503,6 @@ pub type KeySource = (Fingerprint, DerivationPath); pub enum Error { /// A secp256k1 error occurred Secp256k1(secp256k1::Error), - /// A child number was provided that was out of range - InvalidChildNumber(u32), - /// Invalid childnumber format. - InvalidChildNumberFormat, /// Unknown version magic bytes UnknownVersion([u8; 4]), /// Encoded extended key data has wrong length @@ -529,9 +529,6 @@ impl fmt::Display for Error { match *self { Secp256k1(ref e) => write_err!(f, "secp256k1 error"; e), - InvalidChildNumber(ref n) => - write!(f, "child number {} is invalid (not within [0, 2^31 - 1])", n), - InvalidChildNumberFormat => f.write_str("invalid child number format"), UnknownVersion(ref bytes) => write!(f, "unknown version magic bytes: {:?}", bytes), WrongExtendedKeyLength(ref len) => write!(f, "encoded extended key data has wrong length {}", len), @@ -555,10 +552,7 @@ impl std::error::Error for Error { Secp256k1(ref e) => Some(e), Base58(ref e) => Some(e), InvalidBase58PayloadLength(ref e) => Some(e), - InvalidChildNumber(_) - | InvalidChildNumberFormat - | UnknownVersion(_) - | WrongExtendedKeyLength(_) => None, + UnknownVersion(_) | WrongExtendedKeyLength(_) => None, InvalidPrivateKeyPrefix => None, NonZeroParentFingerprintForMasterKey => None, NonZeroChildNumberForMasterKey => None, @@ -605,6 +599,62 @@ impl fmt::Display for DerivationError { } } +/// Out-of-range index when constructing a child number. +/// +/// *Indices* are always in the range [0, 2^31 - 1]. Normal child numbers have the +/// same range, while hardened child numbers lie in the range [2^31, 2^32 - 1]. +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] +pub struct IndexOutOfRangeError { + /// The index that was out of range for a child number. + pub index: u32, +} + +#[cfg(feature = "std")] +impl std::error::Error for IndexOutOfRangeError {} + +impl From for IndexOutOfRangeError { + fn from(never: Infallible) -> Self { match never {} } +} + +impl fmt::Display for IndexOutOfRangeError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "index {} out of range [0, 2^31 - 1] (do you have an hardened child number, rather than an index?)", self.index) + } +} + +/// Error parsing a child number. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ParseChildNumberError { + /// Parsed the child number as an integer, but the integer was out of range. + IndexOutOfRange(IndexOutOfRangeError), + /// Failed to parse the child number as an integer. + ParseInt(core::num::ParseIntError), +} + +impl From for ParseChildNumberError { + fn from(never: Infallible) -> Self { match never {} } +} + +#[cfg(feature = "std")] +impl std::error::Error for ParseChildNumberError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match *self { + Self::IndexOutOfRange(ref e) => Some(e), + Self::ParseInt(ref e) => Some(e), + } + } +} + +impl fmt::Display for ParseChildNumberError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match *self { + Self::IndexOutOfRange(ref e) => e.fmt(f), + Self::ParseInt(ref e) => e.fmt(f), + } + } +} + impl Xpriv { /// Constructs a new master key from a seed value pub fn new_master(network: impl Into, seed: &[u8]) -> Xpriv { @@ -1040,13 +1090,25 @@ mod tests { #[test] fn parse_derivation_path() { - assert_eq!("n/0'/0".parse::(), Err(Error::InvalidChildNumberFormat)); - assert_eq!("4/m/5".parse::(), Err(Error::InvalidChildNumberFormat)); - assert_eq!("//3/0'".parse::(), Err(Error::InvalidChildNumberFormat)); - assert_eq!("0h/0x".parse::(), Err(Error::InvalidChildNumberFormat)); + assert!(matches!( + "n/0'/0".parse::(), + Err(ParseChildNumberError::ParseInt(..)), + )); + assert!(matches!( + "4/m/5".parse::(), + Err(ParseChildNumberError::ParseInt(..)), + )); + assert!(matches!( + "//3/0'".parse::(), + Err(ParseChildNumberError::ParseInt(..)), + )); + assert!(matches!( + "0h/0x".parse::(), + Err(ParseChildNumberError::ParseInt(..)), + )); assert_eq!( "2147483648".parse::(), - Err(Error::InvalidChildNumber(2147483648)) + Err(ParseChildNumberError::IndexOutOfRange(IndexOutOfRangeError { index: 2147483648 })), ); assert_eq!(DerivationPath::master(), "".parse::().unwrap()); @@ -1176,9 +1238,9 @@ mod tests { let max = (1 << 31) - 1; let cn = ChildNumber::from_normal_idx(max).unwrap(); - assert_eq!(cn.increment().err(), Some(Error::InvalidChildNumber(1 << 31))); + assert_eq!(cn.increment(), Err(IndexOutOfRangeError { index: 1 << 31 }),); let cn = ChildNumber::from_hardened_idx(max).unwrap(); - assert_eq!(cn.increment().err(), Some(Error::InvalidChildNumber(1 << 31))); + assert_eq!(cn.increment(), Err(IndexOutOfRangeError { index: 1 << 31 }),); let cn = ChildNumber::from_normal_idx(350).unwrap(); let path = "42'".parse::().unwrap(); From 73781e047be48003fdc3f269b09add0d06b1dd4b Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 22 Apr 2025 14:04:14 +0000 Subject: [PATCH 6/7] bip32: rename Error to ParseError The bip32::Error enum is now exclusively used for errors related to parsing and decoding. It is still a little messy (mainly: it contains a base58 variant which is used when parsing a string but not when decoding from bytes) but much cleaner than it was. --- bitcoin/src/bip32.rs | 58 +++++++++++++++++++++-------------------- bitcoin/src/psbt/mod.rs | 6 ++--- 2 files changed, 33 insertions(+), 31 deletions(-) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 1a32428f4..735734499 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -500,7 +500,7 @@ pub type KeySource = (Fingerprint, DerivationPath); /// A BIP32 error #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] -pub enum Error { +pub enum ParseError { /// A secp256k1 error occurred Secp256k1(secp256k1::Error), /// Unknown version magic bytes @@ -519,13 +519,13 @@ pub enum Error { NonZeroChildNumberForMasterKey, } -impl From for Error { +impl From for ParseError { fn from(never: Infallible) -> Self { match never {} } } -impl fmt::Display for Error { +impl fmt::Display for ParseError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - use Error::*; + use ParseError::*; match *self { Secp256k1(ref e) => write_err!(f, "secp256k1 error"; e), @@ -544,9 +544,9 @@ impl fmt::Display for Error { } #[cfg(feature = "std")] -impl std::error::Error for Error { +impl std::error::Error for ParseError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - use Error::*; + use ParseError::*; match *self { Secp256k1(ref e) => Some(e), @@ -560,16 +560,18 @@ impl std::error::Error for Error { } } -impl From for Error { - fn from(e: secp256k1::Error) -> Error { Error::Secp256k1(e) } +impl From for ParseError { + fn from(e: secp256k1::Error) -> ParseError { ParseError::Secp256k1(e) } } -impl From for Error { - fn from(err: base58::Error) -> Self { Error::Base58(err) } +impl From for ParseError { + fn from(err: base58::Error) -> Self { ParseError::Base58(err) } } -impl From for Error { - fn from(e: InvalidBase58PayloadLengthError) -> Error { Self::InvalidBase58PayloadLength(e) } +impl From for ParseError { + fn from(e: InvalidBase58PayloadLengthError) -> ParseError { + Self::InvalidBase58PayloadLength(e) + } } /// A BIP32 error @@ -759,19 +761,19 @@ impl Xpriv { } /// Decoding extended private key from binary data according to BIP 32 - pub fn decode(data: &[u8]) -> Result { + pub fn decode(data: &[u8]) -> Result { let Common { network, depth, parent_fingerprint, child_number, chain_code, key } = Common::decode(data)?; let network = match network { VERSION_BYTES_MAINNET_PRIVATE => NetworkKind::Main, VERSION_BYTES_TESTNETS_PRIVATE => NetworkKind::Test, - unknown => return Err(Error::UnknownVersion(unknown)), + unknown => return Err(ParseError::UnknownVersion(unknown)), }; let (&zero, private_key) = key.split_first(); if zero != 0 { - return Err(Error::InvalidPrivateKeyPrefix); + return Err(ParseError::InvalidPrivateKeyPrefix); } Ok(Xpriv { @@ -917,14 +919,14 @@ impl Xpub { } /// Decoding extended public key from binary data according to BIP 32 - pub fn decode(data: &[u8]) -> Result { + pub fn decode(data: &[u8]) -> Result { let Common { network, depth, parent_fingerprint, child_number, chain_code, key } = Common::decode(data)?; let network = match network { VERSION_BYTES_MAINNET_PUBLIC => NetworkKind::Main, VERSION_BYTES_TESTNETS_PUBLIC => NetworkKind::Test, - unknown => return Err(Error::UnknownVersion(unknown)), + unknown => return Err(ParseError::UnknownVersion(unknown)), }; Ok(Xpub { @@ -970,9 +972,9 @@ impl fmt::Display for Xpriv { } impl FromStr for Xpriv { - type Err = Error; + type Err = ParseError; - fn from_str(inp: &str) -> Result { + fn from_str(inp: &str) -> Result { let data = base58::decode_check(inp)?; if data.len() != 78 { @@ -990,9 +992,9 @@ impl fmt::Display for Xpub { } impl FromStr for Xpub { - type Err = Error; + type Err = ParseError; - fn from_str(inp: &str) -> Result { + fn from_str(inp: &str) -> Result { let data = base58::decode_check(inp)?; if data.len() != 78 { @@ -1048,9 +1050,9 @@ struct Common { } impl Common { - fn decode(data: &[u8]) -> Result { + fn decode(data: &[u8]) -> Result { let data: &[u8; 78] = - data.try_into().map_err(|_| Error::WrongExtendedKeyLength(data.len()))?; + data.try_into().map_err(|_| ParseError::WrongExtendedKeyLength(data.len()))?; let (&network, data) = data.split_array::<4, 74>(); let (&depth, data) = data.split_first::<73>(); @@ -1060,11 +1062,11 @@ impl Common { if depth == 0 { if parent_fingerprint != [0u8; 4] { - return Err(Error::NonZeroParentFingerprintForMasterKey); + return Err(ParseError::NonZeroParentFingerprintForMasterKey); } if child_number != [0u8; 4] { - return Err(Error::NonZeroChildNumberForMasterKey); + return Err(ParseError::NonZeroChildNumberForMasterKey); } } @@ -1370,7 +1372,7 @@ mod tests { assert!(result.is_err()); match result { - Err(Error::InvalidPrivateKeyPrefix) => {} + Err(ParseError::InvalidPrivateKeyPrefix) => {} _ => panic!("Expected InvalidPrivateKeyPrefix error, got {:?}", result), } } @@ -1381,7 +1383,7 @@ mod tests { assert!(result.is_err()); match result { - Err(Error::NonZeroChildNumberForMasterKey) => {} + Err(ParseError::NonZeroChildNumberForMasterKey) => {} _ => panic!("Expected NonZeroChildNumberForMasterKey error, got {:?}", result), } } @@ -1392,7 +1394,7 @@ mod tests { assert!(result.is_err()); match result { - Err(Error::NonZeroParentFingerprintForMasterKey) => {} + Err(ParseError::NonZeroParentFingerprintForMasterKey) => {} _ => panic!("Expected NonZeroParentFingerprintForMasterKey error, got {:?}", result), } } diff --git a/bitcoin/src/psbt/mod.rs b/bitcoin/src/psbt/mod.rs index d648dfac3..955af4372 100644 --- a/bitcoin/src/psbt/mod.rs +++ b/bitcoin/src/psbt/mod.rs @@ -916,7 +916,7 @@ impl_get_key_for_xonly_map!(HashMap); #[non_exhaustive] pub enum GetKeyError { /// A bip32 error. - Bip32(bip32::Error), + Bip32(bip32::ParseError), /// The GetKey operation is not supported for this key request. NotSupported, } @@ -949,8 +949,8 @@ impl std::error::Error for GetKeyError { } } -impl From for GetKeyError { - fn from(e: bip32::Error) -> Self { GetKeyError::Bip32(e) } +impl From for GetKeyError { + fn from(e: bip32::ParseError) -> Self { GetKeyError::Bip32(e) } } /// The various output types supported by the Bitcoin network. From b9a12043b0a93a2fe4ac79eeda65f85c8fafc302 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 22 Apr 2025 14:43:39 +0000 Subject: [PATCH 7/7] bip32: return error when trying to derive a path too deep This restores the Result return from derive_xpriv which we had removed in a previous PR, but such is life. Fixes #4308 --- bitcoin/examples/bip32.rs | 2 +- bitcoin/examples/ecdsa-psbt-simple.rs | 10 +++++---- bitcoin/examples/ecdsa-psbt.rs | 5 +++-- bitcoin/examples/taproot-psbt-simple.rs | 10 +++++---- bitcoin/examples/taproot-psbt.rs | 18 ++++++++++----- bitcoin/src/bip32.rs | 29 +++++++++++++++---------- bitcoin/src/psbt/mod.rs | 16 +++++--------- bitcoin/tests/bip_174.rs | 3 ++- 8 files changed, 55 insertions(+), 38 deletions(-) diff --git a/bitcoin/examples/bip32.rs b/bitcoin/examples/bip32.rs index 6cc56697e..7cde6c646 100644 --- a/bitcoin/examples/bip32.rs +++ b/bitcoin/examples/bip32.rs @@ -38,7 +38,7 @@ fn main() { // derive child xpub let path = "84h/0h/0h".parse::().unwrap(); - let child = root.derive_xpriv(&secp, &path); + let child = root.derive_xpriv(&secp, &path).expect("only deriving three steps"); println!("Child at {}: {}", path, child); let xpub = Xpub::from_xpriv(&secp, &child); println!("Public key at {}: {}", path, xpub); diff --git a/bitcoin/examples/ecdsa-psbt-simple.rs b/bitcoin/examples/ecdsa-psbt-simple.rs index ac5d84b43..21b9f6cad 100644 --- a/bitcoin/examples/ecdsa-psbt-simple.rs +++ b/bitcoin/examples/ecdsa-psbt-simple.rs @@ -62,11 +62,12 @@ fn get_external_address_xpriv( ) -> Xpriv { let derivation_path = BIP84_DERIVATION_PATH.into_derivation_path().expect("valid derivation path"); - let child_xpriv = master_xpriv.derive_xpriv(secp, &derivation_path); + let child_xpriv = + master_xpriv.derive_xpriv(secp, &derivation_path).expect("only deriving three steps"); let external_index = ChildNumber::ZERO_NORMAL; let idx = ChildNumber::from_normal_idx(index).expect("valid index number"); - child_xpriv.derive_xpriv(secp, &[external_index, idx]) + child_xpriv.derive_xpriv(secp, &[external_index, idx]).expect("only deriving two more steps") } // Derive the internal address xpriv. @@ -77,11 +78,12 @@ fn get_internal_address_xpriv( ) -> Xpriv { let derivation_path = BIP84_DERIVATION_PATH.into_derivation_path().expect("valid derivation path"); - let child_xpriv = master_xpriv.derive_xpriv(secp, &derivation_path); + let child_xpriv = + master_xpriv.derive_xpriv(secp, &derivation_path).expect("only deriving three steps"); let internal_index = ChildNumber::ONE_NORMAL; let idx = ChildNumber::from_normal_idx(index).expect("valid index number"); - child_xpriv.derive_xpriv(secp, &[internal_index, idx]) + child_xpriv.derive_xpriv(secp, &[internal_index, idx]).expect("only deriving two more steps") } // The address to send to. diff --git a/bitcoin/examples/ecdsa-psbt.rs b/bitcoin/examples/ecdsa-psbt.rs index e6bbaf1e6..376604911 100644 --- a/bitcoin/examples/ecdsa-psbt.rs +++ b/bitcoin/examples/ecdsa-psbt.rs @@ -118,11 +118,12 @@ impl ColdStorage { // Hardened children require secret data to derive. let path = "84h/0h/0h".into_derivation_path()?; - let account_0_xpriv = master_xpriv.derive_xpriv(secp, &path); + let account_0_xpriv = + master_xpriv.derive_xpriv(secp, &path).expect("derivation path is short"); let account_0_xpub = Xpub::from_xpriv(secp, &account_0_xpriv); let path = INPUT_UTXO_DERIVATION_PATH.into_derivation_path()?; - let input_xpriv = master_xpriv.derive_xpriv(secp, &path); + let input_xpriv = master_xpriv.derive_xpriv(secp, &path).expect("derivation path is short"); let input_xpub = Xpub::from_xpriv(secp, &input_xpriv); let wallet = ColdStorage { master_xpriv, master_xpub }; diff --git a/bitcoin/examples/taproot-psbt-simple.rs b/bitcoin/examples/taproot-psbt-simple.rs index 8181573a2..d2ca8f9e2 100644 --- a/bitcoin/examples/taproot-psbt-simple.rs +++ b/bitcoin/examples/taproot-psbt-simple.rs @@ -60,11 +60,12 @@ fn get_external_address_xpriv( ) -> Xpriv { let derivation_path = BIP86_DERIVATION_PATH.into_derivation_path().expect("valid derivation path"); - let child_xpriv = master_xpriv.derive_xpriv(secp, &derivation_path); + let child_xpriv = + master_xpriv.derive_xpriv(secp, &derivation_path).expect("only deriving three steps"); let external_index = ChildNumber::ZERO_NORMAL; let idx = ChildNumber::from_normal_idx(index).expect("valid index number"); - child_xpriv.derive_xpriv(secp, &[external_index, idx]) + child_xpriv.derive_xpriv(secp, &[external_index, idx]).expect("only deriving two more steps") } // Derive the internal address xpriv. @@ -75,11 +76,12 @@ fn get_internal_address_xpriv( ) -> Xpriv { let derivation_path = BIP86_DERIVATION_PATH.into_derivation_path().expect("valid derivation path"); - let child_xpriv = master_xpriv.derive_xpriv(secp, &derivation_path); + let child_xpriv = + master_xpriv.derive_xpriv(secp, &derivation_path).expect("only deriving three steps"); let internal_index = ChildNumber::ONE_NORMAL; let idx = ChildNumber::from_normal_idx(index).expect("valid index number"); - child_xpriv.derive_xpriv(secp, &[internal_index, idx]) + child_xpriv.derive_xpriv(secp, &[internal_index, idx]).expect("only deriving two more steps") } // Get the Taproot Key Origin. diff --git a/bitcoin/examples/taproot-psbt.rs b/bitcoin/examples/taproot-psbt.rs index 92af85156..d2914bd7f 100644 --- a/bitcoin/examples/taproot-psbt.rs +++ b/bitcoin/examples/taproot-psbt.rs @@ -299,7 +299,7 @@ fn generate_bip86_key_spend_tx( .ok_or("missing Taproot key origin")?; let secret_key = - master_xpriv.derive_xpriv(secp, &derivation_path).to_private_key().inner; + master_xpriv.derive_xpriv(secp, &derivation_path)?.to_private_key().inner; sign_psbt_taproot( secret_key, input.tap_internal_key.unwrap(), @@ -393,8 +393,11 @@ impl BenefactorWallet { // We use some other derivation path in this example for our inheritance protocol. The important thing is to ensure // that we use an unhardened path so we can make use of xpubs. let derivation_path = format!("101/1/0/0/{}", self.next).parse::()?; - let internal_keypair = - self.master_xpriv.derive_xpriv(&self.secp, &derivation_path).to_keypair(&self.secp); + let internal_keypair = self + .master_xpriv + .derive_xpriv(&self.secp, &derivation_path) + .expect("derivation path is short") + .to_keypair(&self.secp); let beneficiary_key = self.beneficiary_xpub.derive_xpub(&self.secp, &derivation_path)?.to_x_only_public_key(); @@ -486,6 +489,7 @@ impl BenefactorWallet { let new_internal_keypair = self .master_xpriv .derive_xpriv(&self.secp, &new_derivation_path) + .expect("derivation path is short") .to_keypair(&self.secp); let beneficiary_key = self .beneficiary_xpub @@ -538,6 +542,7 @@ impl BenefactorWallet { let secret_key = self .master_xpriv .derive_xpriv(&self.secp, &derivation_path) + .expect("derivation path is short") .to_private_key() .inner; sign_psbt_taproot( @@ -660,8 +665,11 @@ impl BeneficiaryWallet { for (x_only_pubkey, (leaf_hashes, (_, derivation_path))) in &psbt.inputs[0].tap_key_origins.clone() { - let secret_key = - self.master_xpriv.derive_xpriv(&self.secp, &derivation_path).to_private_key().inner; + let secret_key = self + .master_xpriv + .derive_xpriv(&self.secp, &derivation_path)? + .to_private_key() + .inner; for lh in leaf_hashes { let sighash_type = TapSighashType::All; let hash = SighashCache::new(&unsigned_tx).taproot_script_spend_signature_hash( diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 735734499..e94c32e2a 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -185,6 +185,9 @@ impl ChildNumber { /// Returns the child number that is a single increment from this one. pub fn increment(self) -> Result { + // Bare addition in this function is okay, because we have an invariant that + // `index` is always within [0, 2^31 - 1]. FIXME this is not actually an + // invariant because the fields are public. match self { ChildNumber::Normal { index: idx } => ChildNumber::from_normal_idx(idx + 1), ChildNumber::Hardened { index: idx } => ChildNumber::from_hardened_idx(idx + 1), @@ -706,7 +709,7 @@ impl Xpriv { &self, secp: &Secp256k1, path: &P, - ) -> Xpriv { + ) -> Result { self.derive_xpriv(secp, path) } @@ -717,16 +720,20 @@ impl Xpriv { &self, secp: &Secp256k1, path: &P, - ) -> Xpriv { + ) -> Result { let mut sk: Xpriv = *self; for cnum in path.as_ref() { - sk = sk.ckd_priv(secp, *cnum) + sk = sk.ckd_priv(secp, *cnum)?; } - sk + Ok(sk) } /// Private->Private child key derivation - fn ckd_priv(&self, secp: &Secp256k1, i: ChildNumber) -> Xpriv { + fn ckd_priv( + &self, + secp: &Secp256k1, + i: ChildNumber, + ) -> Result { let mut engine = HmacEngine::::new(&self.chain_code[..]); match i { ChildNumber::Normal { .. } => { @@ -750,14 +757,14 @@ impl Xpriv { let tweaked = sk.add_tweak(&self.private_key.into()).expect("statistically impossible to hit"); - Xpriv { + Ok(Xpriv { network: self.network, - depth: self.depth + 1, + depth: self.depth.checked_add(1).ok_or(DerivationError::MaximumDepthExceeded)?, parent_fingerprint: self.fingerprint(secp), child_number: i, private_key: tweaked, chain_code: ChainCode::from_hmac(hmac), - } + }) } /// Decoding extended private key from binary data according to BIP 32 @@ -910,7 +917,7 @@ impl Xpub { Ok(Xpub { network: self.network, - depth: self.depth + 1, + depth: self.depth.checked_add(1).ok_or(DerivationError::MaximumDepthExceeded)?, parent_fingerprint: self.fingerprint(), child_number: i, public_key: tweaked, @@ -1188,7 +1195,7 @@ mod tests { let mut pk = Xpub::from_xpriv(secp, &sk); // Check derivation convenience method for Xpriv - assert_eq!(&sk.derive_xpriv(secp, &path).to_string()[..], expected_sk); + assert_eq!(&sk.derive_xpriv(secp, &path).unwrap().to_string()[..], expected_sk); // Check derivation convenience method for Xpub, should error // appropriately if any ChildNumber is hardened @@ -1203,7 +1210,7 @@ mod tests { // Derive keys, checking hardened and non-hardened derivation one-by-one for &num in path.0.iter() { - sk = sk.ckd_priv(secp, num); + sk = sk.ckd_priv(secp, num).unwrap(); match num { Normal { .. } => { let pk2 = pk.ckd_pub(secp, num).unwrap(); diff --git a/bitcoin/src/psbt/mod.rs b/bitcoin/src/psbt/mod.rs index 955af4372..3cdb9b32b 100644 --- a/bitcoin/src/psbt/mod.rs +++ b/bitcoin/src/psbt/mod.rs @@ -775,14 +775,14 @@ impl GetKey for Xpriv { KeyRequest::XOnlyPubkey(_) => Err(GetKeyError::NotSupported), KeyRequest::Bip32((fingerprint, path)) => { let key = if self.fingerprint(secp) == *fingerprint { - let k = self.derive_xpriv(secp, &path); + let k = self.derive_xpriv(secp, &path).map_err(GetKeyError::Bip32)?; Some(k.to_private_key()) } else if self.parent_fingerprint == *fingerprint && !path.is_empty() && path[0] == self.child_number { let path = DerivationPath::from_iter(path.into_iter().skip(1).copied()); - let k = self.derive_xpriv(secp, &path); + let k = self.derive_xpriv(secp, &path).map_err(GetKeyError::Bip32)?; Some(k.to_private_key()) } else { None @@ -915,8 +915,8 @@ impl_get_key_for_xonly_map!(HashMap); #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub enum GetKeyError { - /// A bip32 error. - Bip32(bip32::ParseError), + /// A bip32 derivation error. + Bip32(bip32::DerivationError), /// The GetKey operation is not supported for this key request. NotSupported, } @@ -930,7 +930,7 @@ impl fmt::Display for GetKeyError { use GetKeyError::*; match *self { - Bip32(ref e) => write_err!(f, "a bip23 error"; e), + Bip32(ref e) => write_err!(f, "bip32 derivation"; e), NotSupported => f.write_str("the GetKey operation is not supported for this key request"), } @@ -949,10 +949,6 @@ impl std::error::Error for GetKeyError { } } -impl From for GetKeyError { - fn from(e: bip32::ParseError) -> Self { GetKeyError::Bip32(e) } -} - /// The various output types supported by the Bitcoin network. #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] #[non_exhaustive] @@ -1454,7 +1450,7 @@ mod tests { ChildNumber::from_normal_idx(31337).unwrap(), ]; - sk = sk.derive_xpriv(secp, &dpath); + sk = sk.derive_xpriv(secp, &dpath).unwrap(); let pk = Xpub::from_xpriv(secp, &sk); diff --git a/bitcoin/tests/bip_174.rs b/bitcoin/tests/bip_174.rs index c9ecef166..90f4f2118 100644 --- a/bitcoin/tests/bip_174.rs +++ b/bitcoin/tests/bip_174.rs @@ -315,7 +315,8 @@ fn parse_and_verify_keys( let path = derivation_path.into_derivation_path().expect("failed to convert derivation path"); - let derived_priv = ext_priv.derive_xpriv(secp, &path).to_private_key(); + let derived_priv = + ext_priv.derive_xpriv(secp, &path).expect("derivation path too long").to_private_key(); assert_eq!(wif_priv, derived_priv); let derived_pub = derived_priv.public_key(secp); key_map.insert(derived_pub, derived_priv);