Merge rust-bitcoin/rust-bitcoin#752: Make Map trait private

dfd8924 Remove insert_pair from Map trait (Tobin Harding)
ad75d51 Make Map trait private to psbt module (Tobin Harding)
53225c0 Improve docs in map module (Tobin Harding)
92059c2 Add full stops to rustdocs (Tobin Harding)
11c046b Refactor match arms (Tobin Harding)
e6af569 Move imports to top of file (Tobin Harding)

Pull request description:

  The `Map` method `insert_pair` is never called for `PartiallySignedTransaction`. Separate the method into its own trait (`Insert`) and delete dead code. The dead code contains the alleged bug in #576.

  - Patch 1: Preparatory cleanup
  - Patch 2: Preparatory refactor
  - Patch 3 and 4: Improve docs in the module that this PR touches
  - Patch 5: Make `Map` trait private to the `psbt` module
  - ~Patch 6: Make `concensus_decode_global` method into a function~
  - Patch ~7~ 6: Pull `insert_pair` method out of `Map` trait into newly create `Insert` trait

  Resolves: rust-bitcoin/rust-bitcoin#576

  (Title of PR is `Make Map trait private` because that is the API break.)

ACKs for top commit:
  dr-orlovsky:
    ACK dfd8924
  apoelstra:
    ACK dfd8924

Tree-SHA512: 1a78294bc8a455552d93caf64db697f886345ba979f574abad55820415958fee1c2dd16945f4eafdbe542fa202cb7e08618aa137ec7ee22b3c9dac5df0328157

Author: ChallengeDev210

src/util/psbt/macros.rs

