Add a proper error to Bitcoin's SignableTransaction::new

Also adds documentation to various parts of bitcoin.

Cargo.lock

@@ -6370,7 +6370,6 @@ version = "0.1.0"
 dependencies = [
  "async-trait",
  "bincode",
- "bitcoin",
  "bitcoin-serai",
  "dalek-ff-group",
  "env_logger",

coins/bitcoin/src/lib.rs

@@ -1,3 +1,6 @@
+/// The bitcoin Rust library.
+pub use bitcoin;
+
 /// Cryptographic helpers.
 pub mod crypto;
 /// BIP-340 Schnorr signature algorithm.

coins/bitcoin/src/wallet.rs

@@ -3,7 +3,9 @@ use std::{
   collections::HashMap,
 };
 
-use rand_core::RngCore;
+use thiserror::Error;
+
+use rand_core::{RngCore, CryptoRng};
 
 use transcript::{Transcript, RecommendedTranscript};
 
@@ -23,11 +25,20 @@ use bitcoin::{
 
 use crate::algorithm::Schnorr;
 
+#[rustfmt::skip]
+// https://github.com/bitcoin/bitcoin/blob/306ccd4927a2efe325c8d84be1bdb79edeb29b04/src/policy/policy.h#L27
+const MAX_STANDARD_TX_WEIGHT: u64 = 400_000;
+
+#[rustfmt::skip]
+//https://github.com/bitcoin/bitcoin/blob/a245429d680eb95cf4c0c78e58e63e3f0f5d979a/src/test/transaction_tests.cpp#L815-L816
+const DUST: u64 = 674;
+
 /// A spendable output.
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub struct SpendableOutput {
   /// The scalar offset to obtain the key usable to spend this output.
-  /// Enables HDKD systems.
+  ///
+  /// This field exists in order to support HDKD schemes.
   pub offset: Scalar,
   /// The output to spend.
   pub output: TxOut,
@@ -36,7 +47,7 @@ pub struct SpendableOutput {
 }
 
 impl SpendableOutput {
-  /// Obtain a unique ID for this output.
+  /// The unique ID for this output (TX ID and vout).
   pub fn id(&self) -> [u8; 36] {
     serialize(&self.outpoint).try_into().unwrap()
   }
@@ -67,52 +78,104 @@ impl SpendableOutput {
   }
 }
 
+#[derive(Clone, PartialEq, Eq, Debug, Error)]
+pub enum TransactionError {
+  #[error("no inputs were specified")]
+  NoInputs,
+  #[error("no outputs were created")]
+  NoOutputs,
+  #[error("a specified payment's amount was less than bitcoin's required minimum")]
+  DustPayment,
+  #[error("too much data was specified")]
+  TooMuchData,
+  #[error("not enough funds for these payments")]
+  NotEnoughFunds,
+  #[error("transaction was too large")]
+  TooLargeTransaction,
+}
+
 /// A signable transaction, clone-able across attempts.
 #[derive(Clone, PartialEq, Eq, Debug)]
-pub struct SignableTransaction(Transaction, Vec<Scalar>, Vec<TxOut>, u64);
+pub struct SignableTransaction {
+  tx: Transaction,
+  offsets: Vec<Scalar>,
+  prevouts: Vec<TxOut>,
+  needed_fee: u64,
+}
 
 impl SignableTransaction {
   fn calculate_weight(inputs: usize, payments: &[(Address, u64)], change: Option<&Address>) -> u64 {
+    // Expand this a full transaction in order to use the bitcoin library's weight function
     let mut tx = Transaction {
       version: 2,
       lock_time: PackedLockTime::ZERO,
       input: vec![
         TxIn {
+          // This is a fixed size
+          // See https://developer.bitcoin.org/reference/transactions.html#raw-transaction-format
           previous_output: OutPoint::default(),
+          // This is empty for a Taproot spend
           script_sig: Script::new(),
+          // This is fixed size, yet we do use Sequence::MAX
           sequence: Sequence::MAX,
+          // Our witnesses contains a single 64-byte signature
           witness: Witness::from_vec(vec![vec![0; 64]])
         };
         inputs
       ],
       output: payments
         .iter()
+        // The payment is a fixed size so we don't have to use it here
+        // The script pub key is not of a fixed size and does have to be used here
         .map(|payment| TxOut { value: payment.1, script_pubkey: payment.0.script_pubkey() })
         .collect(),
     };
     if let Some(change) = change {
+      // Use a 0 value since we're currently unsure what the change amount will be, and since
+      // the value is fixed size (so any value could be used here)
       tx.output.push(TxOut { value: 0, script_pubkey: change.script_pubkey() });
     }
     u64::try_from(tx.weight()).unwrap()
   }
 
-  pub fn fee(&self) -> u64 {
+  /// Returns the fee necessary for this transaction to achieve the fee rate specified at
-    self.3
+  /// construction.
+  ///
+  /// The actual fee this transaction will use is `sum(inputs) - sum(outputs)`.
+  pub fn needed_fee(&self) -> u64 {
+    self.needed_fee
   }
 
   /// Create a new SignableTransaction.
+  ///
+  /// If a change address is specified, any leftover funds will be sent to it if the leftover funds
+  /// exceed the minimum output amount. If a change address isn't specified, all leftover funds
+  /// will become part of the paid fee.
+  ///
+  /// If data is specified, an OP_RETURN output will be added with it.
   pub fn new(
     mut inputs: Vec<SpendableOutput>,
     payments: &[(Address, u64)],
     change: Option<Address>,
     data: Option<Vec<u8>>,
-    fee: u64,
+    fee_per_weight: u64,
-  ) -> Option<SignableTransaction> {
+  ) -> Result<SignableTransaction, TransactionError> {
-    if inputs.is_empty() ||
+    if inputs.is_empty() {
-      (payments.is_empty() && change.is_none()) ||
+      Err(TransactionError::NoInputs)?;
-      (data.as_ref().map(|data| data.len()).unwrap_or(0) > 80)
+    }
-    {
+
-      return None;
+    if payments.is_empty() && change.is_none() {
+      Err(TransactionError::NoOutputs)?;
+    }
+
+    for (_, amount) in payments {
+      if *amount < DUST {
+        Err(TransactionError::DustPayment)?;
+      }
+    }
+
+    if data.as_ref().map(|data| data.len()).unwrap_or(0) > 80 {
+      Err(TransactionError::TooMuchData)?;
     }
 
     let input_sat = inputs.iter().map(|input| input.output.value).sum::<u64>();
@@ -138,29 +201,44 @@ impl SignableTransaction {
       tx_outs.push(TxOut { value: 0, script_pubkey: Script::new_op_return(&data) })
     }
 
-    let mut actual_fee = fee * Self::calculate_weight(tx_ins.len(), payments, None);
+    let mut weight = Self::calculate_weight(tx_ins.len(), payments, None);
-    if input_sat < (payment_sat + actual_fee) {
+    let mut needed_fee = fee_per_weight * weight;
-      return None;
+    if input_sat < (payment_sat + needed_fee) {
+      Err(TransactionError::NotEnoughFunds)?;
     }
 
     // If there's a change address, check if there's change to give it
     if let Some(change) = change.as_ref() {
-      let fee_with_change = fee * Self::calculate_weight(tx_ins.len(), payments, Some(change));
+      let weight_with_change = Self::calculate_weight(tx_ins.len(), payments, Some(change));
+      let fee_with_change = fee_per_weight * weight_with_change;
       if let Some(value) = input_sat.checked_sub(payment_sat + fee_with_change) {
-        tx_outs.push(TxOut { value, script_pubkey: change.script_pubkey() });
+        if value >= DUST {
-        actual_fee = fee_with_change;
+          tx_outs.push(TxOut { value, script_pubkey: change.script_pubkey() });
+          weight = weight_with_change;
+          needed_fee = fee_with_change;
+        }
       }
     }
 
-    // TODO: Drop outputs which BTC will consider spam (outputs worth less than the cost to spend
+    if tx_outs.is_empty() {
-    // them)
+      Err(TransactionError::NoOutputs)?;
+    }
 
-    Some(SignableTransaction(
+    if weight > MAX_STANDARD_TX_WEIGHT {
-      Transaction { version: 2, lock_time: PackedLockTime::ZERO, input: tx_ins, output: tx_outs },
+      Err(TransactionError::TooLargeTransaction)?;
+    }
+
+    Ok(SignableTransaction {
+      tx: Transaction {
+        version: 2,
+        lock_time: PackedLockTime::ZERO,
+        input: tx_ins,
+        output: tx_outs,
+      },
       offsets,
-      inputs.drain(..).map(|input| input.output).collect(),
+      prevouts: inputs.drain(..).map(|input| input.output).collect(),
-      actual_fee,
+      needed_fee,
-    ))
+    })
   }
 
   /// Create a multisig machine for this transaction.
@@ -173,7 +251,7 @@ impl SignableTransaction {
     transcript.append_message(b"root_key", keys.group_key().to_encoded_point(true).as_bytes());
 
     // Transcript the inputs and outputs
-    let tx = &self.0;
+    let tx = &self.tx;
     for input in &tx.input {
       transcript.append_message(b"input_hash", input.previous_output.txid.as_hash().into_inner());
       transcript.append_message(b"input_output_index", input.previous_output.vout.to_le_bytes());
@@ -187,9 +265,10 @@ impl SignableTransaction {
     for i in 0 .. tx.input.len() {
       let mut transcript = transcript.clone();
       transcript.append_message(b"signing_input", u32::try_from(i).unwrap().to_le_bytes());
-      sigs.push(
+      sigs.push(AlgorithmMachine::new(
-        AlgorithmMachine::new(Schnorr::new(transcript), keys.clone().offset(self.1[i])).unwrap(),
+        Schnorr::new(transcript),
-      );
+        keys.clone().offset(self.offsets[i]),
+      ));
     }
 
     Ok(TransactionMachine { tx: self, sigs })
@@ -197,6 +276,9 @@ impl SignableTransaction {
 }
 
 /// A FROST signing machine to produce a Bitcoin transaction.
+///
+/// This does not support caching its preprocess. When sign is called, the message must be empty.
+/// This will panic if it isn't.
 pub struct TransactionMachine {
   tx: SignableTransaction,
   sigs: Vec<AlgorithmMachine<Secp256k1, Schnorr<RecommendedTranscript>>>,
@@ -207,7 +289,7 @@ impl PreprocessMachine for TransactionMachine {
   type Signature = Transaction;
   type SignMachine = TransactionSignMachine;
 
-  fn preprocess<R: RngCore + rand_core::CryptoRng>(
+  fn preprocess<R: RngCore + CryptoRng>(
     mut self,
     rng: &mut R,
   ) -> (Self::SignMachine, Self::Preprocess) {
@@ -266,9 +348,7 @@ impl SignMachine<Transaction> for TransactionSignMachine {
     msg: &[u8],
   ) -> Result<(TransactionSignatureMachine, Self::SignatureShare), FrostError> {
     if !msg.is_empty() {
-      Err(FrostError::InternalError(
+      panic!("message was passed to the TransactionMachine when it generates its own");
-        "message was passed to the TransactionMachine when it generates its own",
-      ))?;
     }
 
     let commitments = (0 .. self.sigs.len())
@@ -280,8 +360,9 @@ impl SignMachine<Transaction> for TransactionSignMachine {
       })
       .collect::<Vec<_>>();
 
-    let mut cache = SighashCache::new(&self.tx.0);
+    let mut cache = SighashCache::new(&self.tx.tx);
-    let prevouts = Prevouts::All(&self.tx.2);
+    // Sign committing to all inputs
+    let prevouts = Prevouts::All(&self.tx.prevouts);
 
     let mut shares = Vec::with_capacity(self.sigs.len());
     let sigs = self
@@ -289,17 +370,18 @@ impl SignMachine<Transaction> for TransactionSignMachine {
       .drain(..)
       .enumerate()
       .map(|(i, sig)| {
-        let tx_sighash = cache
+        let (sig, share) = sig.sign(
-          .taproot_key_spend_signature_hash(i, &prevouts, SchnorrSighashType::Default)
+          commitments[i].clone(),
-          .unwrap();
+          &cache
-
+            .taproot_key_spend_signature_hash(i, &prevouts, SchnorrSighashType::Default)
-        let (sig, share) = sig.sign(commitments[i].clone(), &tx_sighash)?;
+            .unwrap(),
+        )?;
         shares.push(share);
         Ok(sig)
       })
       .collect::<Result<_, _>>()?;
 
-    Ok((TransactionSignatureMachine { tx: self.tx.0, sigs }, shares))
+    Ok((TransactionSignatureMachine { tx: self.tx.tx, sigs }, shares))
   }
 }
 

coins/monero/src/wallet/send/multisig.rs

@@ -150,7 +150,7 @@ impl SignableTransaction {
         clsag.H,
         keys.current_offset().unwrap_or_else(dfg::Scalar::zero).0 + self.inputs[i].key_offset(),
       ));
-      clsags.push(AlgorithmMachine::new(clsag, offset).map_err(TransactionError::FrostError)?);
+      clsags.push(AlgorithmMachine::new(clsag, offset));
     }
 
     // Select decoys

crypto/frost/src/sign.rs

@@ -53,8 +53,8 @@ pub struct Params<C: Curve, A: Algorithm<C>> {
 }
 
 impl<C: Curve, A: Algorithm<C>> Params<C, A> {
-  pub fn new(algorithm: A, keys: ThresholdKeys<C>) -> Result<Params<C, A>, FrostError> {
+  pub fn new(algorithm: A, keys: ThresholdKeys<C>) -> Params<C, A> {
-    Ok(Params { algorithm, keys })
+    Params { algorithm, keys }
   }
 
   pub fn multisig_params(&self) -> ThresholdParams {
@@ -108,8 +108,8 @@ pub struct AlgorithmMachine<C: Curve, A: Algorithm<C>> {
 
 impl<C: Curve, A: Algorithm<C>> AlgorithmMachine<C, A> {
   /// Creates a new machine to generate a signature with the specified keys.
-  pub fn new(algorithm: A, keys: ThresholdKeys<C>) -> Result<AlgorithmMachine<C, A>, FrostError> {
+  pub fn new(algorithm: A, keys: ThresholdKeys<C>) -> AlgorithmMachine<C, A> {
-    Ok(AlgorithmMachine { params: Params::new(algorithm, keys)? })
+    AlgorithmMachine { params: Params::new(algorithm, keys) }
   }
 
   fn seeded_preprocess(
@@ -273,7 +273,7 @@ impl<C: Curve, A: Algorithm<C>> SignMachine<A::Signature> for AlgorithmSignMachi
     keys: ThresholdKeys<C>,
     cache: CachedPreprocess,
   ) -> Result<Self, FrostError> {
-    let (machine, _) = AlgorithmMachine::new(algorithm, keys)?.seeded_preprocess(cache);
+    let (machine, _) = AlgorithmMachine::new(algorithm, keys).seeded_preprocess(cache);
     Ok(machine)
   }
 

crypto/frost/src/tests/mod.rs

@@ -58,7 +58,7 @@ pub fn algorithm_machines<R: RngCore, C: Curve, A: Algorithm<C>>(
     .iter()
     .filter_map(|(i, keys)| {
       if included.contains(i) {
-        Some((*i, AlgorithmMachine::new(algorithm.clone(), keys.clone()).unwrap()))
+        Some((*i, AlgorithmMachine::new(algorithm.clone(), keys.clone())))
       } else {
         None
       }

crypto/frost/src/tests/vectors.rs

@@ -160,8 +160,7 @@ pub fn test_with_vectors<R: RngCore + CryptoRng, C: Curve, H: Hram<C>>(
 
     let mut machines = vec![];
     for i in &vectors.included {
-      machines
+      machines.push((i, AlgorithmMachine::new(IetfSchnorr::<C, H>::ietf(), keys[i].clone())));
-        .push((i, AlgorithmMachine::new(IetfSchnorr::<C, H>::ietf(), keys[i].clone()).unwrap()));
     }
 
     let mut commitments = HashMap::new();
@@ -343,8 +342,7 @@ pub fn test_with_vectors<R: RngCore + CryptoRng, C: Curve, H: Hram<C>>(
     // Create the machines
     let mut machines = vec![];
     for i in &vectors.included {
-      machines
+      machines.push((i, AlgorithmMachine::new(IetfSchnorr::<C, H>::ietf(), keys[i].clone())));
-        .push((i, AlgorithmMachine::new(IetfSchnorr::<C, H>::ietf(), keys[i].clone()).unwrap()));
     }
 
     for (i, machine) in machines.drain(..) {

processor/Cargo.toml

@@ -39,8 +39,6 @@ frost = { package = "modular-frost", path = "../crypto/frost" }
 
 # Bitcoin
 secp256k1 = { version = "0.24", features = ["global-context", "rand-std"], optional = true }
-bitcoin = { version = "0.29", optional = true }
-
 k256 = { version = "0.12", features = ["arithmetic"], optional = true }
 bitcoin-serai = { path = "../coins/bitcoin", optional = true }
 
@@ -65,7 +63,7 @@ env_logger = "0.10"
 
 [features]
 secp256k1 = ["k256", "frost/secp256k1"]
-bitcoin = ["dep:secp256k1", "dep:bitcoin", "secp256k1", "bitcoin-serai", "serai-client/bitcoin"]
+bitcoin = ["dep:secp256k1", "secp256k1", "bitcoin-serai", "serai-client/bitcoin"]
 
 ed25519 = ["dalek-ff-group", "frost/ed25519"]
 monero = ["ed25519", "monero-serai", "serai-client/monero"]

processor/src/coins/bitcoin.rs

@@ -2,24 +2,6 @@ use std::{io, collections::HashMap};
 
 use async_trait::async_trait;
 
-use bitcoin::{
-  hashes::Hash as HashTrait,
-  schnorr::TweakedPublicKey,
-  consensus::{Encodable, Decodable},
-  psbt::serialize::Serialize,
-  OutPoint,
-  blockdata::script::Instruction,
-  Transaction, Block, Network, Address as BAddress,
-};
-
-#[cfg(test)]
-use bitcoin::{
-  secp256k1::{SECP256K1, SecretKey, Message},
-  PrivateKey, PublicKey, EcdsaSighashType,
-  blockdata::script::Builder,
-  PackedLockTime, Sequence, Script, Witness, TxIn, TxOut,
-};
-
 use transcript::RecommendedTranscript;
 use k256::{
   ProjectivePoint, Scalar,
@@ -28,11 +10,31 @@ use k256::{
 use frost::{curve::Secp256k1, ThresholdKeys};
 
 use bitcoin_serai::{
+  bitcoin::{
+    hashes::Hash as HashTrait,
+    schnorr::TweakedPublicKey,
+    consensus::{Encodable, Decodable},
+    psbt::serialize::Serialize,
+    OutPoint,
+    blockdata::script::Instruction,
+    Transaction, Block, Network, Address as BAddress,
+  },
   crypto::{x_only, make_even},
-  wallet::{SpendableOutput, TransactionMachine, SignableTransaction as BSignableTransaction},
+  wallet::{
+    SpendableOutput, TransactionError, SignableTransaction as BSignableTransaction,
+    TransactionMachine,
+  },
   rpc::{RpcError, Rpc},
 };
 
+#[cfg(test)]
+use bitcoin_serai::bitcoin::{
+  secp256k1::{SECP256K1, SecretKey, Message},
+  PrivateKey, PublicKey, EcdsaSighashType,
+  blockdata::script::Builder,
+  PackedLockTime, Sequence, Script, Witness, TxIn, TxOut,
+};
+
 use serai_client::coins::bitcoin::Address;
 
 use crate::{
@@ -255,7 +257,7 @@ impl Coin for Bitcoin {
   const ID: &'static str = "Bitcoin";
   const CONFIRMATIONS: usize = 3;
 
-  // 0.0001 BTC
+  // 0.0001 BTC, 10,000 satoshis
   #[allow(clippy::inconsistent_digit_grouping)]
   const DUST: u64 = 1_00_000_000 / 10_000;
 
@@ -358,10 +360,13 @@ impl Coin for Bitcoin {
     let signable = |plan: &Plan<Self>, tx_fee: Option<_>| {
       let mut payments = vec![];
       for payment in &plan.payments {
-        // If we're solely estimating the fee, don't actually specify an amount
+        // If we're solely estimating the fee, don't specify the actual amount
-        // This won't affect the fee calculation yet will ensure we don't hit an out of funds error
+        // This won't affect the fee calculation yet will ensure we don't hit a not enough funds
-        payments
+        // error
-          .push((payment.address.0.clone(), if tx_fee.is_none() { 0 } else { payment.amount }));
+        payments.push((
+          payment.address.0.clone(),
+          if tx_fee.is_none() { Self::DUST } else { payment.amount },
+        ));
       }
 
       match BSignableTransaction::new(
@@ -371,21 +376,31 @@ impl Coin for Bitcoin {
         None,
         fee.0,
       ) {
-        Some(signable) => Some(signable),
+        Ok(signable) => Some(signable),
-        // TODO: Use a proper error here
+        Err(TransactionError::NoInputs) => {
-        None => {
+          panic!("trying to create a bitcoin transaction without inputs")
+        }
+        // No outputs left and the change isn't worth enough
+        Err(TransactionError::NoOutputs) => None,
+        Err(TransactionError::TooMuchData) => panic!("too much data despite not specifying data"),
+        Err(TransactionError::NotEnoughFunds) => {
           if tx_fee.is_none() {
-            // Not enough funds
+            // Mot even enough funds to pay the fee
             None
           } else {
-            panic!("didn't have enough funds for a Bitcoin TX");
+            panic!("not enough funds for bitcoin TX despite amortizing the fee")
           }
         }
+        // amortize_fee removes payments which fall below the dust threshold
+        Err(TransactionError::DustPayment) => panic!("dust payment despite removing dust"),
+        Err(TransactionError::TooLargeTransaction) => {
+          panic!("created a too large transaction despite limiting inputs/outputs")
+        }
       }
     };
 
     let tx_fee = match signable(&plan, None) {
-      Some(tx) => tx.fee(),
+      Some(tx) => tx.needed_fee(),
       None => return Ok((None, drop_branches(&plan))),
     };