Merge #564: Remove hashbrown dependency

apoelstra · apoelstra · commit 65a004c48f7c · 2023-08-12T15:41:10.000Z
49fd7b9 Remove hashbrown dependency (Tobin C. Harding) 4e174e1 Use BTreeMap in compile test code (Tobin C. Harding) eefd337 Use BTreeMap in test code (Tobin C. Harding) 13b9ec9 Use BTreeSet internally (Tobin C. Harding) 023f13b Fix stale variable names and comments (Tobin C. Harding) cef9d46 Use BTreeMap internally (Tobin C. Harding) 4e2ee86 Use BTreeMap for descriptor::KeyMap (Tobin C. Harding) 49a2bdf Add Satisfier macro for ((hash160, leaf hash), (pk, tap sig)) (Tobin C. Harding) df11203 Add Satisfier macro for (hash160, (pk, ecdsa sig)) (Tobin C. Harding) 42f559f Add Satisfier macro for ((pk, leaf hash), taproot sig) (Tobin C. Harding) 20ca6a5 Implement Satisfier for map<Pk, bitcoin::ecdsa::Signature> (Tobin C. Harding) Pull request description: We can use `BTreeSet` instead of `HashSet` and `BTreeMap` instead of `HashMap` to remove the `hashbrown` dependency. ACKs for top commit: apoelstra: ACK 49fd7b9 Tree-SHA512: 73d0e29812b91c18a10e6dd57363a7b188985c62d74500134b41879539ce2ecc6cf8d638ed27ba4986ddbe3e70f7f5bc0d3cf70491da521b5f724b818cb5334c
diff --git a/Cargo.toml b/Cargo.toml
@@ -13,7 +13,7 @@ edition = "2018"
 [features]
 default = ["std"]
 std = ["bitcoin/std", "bitcoin/secp-recovery"]
-no-std = ["hashbrown", "bitcoin/no-std"]
+no-std = ["bitcoin/no-std"]
 compiler = []
 trace = []
 
@@ -23,7 +23,6 @@ base64 = ["bitcoin/base64"]
 
 [dependencies]
 bitcoin = { version = "0.30.0", default-features = false }
-hashbrown = { version = "0.11", optional = true }
 internals = { package = "bitcoin-private", version = "0.1.0", default_features = false }
 
 # Do NOT use this as a feature! Use the `serde` feature instead.
diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs
@@ -57,7 +57,7 @@ pub use self::key::{
 /// [`Descriptor::parse_descriptor`], since the descriptor will always only contain
 /// public keys. This map allows looking up the corresponding secret key given a
 /// public key from the descriptor.
-pub type KeyMap = HashMap<DescriptorPublicKey, DescriptorSecretKey>;
+pub type KeyMap = BTreeMap<DescriptorPublicKey, DescriptorSecretKey>;
 
 /// Script descriptor
 #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
@@ -656,7 +656,7 @@ impl Descriptor<DescriptorPublicKey> {
             Ok(public_key)
         }
 
-        let mut keymap_pk = KeyMapWrapper(HashMap::new(), secp);
+        let mut keymap_pk = KeyMapWrapper(BTreeMap::new(), secp);
 
         struct KeyMapWrapper<'a, C: secp256k1::Signing>(KeyMap, &'a secp256k1::Secp256k1<C>);
 
@@ -1501,7 +1501,7 @@ mod tests {
             witness: Witness::default(),
         };
         let satisfier = {
-            let mut satisfier = HashMap::with_capacity(2);
+            let mut satisfier = BTreeMap::new();
 
             satisfier.insert(
                 a,
diff --git a/src/lib.rs b/src/lib.rs
@@ -100,9 +100,6 @@ pub use bitcoin;
 #[macro_use]
 extern crate alloc;
 
-#[cfg(not(feature = "std"))]
-extern crate hashbrown;
-
 #[cfg(any(feature = "std", test))]
 extern crate core;
 
@@ -961,9 +958,6 @@ mod prelude {
         vec::Vec,
     };
 
-    #[cfg(all(not(feature = "std"), not(test)))]
-    pub use hashbrown::{HashMap, HashSet};
-
     #[cfg(all(not(feature = "std"), not(test)))]
     pub use self::mutex::Mutex;
 }
diff --git a/src/miniscript/analyzable.rs b/src/miniscript/analyzable.rs
@@ -213,7 +213,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
         // to have an iterator
         let all_pkhs_len = self.iter_pk().count();
 
-        let unique_pkhs_len = self.iter_pk().collect::<HashSet<_>>().len();
+        let unique_pkhs_len = self.iter_pk().collect::<BTreeSet<_>>().len();
 
         unique_pkhs_len != all_pkhs_len
     }
