From c075183a7b6121ea70ec030cffcea2c380f7d6f9 Mon Sep 17 00:00:00 2001 From: Evgenii P Date: Mon, 14 Dec 2020 17:50:47 +0700 Subject: [PATCH 1/5] Revert replacing BTreeMap to HashMap in SingersContainer --- src/wallet/signer.rs | 82 ++++++++++++++++++-------------------------- 1 file changed, 33 insertions(+), 49 deletions(-) diff --git a/src/wallet/signer.rs b/src/wallet/signer.rs index c2cbef02..a2aba04f 100644 --- a/src/wallet/signer.rs +++ b/src/wallet/signer.rs @@ -91,8 +91,9 @@ //! ``` use std::cmp::Ordering; -use std::collections::HashMap; +use std::collections::BTreeMap; use std::fmt; +use std::ops::Bound::Included; use std::sync::Arc; use bitcoin::blockdata::opcodes; @@ -295,7 +296,7 @@ impl Signer for PrivateKey { /// The default value is `100`. Signers with an ordering above that will be called later, /// and they will thus see the partial signatures added to the transaction once they get to sign /// themselves. -#[derive(Debug, Clone, Hash, PartialOrd, PartialEq, Ord, Eq)] +#[derive(Debug, Clone, PartialOrd, PartialEq, Ord, Eq)] pub struct SignerOrdering(pub usize); impl std::default::Default for SignerOrdering { @@ -304,7 +305,7 @@ impl std::default::Default for SignerOrdering { } } -#[derive(Debug, Clone, Hash, Eq, PartialEq)] +#[derive(Debug, Clone)] struct SignersContainerKey { id: SignerId, ordering: SignerOrdering, @@ -321,7 +322,7 @@ impl From<(SignerId, SignerOrdering)> for SignersContainerKey { /// Container for multiple signers #[derive(Debug, Default, Clone)] -pub struct SignersContainer(HashMap>); +pub struct SignersContainer(BTreeMap>); impl SignersContainer { pub fn as_key_map(&self, secp: &SecpCtx) -> KeyMap { @@ -394,18 +395,18 @@ impl SignersContainer { /// Returns the list of signers in the container, sorted by lowest to highest `ordering` pub fn signers(&self) -> Vec<&Arc> { - let mut items = self.0.iter().collect::>(); - items.sort_unstable_by_key(|(key, _)| *key); - items.into_iter().map(|(_, v)| v).collect() + self.0.values().collect() } /// Finds the signer with lowest ordering for a given id in the container. pub fn find(&self, id: SignerId) -> Option<&Arc> { self.0 - .iter() - .filter(|(key, _)| key.id == id) - .min_by(|(k_a, _), (k_b, _)| k_a.cmp(k_b)) + .range(( + Included(&(id.clone(), SignerOrdering(0)).into()), + Included(&(id, SignerOrdering(usize::MAX)).into()), + )) .map(|(_, v)| v) + .next() } } @@ -525,18 +526,26 @@ impl Ord for SignersContainerKey { } } +impl PartialEq for SignersContainerKey { + fn eq(&self, other: &Self) -> bool { + self.ordering == other.ordering + } +} + +impl Eq for SignersContainerKey {} + #[cfg(test)] mod signers_container_tests { use super::*; use crate::descriptor; - use crate::descriptor::ToWalletDescriptor; - use crate::keys::{DescriptorKey, ToDescriptorKey}; - use bitcoin::secp256k1::All; - use bitcoin::util::bip32; - use bitcoin::util::psbt::PartiallySignedTransaction; - use bitcoin::Network; use miniscript::ScriptContext; + use crate::keys::{DescriptorKey, ToDescriptorKey}; + use bitcoin::util::bip32; + use bitcoin::secp256k1::All; use std::str::FromStr; + use crate::descriptor::ToWalletDescriptor; + use bitcoin::Network; + use bitcoin::util::psbt::PartiallySignedTransaction; // Signers added with the same ordering (like `Ordering::default`) created from `KeyMap` // should be preserved and not overwritten. @@ -562,21 +571,9 @@ mod signers_container_tests { let signer2 = Arc::new(DummySigner); let signer3 = Arc::new(DummySigner); - signers.add_external( - SignerId::Fingerprint(b"cafe"[..].into()), - SignerOrdering(1), - signer1.clone(), - ); - signers.add_external( - SignerId::Fingerprint(b"babe"[..].into()), - SignerOrdering(2), - signer2.clone(), - ); - signers.add_external( - SignerId::Fingerprint(b"feed"[..].into()), - SignerOrdering(3), - signer3.clone(), - ); + signers.add_external(SignerId::Fingerprint(b"cafe"[..].into()), SignerOrdering(1), signer1.clone()); + signers.add_external(SignerId::Fingerprint(b"babe"[..].into()), SignerOrdering(2), signer2.clone()); + signers.add_external(SignerId::Fingerprint(b"feed"[..].into()), SignerOrdering(3), signer3.clone()); // Check that signers are sorted from lowest to highest ordering let signers = signers.signers(); @@ -602,22 +599,14 @@ mod signers_container_tests { signers.add_external(id2.clone(), SignerOrdering(2), signer2.clone()); signers.add_external(id3.clone(), SignerOrdering(3), signer3.clone()); - assert!( - matches!(signers.find(id1), Some(signer) if Arc::as_ptr(&signer1) == Arc::as_ptr(signer)) - ); - assert!( - matches!(signers.find(id2), Some(signer) if Arc::as_ptr(&signer2) == Arc::as_ptr(signer)) - ); - assert!( - matches!(signers.find(id3.clone()), Some(signer) if Arc::as_ptr(&signer3) == Arc::as_ptr(signer)) - ); + assert!(matches!(signers.find(id1), Some(signer) if Arc::as_ptr(&signer1) == Arc::as_ptr(signer))); + assert!(matches!(signers.find(id2), Some(signer) if Arc::as_ptr(&signer2) == Arc::as_ptr(signer))); + assert!(matches!(signers.find(id3.clone()), Some(signer) if Arc::as_ptr(&signer3) == Arc::as_ptr(signer))); // The `signer4` has the same ID as `signer3` but lower ordering. // It should be found by `id3` instead of `signer3`. signers.add_external(id3.clone(), SignerOrdering(2), signer4.clone()); - assert!( - matches!(signers.find(id3), Some(signer) if Arc::as_ptr(&signer4) == Arc::as_ptr(signer)) - ); + assert!(matches!(signers.find(id3), Some(signer) if Arc::as_ptr(&signer4) == Arc::as_ptr(signer))); // Can't find anything with ID that doesn't exist assert!(matches!(signers.find(id_nonexistent), None)); @@ -626,12 +615,7 @@ mod signers_container_tests { #[derive(Debug)] struct DummySigner; impl Signer for DummySigner { - fn sign( - &self, - _psbt: &mut PartiallySignedTransaction, - _input_index: Option, - _secp: &SecpCtx, - ) -> Result<(), SignerError> { + fn sign(&self, _psbt: &mut PartiallySignedTransaction, _input_index: Option, _secp: &SecpCtx) -> Result<(), SignerError> { Ok(()) } From 2658a9b05af1c68ecc9cc940ef9cbc2bb7301faa Mon Sep 17 00:00:00 2001 From: Evgenii P Date: Tue, 15 Dec 2020 11:33:33 +0700 Subject: [PATCH 2/5] Fix SignersContainerKey PartialOrd to respect the ID --- src/wallet/signer.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/signer.rs b/src/wallet/signer.rs index a2aba04f..2a201379 100644 --- a/src/wallet/signer.rs +++ b/src/wallet/signer.rs @@ -112,7 +112,7 @@ use crate::descriptor::XKeyUtils; /// Identifier of a signer in the `SignersContainers`. Used as a key to find the right signer among /// multiple of them -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, Ord, PartialOrd, PartialEq, Eq, Hash)] pub enum SignerId { PkHash(hash160::Hash), Fingerprint(Fingerprint), @@ -522,7 +522,7 @@ impl PartialOrd for SignersContainerKey { impl Ord for SignersContainerKey { fn cmp(&self, other: &Self) -> Ordering { - self.ordering.cmp(&other.ordering) + self.ordering.cmp(&other.ordering).then(self.id.cmp(&other.id)) } } From c58236fcd7a2f91d1cdc5a3e4118c1bc8b64dfe3 Mon Sep 17 00:00:00 2001 From: Evgenii P Date: Tue, 15 Dec 2020 11:33:57 +0700 Subject: [PATCH 3/5] Fix SignersContainer::find to filter out incorrect IDs --- src/wallet/signer.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/wallet/signer.rs b/src/wallet/signer.rs index 2a201379..e5179b7f 100644 --- a/src/wallet/signer.rs +++ b/src/wallet/signer.rs @@ -403,8 +403,9 @@ impl SignersContainer { self.0 .range(( Included(&(id.clone(), SignerOrdering(0)).into()), - Included(&(id, SignerOrdering(usize::MAX)).into()), + Included(&(id.clone(), SignerOrdering(usize::MAX)).into()), )) + .filter(|(k, _)| k.id == id) .map(|(_, v)| v) .next() } From 5315c3ef25fcfe330c1d0f5f397ccf48f50ee242 Mon Sep 17 00:00:00 2001 From: Evgenii P Date: Tue, 15 Dec 2020 11:34:25 +0700 Subject: [PATCH 4/5] rustfmt --- src/wallet/signer.rs | 57 ++++++++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 15 deletions(-) diff --git a/src/wallet/signer.rs b/src/wallet/signer.rs index e5179b7f..f7d7790d 100644 --- a/src/wallet/signer.rs +++ b/src/wallet/signer.rs @@ -523,7 +523,9 @@ impl PartialOrd for SignersContainerKey { impl Ord for SignersContainerKey { fn cmp(&self, other: &Self) -> Ordering { - self.ordering.cmp(&other.ordering).then(self.id.cmp(&other.id)) + self.ordering + .cmp(&other.ordering) + .then(self.id.cmp(&other.id)) } } @@ -539,14 +541,14 @@ impl Eq for SignersContainerKey {} mod signers_container_tests { use super::*; use crate::descriptor; - use miniscript::ScriptContext; - use crate::keys::{DescriptorKey, ToDescriptorKey}; - use bitcoin::util::bip32; - use bitcoin::secp256k1::All; - use std::str::FromStr; use crate::descriptor::ToWalletDescriptor; - use bitcoin::Network; + use crate::keys::{DescriptorKey, ToDescriptorKey}; + use bitcoin::secp256k1::All; + use bitcoin::util::bip32; use bitcoin::util::psbt::PartiallySignedTransaction; + use bitcoin::Network; + use miniscript::ScriptContext; + use std::str::FromStr; // Signers added with the same ordering (like `Ordering::default`) created from `KeyMap` // should be preserved and not overwritten. @@ -572,9 +574,21 @@ mod signers_container_tests { let signer2 = Arc::new(DummySigner); let signer3 = Arc::new(DummySigner); - signers.add_external(SignerId::Fingerprint(b"cafe"[..].into()), SignerOrdering(1), signer1.clone()); - signers.add_external(SignerId::Fingerprint(b"babe"[..].into()), SignerOrdering(2), signer2.clone()); - signers.add_external(SignerId::Fingerprint(b"feed"[..].into()), SignerOrdering(3), signer3.clone()); + signers.add_external( + SignerId::Fingerprint(b"cafe"[..].into()), + SignerOrdering(1), + signer1.clone(), + ); + signers.add_external( + SignerId::Fingerprint(b"babe"[..].into()), + SignerOrdering(2), + signer2.clone(), + ); + signers.add_external( + SignerId::Fingerprint(b"feed"[..].into()), + SignerOrdering(3), + signer3.clone(), + ); // Check that signers are sorted from lowest to highest ordering let signers = signers.signers(); @@ -600,14 +614,22 @@ mod signers_container_tests { signers.add_external(id2.clone(), SignerOrdering(2), signer2.clone()); signers.add_external(id3.clone(), SignerOrdering(3), signer3.clone()); - assert!(matches!(signers.find(id1), Some(signer) if Arc::as_ptr(&signer1) == Arc::as_ptr(signer))); - assert!(matches!(signers.find(id2), Some(signer) if Arc::as_ptr(&signer2) == Arc::as_ptr(signer))); - assert!(matches!(signers.find(id3.clone()), Some(signer) if Arc::as_ptr(&signer3) == Arc::as_ptr(signer))); + assert!( + matches!(signers.find(id1), Some(signer) if Arc::as_ptr(&signer1) == Arc::as_ptr(signer)) + ); + assert!( + matches!(signers.find(id2), Some(signer) if Arc::as_ptr(&signer2) == Arc::as_ptr(signer)) + ); + assert!( + matches!(signers.find(id3.clone()), Some(signer) if Arc::as_ptr(&signer3) == Arc::as_ptr(signer)) + ); // The `signer4` has the same ID as `signer3` but lower ordering. // It should be found by `id3` instead of `signer3`. signers.add_external(id3.clone(), SignerOrdering(2), signer4.clone()); - assert!(matches!(signers.find(id3), Some(signer) if Arc::as_ptr(&signer4) == Arc::as_ptr(signer))); + assert!( + matches!(signers.find(id3), Some(signer) if Arc::as_ptr(&signer4) == Arc::as_ptr(signer)) + ); // Can't find anything with ID that doesn't exist assert!(matches!(signers.find(id_nonexistent), None)); @@ -616,7 +638,12 @@ mod signers_container_tests { #[derive(Debug)] struct DummySigner; impl Signer for DummySigner { - fn sign(&self, _psbt: &mut PartiallySignedTransaction, _input_index: Option, _secp: &SecpCtx) -> Result<(), SignerError> { + fn sign( + &self, + _psbt: &mut PartiallySignedTransaction, + _input_index: Option, + _secp: &SecpCtx, + ) -> Result<(), SignerError> { Ok(()) } From 09730c0898b774c0d7cac6cb8f5d681fdf270562 Mon Sep 17 00:00:00 2001 From: Evgenii P Date: Tue, 15 Dec 2020 22:40:07 +0700 Subject: [PATCH 5/5] Take ID into account in SignersContainerKey's PartialEq impl --- src/wallet/signer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/signer.rs b/src/wallet/signer.rs index f7d7790d..6521e2b0 100644 --- a/src/wallet/signer.rs +++ b/src/wallet/signer.rs @@ -531,7 +531,7 @@ impl Ord for SignersContainerKey { impl PartialEq for SignersContainerKey { fn eq(&self, other: &Self) -> bool { - self.ordering == other.ordering + self.id == other.id && self.ordering == other.ordering } }