Merge rust-bitcoin#4157: Enforce MAX_MONEY invariant in amount types

ab4ea7c Enforce the MAX_MONEY invariant in amount types (Tobin C. Harding)

Pull request description:

  Enforcing the `MAX_MONEY` invariant is quite involved because it means multiple things:

  - Constructing amounts is now fallible
  - Converting from unsigned to signed is now infallible
  - Taking the absolute value is now infallible
  - Integer overflow is eliminated in various places

  Details:
  - Update `from_sat` to check the invariant
  - Fix all docs including examples
  - Use the unchecked constructor in test code
  - Comment any other use of the unchecked constructor
  - Deprecate `unchecked_abs`
  - Fail serde (using the horrible string error variant)
  - Try not to use the unchecked constructor in rustdocs, no need to encourage unsuspecting users to use it.
  - Use `?` in rustdoc examples (required by Rust API guidlines)
  - Remove `TryFrom<Amount> for SignedAmount` because the conversion is now infallible. Add a `From` impl.
  - Fix the arbitrary impls
  - Maintain correct formatting
  - Remove private `check_max` function as its no longer needed

  Close rust-bitcoin#620

ACKs for top commit:
  apoelstra:
    ACK ab4ea7c; successfully ran local tests

Tree-SHA512: bec963d8ea69e202f399cd19bca864b06f3e86323d376c2d2126d74093598f8bbbf19792b2327dba0862ef6f0201202778014a2be7a14991f02917d8ca312afb

Author: apoelstra

bitcoin/src/blockdata/script/borrowed.rs