@@ -75,7 +75,7 @@ macro_rules! impl_psbtmap_consensus_decoding {
 
                 loop {
                     match $crate::consensus::Decodable::consensus_decode(&mut d) {
-                        Ok(pair) => $crate::util::psbt::Map::insert_pair(&mut rv, pair)?,
+                        Ok(pair) => rv.insert_pair(pair)?,
                         Err($crate::consensus::encode::Error::Psbt($crate::util::psbt::Error::NoMorePairs)) => return Ok(rv),
                         Err(e) => return Err(e),
                     }

src/util/psbt/map/global.rs

@@ -37,27 +37,6 @@ const PSBT_GLOBAL_VERSION: u8 = 0xFB;
 const PSBT_GLOBAL_PROPRIETARY: u8 = 0xFC;
 
 impl Map for PartiallySignedTransaction {
-    fn insert_pair(&mut self, pair: raw::Pair) -> Result<(), encode::Error> {
-        let raw::Pair {
-            key: raw_key,
-            value: raw_value,
-        } = pair;
-
-        match raw_key.type_value {
-            PSBT_GLOBAL_UNSIGNED_TX => return Err(Error::DuplicateKey(raw_key).into()),
-            PSBT_GLOBAL_PROPRIETARY => match self.proprietary.entry(raw::ProprietaryKey::from_key(raw_key.clone())?) {
-                btree_map::Entry::Vacant(empty_key) => {empty_key.insert(raw_value);},
-                btree_map::Entry::Occupied(_) => return Err(Error::DuplicateKey(raw_key).into()),
-            }
-            _ => match self.unknown.entry(raw_key) {
-                btree_map::Entry::Vacant(empty_key) => {empty_key.insert(raw_value);},
-                btree_map::Entry::Occupied(k) => return Err(Error::DuplicateKey(k.key().clone()).into()),
-            }
-        }
-
-        Ok(())
-    }
-
     fn get_pairs(&self) -> Result<Vec<raw::Pair>, io::Error> {
         let mut rv: Vec<raw::Pair> = Default::default();
 
@@ -278,11 +257,15 @@ impl PartiallySignedTransaction {
                             }
                         }
                         PSBT_GLOBAL_PROPRIETARY => match proprietary.entry(raw::ProprietaryKey::from_key(pair.key.clone())?) {
-                            btree_map::Entry::Vacant(empty_key) => {empty_key.insert(pair.value);},
+                            btree_map::Entry::Vacant(empty_key) => {
+                                empty_key.insert(pair.value);
+                            },
                             btree_map::Entry::Occupied(_) => return Err(Error::DuplicateKey(pair.key).into()),
                         }
                         _ => match unknowns.entry(pair.key) {
-                            btree_map::Entry::Vacant(empty_key) => {empty_key.insert(pair.value);},
+                            btree_map::Entry::Vacant(empty_key) => {
+                                empty_key.insert(pair.value);
+                            },
                             btree_map::Entry::Occupied(k) => return Err(Error::DuplicateKey(k.key().clone()).into()),
                         }
                     }

src/util/psbt/map/input.rs

@@ -108,32 +108,32 @@ pub struct Input {
     /// other scripts necessary for this input to pass validation.
     pub final_script_witness: Option<Witness>,
     /// TODO: Proof of reserves commitment
-    /// RIPEMD160 hash to preimage map
+    /// RIPEMD160 hash to preimage map.
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_byte_values"))]
     pub ripemd160_preimages: BTreeMap<ripemd160::Hash, Vec<u8>>,
-    /// SHA256 hash to preimage map
+    /// SHA256 hash to preimage map.
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_byte_values"))]
     pub sha256_preimages: BTreeMap<sha256::Hash, Vec<u8>>,
-    /// HSAH160 hash to preimage map
+    /// HSAH160 hash to preimage map.
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_byte_values"))]
     pub hash160_preimages: BTreeMap<hash160::Hash, Vec<u8>>,
-    /// HAS256 hash to preimage map
+    /// HAS256 hash to preimage map.
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_byte_values"))]
     pub hash256_preimages: BTreeMap<sha256d::Hash, Vec<u8>>,
-    /// Serialized schnorr signature with sighash type for key spend
+    /// Serialized schnorr signature with sighash type for key spend.
     pub tap_key_sig: Option<SchnorrSig>,
-    /// Map of <xonlypubkey>|<leafhash> with signature
+    /// Map of <xonlypubkey>|<leafhash> with signature.
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_as_seq"))]
     pub tap_script_sigs: BTreeMap<(XOnlyPublicKey, TapLeafHash), SchnorrSig>,
-    /// Map of Control blocks to Script version pair
+    /// Map of Control blocks to Script version pair.
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_as_seq"))]
     pub tap_scripts: BTreeMap<ControlBlock, (Script, LeafVersion)>,
-    /// Map of tap root x only keys to origin info and leaf hashes contained in it
+    /// Map of tap root x only keys to origin info and leaf hashes contained in it.
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_as_seq"))]
     pub tap_key_origins: BTreeMap<XOnlyPublicKey, (Vec<TapLeafHash>, KeySource)>,
-    /// Taproot Internal key
+    /// Taproot Internal key.
     pub tap_internal_key : Option<XOnlyPublicKey>,
-    /// Taproot Merkle root
+    /// Taproot Merkle root.
     pub tap_merkle_root : Option<TapBranchHash>,
     /// Proprietary key-value pairs for this input.
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_as_seq_byte_values"))]
@@ -189,8 +189,8 @@ impl PsbtSigHashType {
     }
 }
 
-impl Map for Input {
-    fn insert_pair(&mut self, pair: raw::Pair) -> Result<(), encode::Error> {
+impl Input {
+    pub(super) fn insert_pair(&mut self, pair: raw::Pair) -> Result<(), encode::Error> {
         let raw::Pair {
             key: raw_key,
             value: raw_value,
@@ -284,23 +284,28 @@ impl Map for Input {
                     self.tap_merkle_root <= <raw_key: _>|< raw_value: TapBranchHash>
                 }
             }
-            PSBT_IN_PROPRIETARY => match self.proprietary.entry(raw::ProprietaryKey::from_key(raw_key.clone())?) {
-                btree_map::Entry::Vacant(empty_key) => {empty_key.insert(raw_value);},
-                btree_map::Entry::Occupied(_) => return Err(Error::DuplicateKey(raw_key).into()),
+            PSBT_IN_PROPRIETARY => {
+                let key = raw::ProprietaryKey::from_key(raw_key.clone())?;
+                match self.proprietary.entry(key) {
+                    btree_map::Entry::Vacant(empty_key) => {
+                        empty_key.insert(raw_value);
+                    },
+                    btree_map::Entry::Occupied(_) => return Err(Error::DuplicateKey(raw_key).into()),
+                }
             }
             _ => match self.unknown.entry(raw_key) {
                 btree_map::Entry::Vacant(empty_key) => {
                     empty_key.insert(raw_value);
                 }
-                btree_map::Entry::Occupied(k) => {
-                    return Err(Error::DuplicateKey(k.key().clone()).into())
-                }
+                btree_map::Entry::Occupied(k) => return Err(Error::DuplicateKey(k.key().clone()).into()),
             },
         }
 
         Ok(())
     }
+}
 
+impl Map for Input {
     fn get_pairs(&self) -> Result<Vec<raw::Pair>, io::Error> {
         let mut rv: Vec<raw::Pair> = Default::default();
 

src/util/psbt/map/mod.rs

@@ -20,18 +20,22 @@ use consensus::encode;
 use util::psbt;
 use util::psbt::raw;
 
-/// A trait that describes a PSBT key-value map.
-pub trait Map {
-    /// Attempt to insert a key-value pair.
-    fn insert_pair(&mut self, pair: raw::Pair) -> Result<(), encode::Error>;
+mod global;
+mod input;
+mod output;
 
+pub use self::input::{Input, PsbtSigHashType};
+pub use self::output::{Output, TapTree};
+
+/// A trait that describes a PSBT key-value map.
+pub(super) trait Map {
     /// Attempt to get all key-value pairs.
     fn get_pairs(&self) -> Result<Vec<raw::Pair>, io::Error>;
 
     /// Attempt to merge with another key-value map of the same type.
     fn merge(&mut self, other: Self) -> Result<(), psbt::Error>;
 
-    /// Encodes map data with bitcoin consensus encoding
+    /// Encodes map data with bitcoin consensus encoding.
     fn consensus_encode_map<S: io::Write>(
         &self,
         mut s: S,
@@ -47,12 +51,3 @@ pub trait Map {
         Ok(len + encode::Encodable::consensus_encode(&0x00_u8, s)?)
     }
 }
-
-// place at end to pick up macros
-mod global;
-mod input;
-mod output;
-
-pub use self::input::{Input, PsbtSigHashType};
-pub use self::output::Output;
-pub use self::output::TapTree;

src/util/psbt/map/output.rs

@@ -58,11 +58,11 @@ pub struct Output {
     /// corresponding master key fingerprints and derivation paths.
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_as_seq"))]
     pub bip32_derivation: BTreeMap<secp256k1::PublicKey, KeySource>,
-    /// The internal pubkey
+    /// The internal pubkey.
     pub tap_internal_key: Option<XOnlyPublicKey>,
-    /// Taproot Output tree
+    /// Taproot Output tree.
     pub tap_tree: Option<TapTree>,
-    /// Map of tap root x only keys to origin info and leaf hashes contained in it
+    /// Map of tap root x only keys to origin info and leaf hashes contained in it.
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_as_seq"))]
     pub tap_key_origins: BTreeMap<XOnlyPublicKey, (Vec<TapLeafHash>, KeySource)>,
     /// Proprietary key-value pairs for this output.
@@ -79,7 +79,7 @@ pub struct Output {
     pub unknown: BTreeMap<raw::Key, Vec<u8>>,
 }
 
-/// Taproot Tree representing a finalized [`TaprootBuilder`] (a complete binary tree)
+/// Taproot Tree representing a finalized [`TaprootBuilder`] (a complete binary tree).
 #[derive(Clone, Debug)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 pub struct TapTree(pub(crate) TaprootBuilder);
@@ -93,7 +93,7 @@ impl PartialEq for TapTree {
 impl Eq for TapTree {}
 
 impl TapTree {
-    // get the inner node info as the builder is finalized
+    /// Gets the inner node info as the builder is finalized.
     fn node_info(&self) -> &NodeInfo {
         // The builder algorithm invariant guarantees that is_complete builder
         // have only 1 element in branch and that is not None.
@@ -102,8 +102,10 @@ impl TapTree {
         self.0.branch()[0].as_ref().expect("from_inner only parses is_complete builders")
     }
 
-    /// Convert a [`TaprootBuilder`] into a tree if it is complete binary tree.
-    /// Returns the inner as Err if it is not a complete tree
+    /// Converts a [`TaprootBuilder`] into a tree if it is complete binary tree.
+    ///
+    /// # Return
+    /// A `TapTree` iff the `inner` builder is complete, otherwise return the inner as `Err`.
     pub fn from_inner(inner: TaprootBuilder) -> Result<Self, TaprootBuilder> {
         if inner.is_complete() {
             Ok(TapTree(inner))
@@ -112,15 +114,14 @@ impl TapTree {
         }
     }
 
-    /// Convert self into builder [`TaprootBuilder`]. The builder is guaranteed to
-    /// be finalized.
+    /// Converts self into builder [`TaprootBuilder`]. The builder is guaranteed to be finalized.
     pub fn into_inner(self) -> TaprootBuilder {
         self.0
     }
 }
 
-impl Map for Output {
-    fn insert_pair(&mut self, pair: raw::Pair) -> Result<(), encode::Error> {
+impl Output {
+    pub(super) fn insert_pair(&mut self, pair: raw::Pair) -> Result<(), encode::Error> {
         let raw::Pair {
             key: raw_key,
             value: raw_value,
@@ -142,9 +143,14 @@ impl Map for Output {
                     self.bip32_derivation <= <raw_key: secp256k1::PublicKey>|<raw_value: KeySource>
                 }
             }
-            PSBT_OUT_PROPRIETARY => match self.proprietary.entry(raw::ProprietaryKey::from_key(raw_key.clone())?) {
-                btree_map::Entry::Vacant(empty_key) => {empty_key.insert(raw_value);},
-                btree_map::Entry::Occupied(_) => return Err(Error::DuplicateKey(raw_key).into()),
+            PSBT_OUT_PROPRIETARY => {
+                let key = raw::ProprietaryKey::from_key(raw_key.clone())?;
+                match self.proprietary.entry(key) {
+                    btree_map::Entry::Vacant(empty_key) => {
+                        empty_key.insert(raw_value);
+                    },
+                    btree_map::Entry::Occupied(_) => return Err(Error::DuplicateKey(raw_key).into()),
+                }
             }
             PSBT_OUT_TAP_INTERNAL_KEY => {
                 impl_psbt_insert_pair! {
@@ -165,15 +171,15 @@ impl Map for Output {
                 btree_map::Entry::Vacant(empty_key) => {
                     empty_key.insert(raw_value);
                 }
-                btree_map::Entry::Occupied(k) => {
-                    return Err(Error::DuplicateKey(k.key().clone()).into())
-                }
-            },
+                btree_map::Entry::Occupied(k) => return Err(Error::DuplicateKey(k.key().clone()).into()),
+            }
         }
 
         Ok(())
     }
+}
 
+impl Map for Output {
     fn get_pairs(&self) -> Result<Vec<raw::Pair>, io::Error> {
         let mut rv: Vec<raw::Pair> = Default::default();
 

src/util/psbt/mod.rs

@@ -39,7 +39,8 @@ mod macros;
 pub mod serialize;
 
 mod map;
-pub use self::map::{Map, Input, Output, TapTree};
+pub use self::map::{Input, Output, TapTree};
+use self::map::Map;
 
 use util::bip32::{ExtendedPubKey, KeySource};