From af61d7e7bc5630bdb5b607331e39db108fe0a51c Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Tue, 10 Nov 2020 09:34:29 +0100 Subject: [PATCH 1/8] PSBT: adding global types (version, xpub) --- src/util/bip32.rs | 4 +-- src/util/psbt/map/global.rs | 67 +++++++++++++++++++++++++++++++++++-- src/util/psbt/mod.rs | 8 +++++ 3 files changed, 75 insertions(+), 4 deletions(-) diff --git a/src/util/bip32.rs b/src/util/bip32.rs index 5611d78d..1dcabe42 100644 --- a/src/util/bip32.rs +++ b/src/util/bip32.rs @@ -390,7 +390,7 @@ pub enum Error { InvalidDerivationPathFormat, /// Unknown version magic bytes UnknownVersion([u8; 4]), - /// Encoded extended key data have wrong length + /// Encoded extended key data has wrong length WrongExtendedKeyLength(usize), /// Base58 encoding error Base58(base58::Error) @@ -406,7 +406,7 @@ impl fmt::Display for Error { Error::InvalidChildNumberFormat => f.write_str("invalid child number format"), Error::InvalidDerivationPathFormat => f.write_str("invalid derivation path format"), Error::UnknownVersion(ref bytes) => write!(f, "unknown version magic bytes: {:?}", bytes), - Error::WrongExtendedKeyLength(ref len) => write!(f, "encoded extended key data have wrong length {}", len), + Error::WrongExtendedKeyLength(ref len) => write!(f, "encoded extended key data has wrong length {}", len), Error::Base58(ref err) => write!(f, "base58 encoding error: {}", err), } } diff --git a/src/util/psbt/map/global.rs b/src/util/psbt/map/global.rs index ba6bf569..5a571e51 100644 --- a/src/util/psbt/map/global.rs +++ b/src/util/psbt/map/global.rs @@ -14,7 +14,7 @@ use std::collections::BTreeMap; use std::collections::btree_map::Entry; -use std::io::{self, Cursor}; +use std::io::{self, Cursor, Read}; use blockdata::transaction::Transaction; use consensus::{encode, Encodable, Decodable}; @@ -22,6 +22,7 @@ use util::psbt::map::Map; use util::psbt::raw; use util::psbt; use util::psbt::Error; +use util::bip32::{ExtendedPubKey, KeySource, Fingerprint, DerivationPath, ChildNumber}; /// Type: Unsigned Transaction PSBT_GLOBAL_UNSIGNED_TX = 0x00 const PSBT_GLOBAL_UNSIGNED_TX: u8 = 0x00; @@ -32,10 +33,15 @@ pub struct Global { /// The unsigned transaction, scriptSigs and witnesses for each input must be /// empty. pub unsigned_tx: Transaction, + /// The version number of this PSBT. If omitted, the version number is 0. + pub version: u32, + /// A global map from extended public keys to the used key fingerprint and + /// derivation path as defined by BIP 32 + pub xpub: BTreeMap, /// Unknown global key-value pairs. pub unknown: BTreeMap>, } -serde_struct_impl!(Global, unsigned_tx, unknown); +serde_struct_impl!(Global, unsigned_tx, version, xpub, unknown); impl Global { /// Create a Global from an unsigned transaction, error if not unsigned @@ -52,6 +58,8 @@ impl Global { Ok(Global { unsigned_tx: tx, + xpub: Default::default(), + version: 0, unknown: Default::default(), }) } @@ -124,7 +132,9 @@ impl Decodable for Global { fn consensus_decode(mut d: D) -> Result { let mut tx: Option = None; + let mut version: Option = None; let mut unknowns: BTreeMap> = Default::default(); + let mut xpub_map: BTreeMap = Default::default(); loop { match raw::Pair::consensus_decode(&mut d) { @@ -158,6 +168,57 @@ impl Decodable for Global { return Err(Error::InvalidKey(pair.key).into()) } } + // Global Xpub + 0x01 => { + if !pair.key.key.is_empty() { + let xpub = ExtendedPubKey::decode(&pair.key.key) + .map_err(|_| { + encode::Error::ParseFailed("Can't deserialize ExtendedPublicKey from global XPUB key data") + })?; + + if pair.value.len() % 4 != 0 { + return Err(encode::Error::ParseFailed("Incorrect length of global xpub list")) + } + + let keys_count = pair.value.len() / 4 - 1; + let mut decoder = Cursor::new(pair.value); + let mut fingerprint = [0u8; 4]; + decoder.read_exact(&mut fingerprint[..])?; + let mut path = Vec::::with_capacity(keys_count); + while let Ok(index) = u32::consensus_decode(&mut decoder) { + path.push(ChildNumber::from(index)) + } + let derivation = DerivationPath::from(path); + // Keys, according to BIP-174, must be unique + if xpub_map.insert(xpub, (Fingerprint::from(&fingerprint[..]), derivation)).is_some() { + return Err(encode::Error::ParseFailed("Repeated global xpub key")) + } + } else { + return Err(encode::Error::ParseFailed("Xpub global key must contain serialized Xpub data")) + } + } + // Version + 0xFB => { + // key has to be empty + if pair.key.key.is_empty() { + // there can only be one version + if version.is_none() { + let vlen: usize = pair.value.len(); + let mut decoder = Cursor::new(pair.value); + if vlen != 4 { + return Err(encode::Error::ParseFailed("Wrong global version value length (must be 4 bytes)")) + } + version = Some(Decodable::consensus_decode(&mut decoder)?); + if decoder.position() != vlen as u64 { + return Err(encode::Error::ParseFailed("data not consumed entirely when explicitly deserializing")) + } + } else { + return Err(Error::DuplicateKey(pair.key).into()) + } + } else { + return Err(Error::InvalidKey(pair.key).into()) + } + } _ => match unknowns.entry(pair.key) { Entry::Vacant(empty_key) => {empty_key.insert(pair.value);}, Entry::Occupied(k) => return Err(Error::DuplicateKey(k.key().clone()).into()), @@ -171,6 +232,8 @@ impl Decodable for Global { if let Some(tx) = tx { let mut rv: Global = Global::from_unsigned_tx(tx)?; + rv.version = version.unwrap_or(0); + rv.xpub = xpub_map; rv.unknown = unknowns; Ok(rv) } else { diff --git a/src/util/psbt/mod.rs b/src/util/psbt/mod.rs index 236643d7..f3384d4a 100644 --- a/src/util/psbt/mod.rs +++ b/src/util/psbt/mod.rs @@ -192,6 +192,8 @@ mod tests { input: vec![], output: vec![], }, + xpub: Default::default(), + version: 0, unknown: BTreeMap::new(), }, inputs: vec![], @@ -279,6 +281,8 @@ mod tests { }, ], }, + xpub: Default::default(), + version: 0, unknown: Default::default(), }; @@ -381,6 +385,8 @@ mod tests { }, ], }, + xpub: Default::default(), + version: 0, unknown: BTreeMap::new(), }, inputs: vec![Input { @@ -604,6 +610,8 @@ mod tests { }, ], }, + version: 0, + xpub: Default::default(), unknown: BTreeMap::new(), }, inputs: vec![Input { From 2f838218a8c4a6d4ab1e6d1db13165de66700a16 Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Sat, 28 Nov 2020 13:49:50 +0100 Subject: [PATCH 2/8] PSBT: merging new global keys Plus necessary changes to BIP 32 implementations and error type --- src/util/bip32.rs | 9 +++++-- src/util/psbt/error.rs | 5 +++- src/util/psbt/map/global.rs | 51 +++++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/src/util/bip32.rs b/src/util/bip32.rs index 1dcabe42..964e79e1 100644 --- a/src/util/bip32.rs +++ b/src/util/bip32.rs @@ -126,14 +126,14 @@ impl ChildNumber { /// Returns `true` if the child number is a [`Normal`] value. /// /// [`Normal`]: #variant.Normal - pub fn is_normal(self) -> bool { + pub fn is_normal(&self) -> bool { !self.is_hardened() } /// Returns `true` if the child number is a [`Hardened`] value. /// /// [`Hardened`]: #variant.Hardened - pub fn is_hardened(self) -> bool { + pub fn is_hardened(&self) -> bool { match self { ChildNumber::Hardened {..} => true, ChildNumber::Normal {..} => false, @@ -299,6 +299,11 @@ impl<'a> Iterator for DerivationPathIterator<'a> { } impl DerivationPath { + /// Returns length of the derivation path + pub fn len(&self) -> usize { + self.0.len() + } + /// Create a new [DerivationPath] that is a child of this one. pub fn child(&self, cn: ChildNumber) -> DerivationPath { let mut path = self.0.clone(); diff --git a/src/util/psbt/error.rs b/src/util/psbt/error.rs index f8bf549a..a418ef07 100644 --- a/src/util/psbt/error.rs +++ b/src/util/psbt/error.rs @@ -68,7 +68,9 @@ pub enum Error { preimage: Vec, /// Hash value hash: Vec, - } + }, + /// Data inconsistency/conflicting data during merge procedure + MergeConflict(String), } impl fmt::Display for Error { @@ -91,6 +93,7 @@ impl fmt::Display for Error { // directly using debug forms of psbthash enums write!(f, "Preimage {:?} does not match {:?} hash {:?}", preimage, hash_type, hash ) } + Error::MergeConflict(ref s) => { write!(f, "Merge conflict: {}", s) } } } } diff --git a/src/util/psbt/map/global.rs b/src/util/psbt/map/global.rs index 5a571e51..d58dc6e7 100644 --- a/src/util/psbt/map/global.rs +++ b/src/util/psbt/map/global.rs @@ -15,6 +15,7 @@ use std::collections::BTreeMap; use std::collections::btree_map::Entry; use std::io::{self, Cursor, Read}; +use std::cmp::Ordering; use blockdata::transaction::Transaction; use consensus::{encode, Encodable, Decodable}; @@ -113,6 +114,8 @@ impl Map for Global { Ok(rv) } + // Keep in mind that according to BIP 174 this function must be transitive, i.e. + // A.merge(B) == B.merge(A) fn merge(&mut self, other: Self) -> Result<(), psbt::Error> { if self.unsigned_tx != other.unsigned_tx { return Err(psbt::Error::UnexpectedUnsignedTx { @@ -121,6 +124,54 @@ impl Map for Global { }); } + // BIP 174: The Combiner must remove any duplicate key-value pairs, in accordance with + // the specification. It can pick arbitrarily when conflicts occur. + + // Keeping the highest version + self.version = self.version.max(other.version); + + // Merging xpubs + for (xpub, (fingerprint1, derivation1)) in other.xpub { + match self.xpub.entry(xpub) { + Entry::Vacant(entry) => { + entry.insert((fingerprint1, derivation1)); + }, + Entry::Occupied(mut entry) => { + // Here in case of the conflict we select the version with algorithm: + // 1) if fingerprints are equal but derivations are not, we report an error; otherwise + // 2) choose longest unhardened derivation; if equal + // 3) choose longest derivation; if equal + // 4) choose largest fingerprint; if equal + // 5) do nothing + // Clone is required for Rust 1.29 borrow checker + let (fingerprint2, derivation2) = entry.get().clone(); + let len1 = derivation1.len(); + let len2 = derivation2.len(); + let normal_len1 = derivation1.into_iter().rposition(ChildNumber::is_normal); + let normal_len2 = derivation2.into_iter().rposition(ChildNumber::is_normal); + match (normal_len1.cmp(&normal_len2), len1.cmp(&len2), fingerprint1.cmp(&fingerprint2)) { + (Ordering::Equal, Ordering::Equal, Ordering::Equal) => {}, + (Ordering::Equal, _, Ordering::Equal) => { + return Err(psbt::Error::MergeConflict(format!( + "global xpub {} has inconsistent key sources", + xpub + ).to_owned())); + } + (Ordering::Greater, _, _) + | (Ordering::Equal, Ordering::Greater, _) + | (Ordering::Equal, Ordering::Equal, Ordering::Greater) => { + entry.insert((fingerprint1, derivation1)); + }, + (Ordering::Less, _, _) + | (Ordering::Equal, Ordering::Less, _) + | (Ordering::Equal, Ordering::Equal, Ordering::Less) => { + // do nothing here: we already own the proper data + } + } + } + } + } + self.unknown.extend(other.unknown); Ok(()) } From df8635c5fe357a9cbe6565264d7928fafebd12d9 Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Sat, 28 Nov 2020 14:41:52 +0100 Subject: [PATCH 3/8] PSBT: Key pair serialization for new global keys Conflicts: src/util/psbt/map/global.rs --- src/util/psbt/error.rs | 4 ++-- src/util/psbt/map/global.rs | 38 ++++++++++++++++++++++++++++++++----- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/util/psbt/error.rs b/src/util/psbt/error.rs index a418ef07..9da6bd26 100644 --- a/src/util/psbt/error.rs +++ b/src/util/psbt/error.rs @@ -20,7 +20,7 @@ use util::psbt::raw; use hashes; -#[derive(Debug)] +#[derive(Copy, Clone, PartialEq, Eq, Debug)] /// Enum for marking psbt hash error pub enum PsbtHash { Ripemd, @@ -29,7 +29,7 @@ pub enum PsbtHash { Hash256, } /// Ways that a Partially Signed Transaction might fail. -#[derive(Debug)] +#[derive(Clone, PartialEq, Eq, Debug)] pub enum Error { /// Magic bytes for a PSBT must be the ASCII for "psbt" serialized in most /// significant byte order. diff --git a/src/util/psbt/map/global.rs b/src/util/psbt/map/global.rs index d58dc6e7..04b703ad 100644 --- a/src/util/psbt/map/global.rs +++ b/src/util/psbt/map/global.rs @@ -23,11 +23,13 @@ use util::psbt::map::Map; use util::psbt::raw; use util::psbt; use util::psbt::Error; +use util::endian::u32_to_array_le; use util::bip32::{ExtendedPubKey, KeySource, Fingerprint, DerivationPath, ChildNumber}; /// Type: Unsigned Transaction PSBT_GLOBAL_UNSIGNED_TX = 0x00 const PSBT_GLOBAL_UNSIGNED_TX: u8 = 0x00; - +const PSBT_GLOBAL_XPUB: u8 = 0x01; +const PSBT_GLOBAL_VERSION: u8 = 0xFB; /// A key-value map for global data. #[derive(Clone, Debug, PartialEq)] pub struct Global { @@ -104,6 +106,34 @@ impl Map for Global { }, }); + for (xpub, (fingerprint, derivation)) in &self.xpub { + rv.push(raw::Pair { + key: raw::Key { + type_value: PSBT_GLOBAL_XPUB, + key: xpub.encode().to_vec(), + }, + value: { + let mut ret = Vec::with_capacity(4 + derivation.len() * 4); + ret.extend(fingerprint.as_bytes()); + for no in 0..derivation.len() { + ret.extend(&u32_to_array_le(derivation[no].into())[..]) + } + ret + } + }); + } + + // Serializing version only for non-default value; otherwise test vectors fail + if self.version > 0 { + rv.push(raw::Pair { + key: raw::Key { + type_value: PSBT_GLOBAL_VERSION, + key: vec![], + }, + value: u32_to_array_le(self.version).to_vec() + }); + } + for (key, value) in self.unknown.iter() { rv.push(raw::Pair { key: key.clone(), @@ -219,8 +249,7 @@ impl Decodable for Global { return Err(Error::InvalidKey(pair.key).into()) } } - // Global Xpub - 0x01 => { + PSBT_GLOBAL_XPUB => { if !pair.key.key.is_empty() { let xpub = ExtendedPubKey::decode(&pair.key.key) .map_err(|_| { @@ -248,8 +277,7 @@ impl Decodable for Global { return Err(encode::Error::ParseFailed("Xpub global key must contain serialized Xpub data")) } } - // Version - 0xFB => { + PSBT_GLOBAL_VERSION => { // key has to be empty if pair.key.key.is_empty() { // there can only be one version From 0235abfac2fabd039a3171f72ade633ab4086f08 Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Mon, 7 Dec 2020 16:41:57 +0100 Subject: [PATCH 4/8] Improving PSBT merge with dedicated 1.29 rustc borrow scope --- src/util/psbt/map/global.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/util/psbt/map/global.rs b/src/util/psbt/map/global.rs index 04b703ad..fc6b6f06 100644 --- a/src/util/psbt/map/global.rs +++ b/src/util/psbt/map/global.rs @@ -173,12 +173,13 @@ impl Map for Global { // 3) choose longest derivation; if equal // 4) choose largest fingerprint; if equal // 5) do nothing - // Clone is required for Rust 1.29 borrow checker - let (fingerprint2, derivation2) = entry.get().clone(); + let (fingerprint2, len2, normal_len2) = { + // weird scope needed for rustc 1.29 borrow checker + let (fp, deriv) = entry.get().clone(); + (fp, deriv.len(), deriv.into_iter().rposition(ChildNumber::is_normal)) + }; let len1 = derivation1.len(); - let len2 = derivation2.len(); let normal_len1 = derivation1.into_iter().rposition(ChildNumber::is_normal); - let normal_len2 = derivation2.into_iter().rposition(ChildNumber::is_normal); match (normal_len1.cmp(&normal_len2), len1.cmp(&len2), fingerprint1.cmp(&fingerprint2)) { (Ordering::Equal, Ordering::Equal, Ordering::Equal) => {}, (Ordering::Equal, _, Ordering::Equal) => { From 8b1666295c386dc702217e5a07ba5770d2c5ddf7 Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Mon, 7 Dec 2020 20:04:23 +0100 Subject: [PATCH 5/8] Nits in new PSBT global types reviews --- src/util/psbt/map/global.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/util/psbt/map/global.rs b/src/util/psbt/map/global.rs index fc6b6f06..3fe94fae 100644 --- a/src/util/psbt/map/global.rs +++ b/src/util/psbt/map/global.rs @@ -15,7 +15,7 @@ use std::collections::BTreeMap; use std::collections::btree_map::Entry; use std::io::{self, Cursor, Read}; -use std::cmp::Ordering; +use std::cmp::{self, Ordering}; use blockdata::transaction::Transaction; use consensus::{encode, Encodable, Decodable}; @@ -28,8 +28,11 @@ use util::bip32::{ExtendedPubKey, KeySource, Fingerprint, DerivationPath, ChildN /// Type: Unsigned Transaction PSBT_GLOBAL_UNSIGNED_TX = 0x00 const PSBT_GLOBAL_UNSIGNED_TX: u8 = 0x00; +/// Type: Extended Public Key PSBT_GLOBAL_XPUB = 0x01 const PSBT_GLOBAL_XPUB: u8 = 0x01; +/// Type: Version Number PSBT_GLOBAL_VERSION = 0xFB const PSBT_GLOBAL_VERSION: u8 = 0xFB; + /// A key-value map for global data. #[derive(Clone, Debug, PartialEq)] pub struct Global { @@ -115,9 +118,7 @@ impl Map for Global { value: { let mut ret = Vec::with_capacity(4 + derivation.len() * 4); ret.extend(fingerprint.as_bytes()); - for no in 0..derivation.len() { - ret.extend(&u32_to_array_le(derivation[no].into())[..]) - } + derivation.into_iter().for_each(|n| ret.extend(&u32_to_array_le((*n).into()))); ret } }); @@ -144,7 +145,7 @@ impl Map for Global { Ok(rv) } - // Keep in mind that according to BIP 174 this function must be transitive, i.e. + // Keep in mind that according to BIP 174 this function must be commutative, i.e. // A.merge(B) == B.merge(A) fn merge(&mut self, other: Self) -> Result<(), psbt::Error> { if self.unsigned_tx != other.unsigned_tx { @@ -158,7 +159,7 @@ impl Map for Global { // the specification. It can pick arbitrarily when conflicts occur. // Keeping the highest version - self.version = self.version.max(other.version); + self.version = cmp::max(self.version, other.version); // Merging xpubs for (xpub, (fingerprint1, derivation1)) in other.xpub { @@ -171,7 +172,7 @@ impl Map for Global { // 1) if fingerprints are equal but derivations are not, we report an error; otherwise // 2) choose longest unhardened derivation; if equal // 3) choose longest derivation; if equal - // 4) choose largest fingerprint; if equal + // 4) choose largest fingerprint using binary ordering; if equal // 5) do nothing let (fingerprint2, len2, normal_len2) = { // weird scope needed for rustc 1.29 borrow checker @@ -184,8 +185,7 @@ impl Map for Global { (Ordering::Equal, Ordering::Equal, Ordering::Equal) => {}, (Ordering::Equal, _, Ordering::Equal) => { return Err(psbt::Error::MergeConflict(format!( - "global xpub {} has inconsistent key sources", - xpub + "global xpub {} has inconsistent key sources", xpub ).to_owned())); } (Ordering::Greater, _, _) @@ -253,9 +253,9 @@ impl Decodable for Global { PSBT_GLOBAL_XPUB => { if !pair.key.key.is_empty() { let xpub = ExtendedPubKey::decode(&pair.key.key) - .map_err(|_| { - encode::Error::ParseFailed("Can't deserialize ExtendedPublicKey from global XPUB key data") - })?; + .map_err(|_| encode::Error::ParseFailed( + "Can't deserialize ExtendedPublicKey from global XPUB key data" + ))?; if pair.value.len() % 4 != 0 { return Err(encode::Error::ParseFailed("Incorrect length of global xpub list")) From 21c11e3315d6753b868abc7a61a0772c46f5646c Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Wed, 16 Dec 2020 16:39:46 +0100 Subject: [PATCH 6/8] BSPT: Improving global xpub merging algorithm --- src/util/psbt/map/global.rs | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/util/psbt/map/global.rs b/src/util/psbt/map/global.rs index 3fe94fae..d4dc5c4d 100644 --- a/src/util/psbt/map/global.rs +++ b/src/util/psbt/map/global.rs @@ -172,30 +172,36 @@ impl Map for Global { // 1) if fingerprints are equal but derivations are not, we report an error; otherwise // 2) choose longest unhardened derivation; if equal // 3) choose longest derivation; if equal - // 4) choose largest fingerprint using binary ordering; if equal - // 5) do nothing - let (fingerprint2, len2, normal_len2) = { + // 4) choose the largest derivation using binary ordering; if equal + // 5) check that fingerprints are equal, otherwise fail; + // 6) if fingerprints are also equal we do nothing + let (fingerprint2, len2, normal_len2, deriv_cmp) = { // weird scope needed for rustc 1.29 borrow checker let (fp, deriv) = entry.get().clone(); - (fp, deriv.len(), deriv.into_iter().rposition(ChildNumber::is_normal)) + ( + fp, + deriv.len(), + deriv.into_iter().rposition(ChildNumber::is_normal), + derivation1.cmp(&deriv) + ) }; let len1 = derivation1.len(); let normal_len1 = derivation1.into_iter().rposition(ChildNumber::is_normal); - match (normal_len1.cmp(&normal_len2), len1.cmp(&len2), fingerprint1.cmp(&fingerprint2)) { - (Ordering::Equal, Ordering::Equal, Ordering::Equal) => {}, - (Ordering::Equal, _, Ordering::Equal) => { + match (normal_len1.cmp(&normal_len2), len1.cmp(&len2), deriv_cmp, fingerprint1.cmp(&fingerprint2)) { + (Ordering::Equal, Ordering::Equal, Ordering::Equal, Ordering::Equal) => {}, + (Ordering::Equal, Ordering::Equal, Ordering::Equal, _) => { return Err(psbt::Error::MergeConflict(format!( "global xpub {} has inconsistent key sources", xpub ).to_owned())); } - (Ordering::Greater, _, _) - | (Ordering::Equal, Ordering::Greater, _) - | (Ordering::Equal, Ordering::Equal, Ordering::Greater) => { + (Ordering::Greater, ..) + | (Ordering::Equal, Ordering::Greater, ..) + | (Ordering::Equal, Ordering::Equal, Ordering::Greater, _) => { entry.insert((fingerprint1, derivation1)); }, - (Ordering::Less, _, _) - | (Ordering::Equal, Ordering::Less, _) - | (Ordering::Equal, Ordering::Equal, Ordering::Less) => { + (Ordering::Less, ..) + | (Ordering::Equal, Ordering::Less, ..) + | (Ordering::Equal, Ordering::Equal, Ordering::Less, _) => { // do nothing here: we already own the proper data } } From b84faa7f5e6f9d273a4a6cb78098f10c73594783 Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Wed, 16 Dec 2020 16:46:28 +0100 Subject: [PATCH 7/8] PSBT: Improved global keys version and xpub handling --- src/util/psbt/map/global.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/util/psbt/map/global.rs b/src/util/psbt/map/global.rs index d4dc5c4d..e55775ff 100644 --- a/src/util/psbt/map/global.rs +++ b/src/util/psbt/map/global.rs @@ -263,15 +263,15 @@ impl Decodable for Global { "Can't deserialize ExtendedPublicKey from global XPUB key data" ))?; - if pair.value.len() % 4 != 0 { - return Err(encode::Error::ParseFailed("Incorrect length of global xpub list")) + if pair.value.is_empty() || pair.value.len() % 4 != 0 { + return Err(encode::Error::ParseFailed("Incorrect length of global xpub derivation data")) } - let keys_count = pair.value.len() / 4 - 1; + let child_count = pair.value.len() / 4 - 1; let mut decoder = Cursor::new(pair.value); let mut fingerprint = [0u8; 4]; decoder.read_exact(&mut fingerprint[..])?; - let mut path = Vec::::with_capacity(keys_count); + let mut path = Vec::::with_capacity(child_count); while let Ok(index) = u32::consensus_decode(&mut decoder) { path.push(ChildNumber::from(index)) } @@ -295,8 +295,10 @@ impl Decodable for Global { return Err(encode::Error::ParseFailed("Wrong global version value length (must be 4 bytes)")) } version = Some(Decodable::consensus_decode(&mut decoder)?); - if decoder.position() != vlen as u64 { - return Err(encode::Error::ParseFailed("data not consumed entirely when explicitly deserializing")) + // We only understand version 0 PSBTs. According to BIP-174 we + // should throw an error if we see anything other than version 0. + if version != Some(0) { + return Err(encode::Error::ParseFailed("PSBT versions greater than 0 are not supported")) } } else { return Err(Error::DuplicateKey(pair.key).into()) From 7f5c2795d6d581276119edf2becf4377d7c7df08 Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Fri, 18 Dec 2020 22:32:02 +0100 Subject: [PATCH 8/8] PSBT global xpub merging algorithm reworked --- src/util/psbt/map/global.rs | 64 ++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 36 deletions(-) diff --git a/src/util/psbt/map/global.rs b/src/util/psbt/map/global.rs index e55775ff..15e9c945 100644 --- a/src/util/psbt/map/global.rs +++ b/src/util/psbt/map/global.rs @@ -15,7 +15,7 @@ use std::collections::BTreeMap; use std::collections::btree_map::Entry; use std::io::{self, Cursor, Read}; -use std::cmp::{self, Ordering}; +use std::cmp; use blockdata::transaction::Transaction; use consensus::{encode, Encodable, Decodable}; @@ -169,42 +169,34 @@ impl Map for Global { }, Entry::Occupied(mut entry) => { // Here in case of the conflict we select the version with algorithm: - // 1) if fingerprints are equal but derivations are not, we report an error; otherwise - // 2) choose longest unhardened derivation; if equal - // 3) choose longest derivation; if equal - // 4) choose the largest derivation using binary ordering; if equal - // 5) check that fingerprints are equal, otherwise fail; - // 6) if fingerprints are also equal we do nothing - let (fingerprint2, len2, normal_len2, deriv_cmp) = { - // weird scope needed for rustc 1.29 borrow checker - let (fp, deriv) = entry.get().clone(); - ( - fp, - deriv.len(), - deriv.into_iter().rposition(ChildNumber::is_normal), - derivation1.cmp(&deriv) - ) - }; - let len1 = derivation1.len(); - let normal_len1 = derivation1.into_iter().rposition(ChildNumber::is_normal); - match (normal_len1.cmp(&normal_len2), len1.cmp(&len2), deriv_cmp, fingerprint1.cmp(&fingerprint2)) { - (Ordering::Equal, Ordering::Equal, Ordering::Equal, Ordering::Equal) => {}, - (Ordering::Equal, Ordering::Equal, Ordering::Equal, _) => { - return Err(psbt::Error::MergeConflict(format!( - "global xpub {} has inconsistent key sources", xpub - ).to_owned())); - } - (Ordering::Greater, ..) - | (Ordering::Equal, Ordering::Greater, ..) - | (Ordering::Equal, Ordering::Equal, Ordering::Greater, _) => { - entry.insert((fingerprint1, derivation1)); - }, - (Ordering::Less, ..) - | (Ordering::Equal, Ordering::Less, ..) - | (Ordering::Equal, Ordering::Equal, Ordering::Less, _) => { - // do nothing here: we already own the proper data - } + // 1) if everything is equal we do nothing + // 2) report an error if + // - derivation paths are equal and fingerprints are not + // - derivation paths are of the same length, but not equal + // - derivation paths has different length, but the shorter one + // is not the strict suffix of the longer one + // 3) choose longest derivation otherwise + + let (fingerprint2, derivation2) = entry.get().clone(); + + if derivation1 == derivation2 && fingerprint1 == fingerprint2 + { + continue } + else if + derivation1.len() < derivation2.len() && + derivation1[..] == derivation2[derivation2.len() - derivation1.len()..] + { + continue + } + else if derivation2[..] == derivation1[derivation1.len() - derivation2.len()..] + { + entry.insert((fingerprint1, derivation1)); + continue + } + return Err(psbt::Error::MergeConflict(format!( + "global xpub {} has inconsistent key sources", xpub + ).to_owned())); } } }