@@ -413,7 +413,7 @@ crate::internal_macros::define_extension_trait! {
                         // Note: We ensure the division happens at the end, since Core performs the division at the end.
                         //       This will make sure none of the implicit floor operations mess with the value.
 
-            Some(Amount::from_sat(sats))
+            Amount::from_sat(sats).ok()
         }
 
         fn count_sigops_internal(&self, accurate: bool) -> usize {

bitcoin/src/lib.rs

@@ -198,7 +198,7 @@ pub mod amount {
     //! This module mainly introduces the [`Amount`] and [`SignedAmount`] types.
     //! We refer to the documentation on the types for more information.
 
-    use crate::consensus::{encode, Decodable, Encodable};
+    use crate::consensus::{self, encode, Decodable, Encodable};
     use crate::io::{BufRead, Write};
 
     #[rustfmt::skip]            // Keep public re-exports separate.
@@ -215,7 +215,9 @@ pub mod amount {
     impl Decodable for Amount {
         #[inline]
         fn consensus_decode<R: BufRead + ?Sized>(r: &mut R) -> Result<Self, encode::Error> {
-            Ok(Amount::from_sat(Decodable::consensus_decode(r)?))
+            Amount::from_sat(Decodable::consensus_decode(r)?).map_err(|_| {
+                consensus::parse_failed_error("amount is greater than Amount::MAX_MONEY")
+            })
         }
     }
 

bitcoin/src/psbt/mod.rs

@@ -1268,7 +1268,7 @@ mod tests {
                     witness: Witness::default(),
                 }],
                 output: vec![TxOut {
-                    value: Amount::from_sat(output),
+                    value: Amount::from_sat(output).unwrap(),
                     script_pubkey: ScriptBuf::from_hex(
                         "a9143545e6e33b832c47050f24d3eeb93c9c03948bc787",
                     )
@@ -1282,7 +1282,7 @@ mod tests {
 
             inputs: vec![Input {
                 witness_utxo: Some(TxOut {
-                    value: Amount::from_sat(input),
+                    value: Amount::from_sat(input).unwrap(),
                     script_pubkey: ScriptBuf::from_hex(
                         "a914339725ba21efd62ac753a9bcd067d6c7a6a39d0587",
                     )

bitcoin/tests/psbt-sign-taproot.rs

@@ -205,7 +205,7 @@ fn create_psbt_for_taproot_key_path_spend(
 ) -> Psbt {
     let send_value = 6400;
     let out_puts = vec![TxOut {
-        value: Amount::from_sat(send_value),
+        value: Amount::from_sat(send_value).unwrap(),
         script_pubkey: to_address.script_pubkey(),
     }];
     let prev_tx_id = "06980ca116f74c7845a897461dd0e1d15b114130176de5004957da516b4dee3a";
@@ -243,7 +243,7 @@ fn create_psbt_for_taproot_key_path_spend(
     let mut input = Input {
         witness_utxo: {
             let script_pubkey = from_address.script_pubkey();
-            Some(TxOut { value: Amount::from_sat(utxo_value), script_pubkey })
+            Some(TxOut { value: Amount::from_sat(utxo_value).unwrap(), script_pubkey })
         },
         tap_key_origins: origins,
         ..Default::default()
@@ -283,7 +283,7 @@ fn create_psbt_for_taproot_script_path_spend(
     let mfp = "73c5da0a";
 
     let out_puts = vec![TxOut {
-        value: Amount::from_sat(send_value),
+        value: Amount::from_sat(send_value).unwrap(),
         script_pubkey: to_address.script_pubkey(),
     }];
     let prev_tx_id = "9d7c6770fca57285babab60c51834cfcfd10ad302119cae842d7216b4ac9a376";
@@ -322,7 +322,7 @@ fn create_psbt_for_taproot_script_path_spend(
     let mut input = Input {
         witness_utxo: {
             let script_pubkey = from_address.script_pubkey();
-            Some(TxOut { value: Amount::from_sat(utxo_value), script_pubkey })
+            Some(TxOut { value: Amount::from_sat(utxo_value).unwrap(), script_pubkey })
         },
         tap_key_origins: origins,
         tap_scripts,

primitives/src/transaction.rs

@@ -670,7 +670,7 @@ mod tests {
             witness: Witness::new(),
         };
 
-        let txout = TxOut { value: Amount::from_sat(123_456_789), script_pubkey: ScriptBuf::new() };
+        let txout = TxOut { value: Amount::from_sat(123_456_789).unwrap(), script_pubkey: ScriptBuf::new() };
 
         let tx_orig = Transaction {
             version: Version::ONE,
@@ -682,7 +682,7 @@ mod tests {
         // Test changing the transaction
         let mut tx = tx_orig.clone();
         tx.inputs_mut()[0].previous_output.txid = Txid::from_byte_array([0xFF; 32]);
-        tx.outputs_mut()[0].value = Amount::from_sat(987_654_321);
+        tx.outputs_mut()[0].value = Amount::from_sat(987_654_321).unwrap();
         assert_eq!(tx.inputs()[0].previous_output.txid.to_byte_array(), [0xFF; 32]);
         assert_eq!(tx.outputs()[0].value.to_sat(), 987_654_321);
 

units/src/amount/mod.rs

@@ -59,7 +59,7 @@ pub(crate) use self::result::OptionExt;
 /// # Examples
 ///
 /// ```
-/// # use bitcoin_units::Amount;
+/// # use bitcoin_units::{amount, Amount};
 ///
 /// let equal = [
 ///     ("1 BTC", 100_000_000),
@@ -72,9 +72,10 @@ pub(crate) use self::result::OptionExt;
 /// for (string, sats) in equal {
 ///     assert_eq!(
 ///         string.parse::<Amount>().expect("valid bitcoin amount string"),
-///         Amount::from_sat(sats),
+///         Amount::from_sat(sats)?,
 ///     )
 /// }
+/// # Ok::<_, amount::OutOfRangeError>(())
 /// ```
 #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)]
 #[non_exhaustive]

units/src/amount/result.rs

@@ -328,7 +328,9 @@ crate::internal_macros::impl_op_for_references! {
 impl ops::Neg for SignedAmount {
     type Output = Self;
 
-    fn neg(self) -> Self::Output { Self::from_sat(self.to_sat().neg()) }
+    fn neg(self) -> Self::Output {
+        Self::from_sat(self.to_sat().neg()).expect("all +ve and -ve values are valid")
+    }
 }
 
 impl core::iter::Sum<NumOpResult<Amount>> for NumOpResult<Amount> {

units/src/amount/serde.rs

@@ -92,7 +92,8 @@ impl SerdeAmount for Amount {
         u64::serialize(&self.to_sat(), s)
     }
     fn des_sat<'d, D: Deserializer<'d>>(d: D, _: private::Token) -> Result<Self, D::Error> {
-        Ok(Amount::from_sat(u64::deserialize(d)?))
+        use serde::de::Error;
+        Amount::from_sat(u64::deserialize(d)?).map_err(D::Error::custom)
     }
     #[cfg(feature = "alloc")]
     fn ser_btc<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error> {
@@ -137,7 +138,8 @@ impl SerdeAmount for SignedAmount {
         i64::serialize(&self.to_sat(), s)
     }
     fn des_sat<'d, D: Deserializer<'d>>(d: D, _: private::Token) -> Result<Self, D::Error> {
-        Ok(SignedAmount::from_sat(i64::deserialize(d)?))
+        use serde::de::Error;
+        SignedAmount::from_sat(i64::deserialize(d)?).map_err(D::Error::custom)
     }
     #[cfg(feature = "alloc")]
     fn ser_btc<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error> {