diff --git a/src/miniscript/satisfy.rs b/src/miniscript/satisfy.rs
@@ -153,71 +153,130 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfier<Pk> for absolute::LockTime {
         }
     }
 }
-impl<Pk: MiniscriptKey + ToPublicKey> Satisfier<Pk> for HashMap<Pk, bitcoin::ecdsa::Signature> {
-    fn lookup_ecdsa_sig(&self, key: &Pk) -> Option<bitcoin::ecdsa::Signature> {
-        self.get(key).copied()
-    }
+
+macro_rules! impl_satisfier_for_map_key_to_ecdsa_sig {
+    ($(#[$($attr:meta)*])* impl Satisfier<Pk> for $map:ident<$key:ty, $val:ty>) => {
+        $(#[$($attr)*])*
+        impl<Pk: MiniscriptKey + ToPublicKey> Satisfier<Pk>
+            for $map<Pk, bitcoin::ecdsa::Signature>
+        {
+            fn lookup_ecdsa_sig(&self, key: &Pk) -> Option<bitcoin::ecdsa::Signature> {
+                self.get(key).copied()
+            }
+        }
+    };
 }
 
-impl<Pk: MiniscriptKey + ToPublicKey> Satisfier<Pk>
-    for HashMap<(Pk, TapLeafHash), bitcoin::taproot::Signature>
-{
-    fn lookup_tap_leaf_script_sig(
-        &self,
-        key: &Pk,
-        h: &TapLeafHash,
-    ) -> Option<bitcoin::taproot::Signature> {
-        // Unfortunately, there is no way to get a &(a, b) from &a and &b without allocating
-        // If we change the signature the of lookup_tap_leaf_script_sig to accept a tuple. We would
-        // face the same problem while satisfying PkK.
-        // We use this signature to optimize for the psbt common use case.
-        self.get(&(key.clone(), *h)).copied()
-    }
+impl_satisfier_for_map_key_to_ecdsa_sig! {
+    impl Satisfier<Pk> for BTreeMap<Pk, bitcoin::ecdsa::Signature>
 }
 
-impl<Pk: MiniscriptKey + ToPublicKey> Satisfier<Pk>
-    for HashMap<hash160::Hash, (Pk, bitcoin::ecdsa::Signature)>
-where
-    Pk: MiniscriptKey + ToPublicKey,
-{
-    fn lookup_ecdsa_sig(&self, key: &Pk) -> Option<bitcoin::ecdsa::Signature> {
-        self.get(&key.to_pubkeyhash(SigType::Ecdsa)).map(|x| x.1)
-    }
+impl_satisfier_for_map_key_to_ecdsa_sig! {
+    #[cfg(feature = "std")]
+    impl Satisfier<Pk> for HashMap<Pk, bitcoin::ecdsa::Signature>
+}
 
-    fn lookup_raw_pkh_pk(&self, pk_hash: &hash160::Hash) -> Option<bitcoin::PublicKey> {
-        self.get(pk_hash).map(|x| x.0.to_public_key())
-    }
+macro_rules! impl_satisfier_for_map_key_hash_to_taproot_sig {
+    ($(#[$($attr:meta)*])* impl Satisfier<Pk> for $map:ident<$key:ty, $val:ty>) => {
+        $(#[$($attr)*])*
+        impl<Pk: MiniscriptKey + ToPublicKey> Satisfier<Pk>
+            for $map<(Pk, TapLeafHash), bitcoin::taproot::Signature>
+        {
+            fn lookup_tap_leaf_script_sig(
+                &self,
+                key: &Pk,
+                h: &TapLeafHash,
+            ) -> Option<bitcoin::taproot::Signature> {
+                // Unfortunately, there is no way to get a &(a, b) from &a and &b without allocating
+                // If we change the signature the of lookup_tap_leaf_script_sig to accept a tuple. We would
+                // face the same problem while satisfying PkK.
+                // We use this signature to optimize for the psbt common use case.
+                self.get(&(key.clone(), *h)).copied()
+            }
+        }
+    };
+}
 
-    fn lookup_raw_pkh_ecdsa_sig(
-        &self,
-        pk_hash: &hash160::Hash,
-    ) -> Option<(bitcoin::PublicKey, bitcoin::ecdsa::Signature)> {
-        self.get(pk_hash)
-            .map(|&(ref pk, sig)| (pk.to_public_key(), sig))
-    }
+impl_satisfier_for_map_key_hash_to_taproot_sig! {
+    impl Satisfier<Pk> for BTreeMap<(Pk, TapLeafHash), bitcoin::taproot::Signature>
 }
 
-impl<Pk: MiniscriptKey + ToPublicKey> Satisfier<Pk>
-    for HashMap<(hash160::Hash, TapLeafHash), (Pk, bitcoin::taproot::Signature)>
-where
-    Pk: MiniscriptKey + ToPublicKey,
-{
-    fn lookup_tap_leaf_script_sig(
-        &self,
-        key: &Pk,
-        h: &TapLeafHash,
-    ) -> Option<bitcoin::taproot::Signature> {
-        self.get(&(key.to_pubkeyhash(SigType::Schnorr), *h))
-            .map(|x| x.1)
-    }
+impl_satisfier_for_map_key_hash_to_taproot_sig! {
+    #[cfg(feature = "std")]
+    impl Satisfier<Pk> for HashMap<(Pk, TapLeafHash), bitcoin::taproot::Signature>
+}
 
-    fn lookup_raw_pkh_tap_leaf_script_sig(
-        &self,
-        pk_hash: &(hash160::Hash, TapLeafHash),
-    ) -> Option<(XOnlyPublicKey, bitcoin::taproot::Signature)> {
-        self.get(pk_hash)
-            .map(|&(ref pk, sig)| (pk.to_x_only_pubkey(), sig))
-    }
+macro_rules! impl_satisfier_for_map_hash_to_key_ecdsa_sig {
+    ($(#[$($attr:meta)*])* impl Satisfier<Pk> for $map:ident<$key:ty, $val:ty>) => {
+        $(#[$($attr)*])*
+        impl<Pk: MiniscriptKey + ToPublicKey> Satisfier<Pk>
+            for $map<hash160::Hash, (Pk, bitcoin::ecdsa::Signature)>
+        where
+            Pk: MiniscriptKey + ToPublicKey,
+        {
+            fn lookup_ecdsa_sig(&self, key: &Pk) -> Option<bitcoin::ecdsa::Signature> {
+                self.get(&key.to_pubkeyhash(SigType::Ecdsa)).map(|x| x.1)
+            }
+
+            fn lookup_raw_pkh_pk(&self, pk_hash: &hash160::Hash) -> Option<bitcoin::PublicKey> {
+                self.get(pk_hash).map(|x| x.0.to_public_key())
+            }
+
+            fn lookup_raw_pkh_ecdsa_sig(
+                &self,
+                pk_hash: &hash160::Hash,
+            ) -> Option<(bitcoin::PublicKey, bitcoin::ecdsa::Signature)> {
+                self.get(pk_hash)
+                    .map(|&(ref pk, sig)| (pk.to_public_key(), sig))
+            }
+        }
+    };
+}
+
+impl_satisfier_for_map_hash_to_key_ecdsa_sig! {
+    impl Satisfier<Pk> for BTreeMap<hash160::Hash, (Pk, bitcoin::ecdsa::Signature)>
+}
+
+impl_satisfier_for_map_hash_to_key_ecdsa_sig! {
+    #[cfg(feature = "std")]
+    impl Satisfier<Pk> for HashMap<hash160::Hash, (Pk, bitcoin::ecdsa::Signature)>
+}
+
+macro_rules! impl_satisfier_for_map_hash_tapleafhash_to_key_taproot_sig {
+    ($(#[$($attr:meta)*])* impl Satisfier<Pk> for $map:ident<$key:ty, $val:ty>) => {
+        $(#[$($attr)*])*
+        impl<Pk: MiniscriptKey + ToPublicKey> Satisfier<Pk>
+            for $map<(hash160::Hash, TapLeafHash), (Pk, bitcoin::taproot::Signature)>
+        where
+            Pk: MiniscriptKey + ToPublicKey,
+        {
+            fn lookup_tap_leaf_script_sig(
+                &self,
+                key: &Pk,
+                h: &TapLeafHash,
+            ) -> Option<bitcoin::taproot::Signature> {
+                self.get(&(key.to_pubkeyhash(SigType::Schnorr), *h))
+                    .map(|x| x.1)
+            }
+
+            fn lookup_raw_pkh_tap_leaf_script_sig(
+                &self,
+                pk_hash: &(hash160::Hash, TapLeafHash),
+            ) -> Option<(XOnlyPublicKey, bitcoin::taproot::Signature)> {
+                self.get(pk_hash)
+                    .map(|&(ref pk, sig)| (pk.to_x_only_pubkey(), sig))
+            }
+        }
+    };
+}
+
+impl_satisfier_for_map_hash_tapleafhash_to_key_taproot_sig! {
+    impl Satisfier<Pk> for BTreeMap<(hash160::Hash, TapLeafHash), (Pk, bitcoin::taproot::Signature)>
+}
+
+impl_satisfier_for_map_hash_tapleafhash_to_key_taproot_sig! {
+    #[cfg(feature = "std")]
+    impl Satisfier<Pk> for HashMap<(hash160::Hash, TapLeafHash), (Pk, bitcoin::taproot::Signature)>
 }
 
 impl<'a, Pk: MiniscriptKey + ToPublicKey, S: Satisfier<Pk>> Satisfier<Pk> for &'a S {
diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs
@@ -1393,11 +1393,12 @@ mod tests {
         };
         let sigvec = bitcoinsig.to_vec();
 
-        let no_sat = HashMap::<bitcoin::PublicKey, bitcoin::ecdsa::Signature>::new();
-        let mut left_sat = HashMap::<bitcoin::PublicKey, bitcoin::ecdsa::Signature>::new();
-        let mut right_sat =
-            HashMap::<hashes::hash160::Hash, (bitcoin::PublicKey, bitcoin::ecdsa::Signature)>::new(
-            );
+        let no_sat = BTreeMap::<bitcoin::PublicKey, bitcoin::ecdsa::Signature>::new();
+        let mut left_sat = BTreeMap::<bitcoin::PublicKey, bitcoin::ecdsa::Signature>::new();
+        let mut right_sat = BTreeMap::<
+            hashes::hash160::Hash,
+            (bitcoin::PublicKey, bitcoin::ecdsa::Signature),
+        >::new();
 
         for i in 0..5 {
             left_sat.insert(keys[i], bitcoinsig);
diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs
@@ -327,7 +327,7 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
             let mut prob = 0.;
             let semantic_policy = self.lift()?;
             let concrete_keys = self.keys();
-            let key_prob_map: HashMap<_, _> = self
+            let key_prob_map: BTreeMap<_, _> = self
                 .to_tapleaf_prob_vec(1.0)
                 .into_iter()
                 .filter(|(_, ref pol)| matches!(*pol, Concrete::Key(..)))
@@ -347,7 +347,7 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
                                 internal_key = Some(key.clone());
                             }
                         }
-                        None => return Err(errstr("Key should have existed in the HashMap!")),
+                        None => return Err(errstr("Key should have existed in the BTreeMap!")),
                     }
                 }
             }
@@ -563,7 +563,7 @@ impl<Pk: MiniscriptKey> PolicyArc<Pk> {
         // owing to the current [policy element enumeration algorithm][`Policy::enumerate_pol`],
         // two passes of the algorithm might result in same sub-policy showing up. Currently, we
         // merge the nodes by adding up the corresponding probabilities for the same policy.
-        let mut pol_prob_map = HashMap::<Arc<Self>, OrdF64>::new();
+        let mut pol_prob_map = BTreeMap::<Arc<Self>, OrdF64>::new();
 
         let arc_self = Arc::new(self);
         tapleaf_prob_vec.insert((Reverse(OrdF64(prob)), Arc::clone(&arc_self)));
@@ -791,7 +791,7 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
     pub fn check_duplicate_keys(&self) -> Result<(), PolicyError> {
         let pks = self.keys();
         let pks_len = pks.len();
-        let unique_pks_len = pks.into_iter().collect::<HashSet<_>>().len();
+        let unique_pks_len = pks.into_iter().collect::<BTreeSet<_>>().len();
 
         if pks_len > unique_pks_len {
             Err(PolicyError::DuplicatePubKeys)
diff --git a/src/psbt/finalizer.rs b/src/psbt/finalizer.rs
@@ -36,18 +36,16 @@ fn construct_tap_witness(
 ) -> Result<Vec<Vec<u8>>, InputError> {
     // When miniscript tries to finalize the PSBT, it doesn't have the full descriptor (which contained a pkh() fragment)
     // and instead resorts to parsing the raw script sig, which is translated into a "expr_raw_pkh" internally.
-    let mut hash_map: BTreeMap<hash160::Hash, bitcoin::key::XOnlyPublicKey> = BTreeMap::new();
+    let mut map: BTreeMap<hash160::Hash, bitcoin::key::XOnlyPublicKey> = BTreeMap::new();
     let psbt_inputs = &sat.psbt.inputs;
     for psbt_input in psbt_inputs {
         // We need to satisfy or dissatisfy any given key. `tap_key_origin` is the only field of PSBT Input which consist of
         // all the keys added on a descriptor and thus we get keys from it.
         let public_keys = psbt_input.tap_key_origins.keys();
         for key in public_keys {
             let bitcoin_key = *key;
-            // Convert PubKeyHash into Hash::hash160
             let hash = bitcoin_key.to_pubkeyhash(SigType::Schnorr);
-            // Insert pair in HashMap
-            hash_map.insert(hash, bitcoin_key);
+            map.insert(hash, bitcoin_key);
         }
     }
     assert!(spk.is_v1_p2tr());
@@ -72,7 +70,7 @@ fn construct_tap_witness(
                 script,
                 &ExtParams::allow_all(),
             ) {
-                Ok(ms) => ms.substitute_raw_pkh(&hash_map),
+                Ok(ms) => ms.substitute_raw_pkh(&map),
                 Err(..) => continue, // try another script
             };
             let mut wit = if allow_mall {
@@ -142,19 +140,15 @@ pub(super) fn prevouts(psbt: &Psbt) -> Result<Vec<&bitcoin::TxOut>, super::Error
 // we want to move the script is probably already created
 // and we want to satisfy it in any way possible.
 fn get_descriptor(psbt: &Psbt, index: usize) -> Result<Descriptor<PublicKey>, InputError> {
-    // Create a HashMap <Hash(Pk),Pk>
-    let mut hash_map: BTreeMap<hash160::Hash, PublicKey> = BTreeMap::new();
+    let mut map: BTreeMap<hash160::Hash, PublicKey> = BTreeMap::new();
     let psbt_inputs = &psbt.inputs;
     for psbt_input in psbt_inputs {
         // Use BIP32 Derviation to get set of all possible keys.
         let public_keys = psbt_input.bip32_derivation.keys();
         for key in public_keys {
-            // Convert bitcoin::secp256k1::PublicKey into just bitcoin::PublicKey
             let bitcoin_key = bitcoin::PublicKey::new(*key);
-            // Convert PubKeyHash into Hash::hash160
             let hash = bitcoin_key.pubkey_hash().to_raw_hash();
-            // Insert pair in HashMap
-            hash_map.insert(hash, bitcoin_key);
+            map.insert(hash, bitcoin_key);
         }
     }
 
@@ -214,7 +208,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result<Descriptor<PublicKey>, In
                 witness_script,
                 &ExtParams::allow_all(),
             )?;
-            Ok(Descriptor::new_wsh(ms.substitute_raw_pkh(&hash_map))?)
+            Ok(Descriptor::new_wsh(ms.substitute_raw_pkh(&map))?)
         } else {
             Err(InputError::MissingWitnessScript)
         }
@@ -241,7 +235,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result<Descriptor<PublicKey>, In
                             witness_script,
                             &ExtParams::allow_all(),
                         )?;
-                        Ok(Descriptor::new_sh_wsh(ms.substitute_raw_pkh(&hash_map))?)
+                        Ok(Descriptor::new_sh_wsh(ms.substitute_raw_pkh(&map))?)
                     } else {
                         Err(InputError::MissingWitnessScript)
                     }
@@ -285,7 +279,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result<Descriptor<PublicKey>, In
             &script_pubkey,
             &ExtParams::allow_all(),
         )?;
-        Ok(Descriptor::new_bare(ms.substitute_raw_pkh(&hash_map))?)
+        Ok(Descriptor::new_bare(ms.substitute_raw_pkh(&map))?)
     }
 }
 

Original file line number	Diff line number	Diff line change
`@@ -213,7 +213,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {`
`213`	`213`	`// to have an iterator`
`214`	`214`	`let all_pkhs_len = self.iter_pk().count();`
`215`	`215`
`216`		`- let unique_pkhs_len = self.iter_pk().collect::<HashSet<_>>().len();`
	`216`	`+ let unique_pkhs_len = self.iter_pk().collect::<BTreeSet<_>>().len();`
`217`	`217`
`218`	`218`	`unique_pkhs_len != all_pkhs_len`
`219`	`219`	`}`