diff --git a/Cargo.lock b/Cargo.lock index d9207111..7a829640 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6370,7 +6370,6 @@ version = "0.1.0" dependencies = [ "async-trait", "bincode", - "bitcoin", "bitcoin-serai", "dalek-ff-group", "env_logger", diff --git a/coins/bitcoin/src/lib.rs b/coins/bitcoin/src/lib.rs index 31b455b2..289104bd 100644 --- a/coins/bitcoin/src/lib.rs +++ b/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. diff --git a/coins/bitcoin/src/wallet.rs b/coins/bitcoin/src/wallet.rs index d1a16648..a7405197 100644 --- a/coins/bitcoin/src/wallet.rs +++ b/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, Vec, u64); +pub struct SignableTransaction { + tx: Transaction, + offsets: Vec, + prevouts: Vec, + 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 { - self.3 + /// Returns the fee necessary for this transaction to achieve the fee rate specified at + /// 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, payments: &[(Address, u64)], change: Option
, data: Option>, - fee: u64, - ) -> Option { - if inputs.is_empty() || - (payments.is_empty() && change.is_none()) || - (data.as_ref().map(|data| data.len()).unwrap_or(0) > 80) - { - return None; + fee_per_weight: u64, + ) -> Result { + if inputs.is_empty() { + Err(TransactionError::NoInputs)?; + } + + 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::(); @@ -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); - if input_sat < (payment_sat + actual_fee) { - return None; + let mut weight = Self::calculate_weight(tx_ins.len(), payments, None); + let mut needed_fee = fee_per_weight * weight; + 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() }); - actual_fee = fee_with_change; + if value >= DUST { + 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 - // them) + if tx_outs.is_empty() { + Err(TransactionError::NoOutputs)?; + } - Some(SignableTransaction( - Transaction { version: 2, lock_time: PackedLockTime::ZERO, input: tx_ins, output: tx_outs }, + if weight > MAX_STANDARD_TX_WEIGHT { + 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(), - actual_fee, - )) + prevouts: inputs.drain(..).map(|input| input.output).collect(), + 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( - AlgorithmMachine::new(Schnorr::new(transcript), keys.clone().offset(self.1[i])).unwrap(), - ); + sigs.push(AlgorithmMachine::new( + 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>>, @@ -207,7 +289,7 @@ impl PreprocessMachine for TransactionMachine { type Signature = Transaction; type SignMachine = TransactionSignMachine; - fn preprocess( + fn preprocess( mut self, rng: &mut R, ) -> (Self::SignMachine, Self::Preprocess) { @@ -266,9 +348,7 @@ impl SignMachine for TransactionSignMachine { msg: &[u8], ) -> Result<(TransactionSignatureMachine, Self::SignatureShare), FrostError> { if !msg.is_empty() { - Err(FrostError::InternalError( - "message was passed to the TransactionMachine when it generates its own", - ))?; + panic!("message was passed to the TransactionMachine when it generates its own"); } let commitments = (0 .. self.sigs.len()) @@ -280,8 +360,9 @@ impl SignMachine for TransactionSignMachine { }) .collect::>(); - let mut cache = SighashCache::new(&self.tx.0); - let prevouts = Prevouts::All(&self.tx.2); + let mut cache = SighashCache::new(&self.tx.tx); + // 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 for TransactionSignMachine { .drain(..) .enumerate() .map(|(i, sig)| { - let tx_sighash = cache - .taproot_key_spend_signature_hash(i, &prevouts, SchnorrSighashType::Default) - .unwrap(); - - let (sig, share) = sig.sign(commitments[i].clone(), &tx_sighash)?; + let (sig, share) = sig.sign( + commitments[i].clone(), + &cache + .taproot_key_spend_signature_hash(i, &prevouts, SchnorrSighashType::Default) + .unwrap(), + )?; shares.push(share); Ok(sig) }) .collect::>()?; - Ok((TransactionSignatureMachine { tx: self.tx.0, sigs }, shares)) + Ok((TransactionSignatureMachine { tx: self.tx.tx, sigs }, shares)) } } diff --git a/coins/monero/src/wallet/send/multisig.rs b/coins/monero/src/wallet/send/multisig.rs index 749f3afe..a953d403 100644 --- a/coins/monero/src/wallet/send/multisig.rs +++ b/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 diff --git a/crypto/frost/src/sign.rs b/crypto/frost/src/sign.rs index b7e47e25..27257433 100644 --- a/crypto/frost/src/sign.rs +++ b/crypto/frost/src/sign.rs @@ -53,8 +53,8 @@ pub struct Params> { } impl> Params { - pub fn new(algorithm: A, keys: ThresholdKeys) -> Result, FrostError> { - Ok(Params { algorithm, keys }) + pub fn new(algorithm: A, keys: ThresholdKeys) -> Params { + Params { algorithm, keys } } pub fn multisig_params(&self) -> ThresholdParams { @@ -108,8 +108,8 @@ pub struct AlgorithmMachine> { impl> AlgorithmMachine { /// Creates a new machine to generate a signature with the specified keys. - pub fn new(algorithm: A, keys: ThresholdKeys) -> Result, FrostError> { - Ok(AlgorithmMachine { params: Params::new(algorithm, keys)? }) + pub fn new(algorithm: A, keys: ThresholdKeys) -> AlgorithmMachine { + AlgorithmMachine { params: Params::new(algorithm, keys) } } fn seeded_preprocess( @@ -273,7 +273,7 @@ impl> SignMachine for AlgorithmSignMachi keys: ThresholdKeys, cache: CachedPreprocess, ) -> Result { - let (machine, _) = AlgorithmMachine::new(algorithm, keys)?.seeded_preprocess(cache); + let (machine, _) = AlgorithmMachine::new(algorithm, keys).seeded_preprocess(cache); Ok(machine) } diff --git a/crypto/frost/src/tests/mod.rs b/crypto/frost/src/tests/mod.rs index 1297cdd9..7930d5f3 100644 --- a/crypto/frost/src/tests/mod.rs +++ b/crypto/frost/src/tests/mod.rs @@ -58,7 +58,7 @@ pub fn algorithm_machines>( .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 } diff --git a/crypto/frost/src/tests/vectors.rs b/crypto/frost/src/tests/vectors.rs index 84d3676e..1068b707 100644 --- a/crypto/frost/src/tests/vectors.rs +++ b/crypto/frost/src/tests/vectors.rs @@ -160,8 +160,7 @@ pub fn test_with_vectors>( let mut machines = vec![]; for i in &vectors.included { - machines - .push((i, AlgorithmMachine::new(IetfSchnorr::::ietf(), keys[i].clone()).unwrap())); + machines.push((i, AlgorithmMachine::new(IetfSchnorr::::ietf(), keys[i].clone()))); } let mut commitments = HashMap::new(); @@ -343,8 +342,7 @@ pub fn test_with_vectors>( // Create the machines let mut machines = vec![]; for i in &vectors.included { - machines - .push((i, AlgorithmMachine::new(IetfSchnorr::::ietf(), keys[i].clone()).unwrap())); + machines.push((i, AlgorithmMachine::new(IetfSchnorr::::ietf(), keys[i].clone()))); } for (i, machine) in machines.drain(..) { diff --git a/processor/Cargo.toml b/processor/Cargo.toml index 81ab8966..1c4dbb53 100644 --- a/processor/Cargo.toml +++ b/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"] diff --git a/processor/src/coins/bitcoin.rs b/processor/src/coins/bitcoin.rs index e137fe56..bd12da12 100644 --- a/processor/src/coins/bitcoin.rs +++ b/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, 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 - // This won't affect the fee calculation yet will ensure we don't hit an out of funds error - payments - .push((payment.address.0.clone(), if tx_fee.is_none() { 0 } else { payment.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 a not enough funds + // error + 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), - // TODO: Use a proper error here - None => { + Ok(signable) => Some(signable), + Err(TransactionError::NoInputs) => { + 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))), };