From 5977121c4824044774015a6a1d9e79f0807c3250 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Fri, 20 Oct 2023 10:56:18 -0400 Subject: [PATCH] Don't mutate Plans when signing This is achieved by not using the Plan struct anymore, yet rather its decomposition. While less ergonomic, it meets our wants re: safety. --- processor/src/networks/bitcoin.rs | 77 +++++++++++++--------- processor/src/networks/mod.rs | 102 ++++++++++++++++++------------ processor/src/networks/monero.rs | 71 +++++++++++++-------- processor/src/plan.rs | 11 ---- 4 files changed, 150 insertions(+), 111 deletions(-) diff --git a/processor/src/networks/bitcoin.rs b/processor/src/networks/bitcoin.rs index a673648e8..7ca5fcc24 100644 --- a/processor/src/networks/bitcoin.rs +++ b/processor/src/networks/bitcoin.rs @@ -2,7 +2,7 @@ use std::{time::Duration, io, collections::HashMap}; use async_trait::async_trait; -use transcript::RecommendedTranscript; +use transcript::{Transcript, RecommendedTranscript}; use ciphersuite::group::ff::PrimeField; use k256::{ProjectivePoint, Scalar}; use frost::{ @@ -49,7 +49,7 @@ use crate::{ Transaction as TransactionTrait, SignableTransaction as SignableTransactionTrait, Eventuality as EventualityTrait, EventualitiesTracker, Network, }, - Plan, + Payment, }; #[derive(Clone, PartialEq, Eq, Debug)] @@ -322,25 +322,29 @@ impl Bitcoin { async fn make_signable_transaction( &self, - plan: &Plan, + inputs: &[Output], + payments: &[Payment], + change: &Option
, fee: Fee, calculating_fee: bool, ) -> Option { - let mut payments = vec![]; - for payment in &plan.payments { - // 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 calculating_fee { Self::DUST } else { payment.amount }, - )); - } + let payments = payments + .iter() + .map(|payment| { + ( + payment.address.0.clone(), + // 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 + if calculating_fee { Self::DUST } else { payment.amount }, + ) + }) + .collect::>(); match BSignableTransaction::new( - plan.inputs.iter().map(|input| input.output.clone()).collect(), + inputs.iter().map(|input| input.output.clone()).collect(), &payments, - plan.change.as_ref().map(|change| change.0.clone()), + change.as_ref().map(|change| change.0.clone()), None, fee.0, ) { @@ -415,12 +419,12 @@ impl Network for Bitcoin { Address(BAddress::::new(BitcoinNetwork::Bitcoin, address_payload(key).unwrap())) } - fn branch_address(key: ProjectivePoint) -> Self::Address { + fn branch_address(key: ProjectivePoint) -> Address { let (_, offsets, _) = scanner(key); Self::address(key + (ProjectivePoint::GENERATOR * offsets[&OutputType::Branch])) } - fn change_address(key: ProjectivePoint) -> Self::Address { + fn change_address(key: ProjectivePoint) -> Address { let (_, offsets, _) = scanner(key); Self::address(key + (ProjectivePoint::GENERATOR * offsets[&OutputType::Change])) } @@ -435,7 +439,7 @@ impl Network for Bitcoin { self.rpc.get_block(&block_hash).await.map_err(|_| NetworkError::ConnectionError) } - async fn get_outputs(&self, block: &Self::Block, key: ProjectivePoint) -> Vec { + async fn get_outputs(&self, block: &Self::Block, key: ProjectivePoint) -> Vec { let (scanner, _, kinds) = scanner(key); let mut outputs = vec![]; @@ -552,12 +556,15 @@ impl Network for Bitcoin { async fn needed_fee( &self, _: usize, - plan: &Plan, + _: &[u8; 32], + inputs: &[Output], + payments: &[Payment], + change: &Option
, fee_rate: Fee, ) -> Result, NetworkError> { Ok( self - .make_signable_transaction(plan, fee_rate, true) + .make_signable_transaction(inputs, payments, change, fee_rate, true) .await .map(|signable| signable.needed_fee()), ) @@ -566,17 +573,27 @@ impl Network for Bitcoin { async fn signable_transaction( &self, _: usize, - plan: &Plan, + plan_id: &[u8; 32], + inputs: &[Output], + payments: &[Payment], + change: &Option
, fee_rate: Fee, ) -> Result, NetworkError> { - Ok(self.make_signable_transaction(plan, fee_rate, false).await.map(|signable| { - let plan_binding_input = *plan.inputs[0].output.outpoint(); - let outputs = signable.outputs().to_vec(); - ( - SignableTransaction { transcript: plan.transcript(), actual: signable }, - Eventuality { plan_binding_input, outputs }, - ) - })) + Ok(self.make_signable_transaction(inputs, payments, change, fee_rate, false).await.map( + |signable| { + let mut transcript = + RecommendedTranscript::new(b"Serai Processor Bitcoin Transaction Transcript"); + transcript.append_message(b"plan", plan_id); + + let plan_binding_input = *inputs[0].output.outpoint(); + let outputs = signable.outputs().to_vec(); + + ( + SignableTransaction { transcript, actual: signable }, + Eventuality { plan_binding_input, outputs }, + ) + }, + )) } async fn attempt_send( @@ -636,7 +653,7 @@ impl Network for Bitcoin { } #[cfg(test)] - async fn test_send(&self, address: Self::Address) -> Block { + async fn test_send(&self, address: Address) -> Block { let secret_key = SecretKey::new(&mut rand_core::OsRng); let private_key = PrivateKey::new(secret_key, BitcoinNetwork::Regtest); let public_key = PublicKey::from_private_key(SECP256K1, &private_key); diff --git a/processor/src/networks/mod.rs b/processor/src/networks/mod.rs index 7d1f8b982..3cc9c4aae 100644 --- a/processor/src/networks/mod.rs +++ b/processor/src/networks/mod.rs @@ -26,7 +26,7 @@ pub mod monero; #[cfg(feature = "monero")] pub use monero::Monero; -use crate::Plan; +use crate::{Payment, Plan}; #[derive(Clone, Copy, Error, Debug)] pub enum NetworkError { @@ -199,10 +199,13 @@ pub struct PostFeeBranch { } // Return the PostFeeBranches needed when dropping a transaction -fn drop_branches(plan: &Plan) -> Vec { +fn drop_branches( + key: ::G, + payments: &[Payment], +) -> Vec { let mut branch_outputs = vec![]; - for payment in &plan.payments { - if payment.address == N::branch_address(plan.key) { + for payment in payments { + if payment.address == N::branch_address(key) { branch_outputs.push(PostFeeBranch { expected: payment.amount, actual: None }); } } @@ -315,7 +318,10 @@ pub trait Network: 'static + Send + Sync + Clone + PartialEq + Eq + Debug { async fn needed_fee( &self, block_number: usize, - plan: &Plan, + plan_id: &[u8; 32], + inputs: &[Self::Output], + payments: &[Payment], + change: &Option, fee_rate: Self::Fee, ) -> Result, NetworkError>; @@ -328,7 +334,10 @@ pub trait Network: 'static + Send + Sync + Clone + PartialEq + Eq + Debug { async fn signable_transaction( &self, block_number: usize, - plan: &Plan, + plan_id: &[u8; 32], + inputs: &[Self::Output], + payments: &[Payment], + change: &Option, fee_rate: Self::Fee, ) -> Result, NetworkError>; @@ -336,25 +345,32 @@ pub trait Network: 'static + Send + Sync + Clone + PartialEq + Eq + Debug { async fn prepare_send( &self, block_number: usize, - mut plan: Plan, + plan: Plan, fee_rate: Self::Fee, operating_costs: u64, ) -> Result, NetworkError> { // Sanity check this has at least one output planned assert!((!plan.payments.is_empty()) || plan.change.is_some()); - let Some(tx_fee) = self.needed_fee(block_number, &plan, fee_rate).await? else { + let plan_id = plan.id(); + let Plan { key, inputs, mut payments, change } = plan; + let theoretical_change_amount = inputs.iter().map(|input| input.amount()).sum::() - + payments.iter().map(|payment| payment.amount).sum::(); + + let Some(tx_fee) = + self.needed_fee(block_number, &plan_id, &inputs, &payments, &change, fee_rate).await? + else { // This Plan is not fulfillable // TODO: Have Plan explicitly distinguish payments and branches in two separate Vecs? return Ok(PreparedSend { tx: None, // Have all of its branches dropped - post_fee_branches: drop_branches(&plan), + post_fee_branches: drop_branches(key, &payments), // This plan expects a change output valued at sum(inputs) - sum(outputs) // Since we can no longer create this change output, it becomes an operating cost // TODO: Look at input restoration to reduce this operating cost operating_costs: operating_costs + - if plan.change.is_some() { plan.expected_change() } else { 0 }, + if change.is_some() { theoretical_change_amount } else { 0 }, }); }; @@ -362,32 +378,32 @@ pub trait Network: 'static + Send + Sync + Clone + PartialEq + Eq + Debug { let (post_fee_branches, mut operating_costs) = (|| { // If we're creating a change output, letting us recoup coins, amortize the operating costs // as well - let total_fee = tx_fee + if plan.change.is_some() { operating_costs } else { 0 }; + let total_fee = tx_fee + if change.is_some() { operating_costs } else { 0 }; - let original_outputs = plan.payments.iter().map(|payment| payment.amount).sum::(); + let original_outputs = payments.iter().map(|payment| payment.amount).sum::(); // If this isn't enough for the total fee, drop and move on if original_outputs < total_fee { let mut remaining_operating_costs = operating_costs; - if plan.change.is_some() { + if change.is_some() { // Operating costs increase by the TX fee remaining_operating_costs += tx_fee; // Yet decrease by the payments we managed to drop remaining_operating_costs = remaining_operating_costs.saturating_sub(original_outputs); } - return (drop_branches(&plan), remaining_operating_costs); + return (drop_branches(key, &payments), remaining_operating_costs); } let initial_payment_amounts = - plan.payments.iter().map(|payment| payment.amount).collect::>(); + payments.iter().map(|payment| payment.amount).collect::>(); // Amortize the transaction fee across outputs let mut remaining_fee = total_fee; // Run as many times as needed until we can successfully subtract this fee while remaining_fee != 0 { // This shouldn't be a / by 0 as these payments have enough value to cover the fee - let this_iter_fee = remaining_fee / u64::try_from(plan.payments.len()).unwrap(); - let mut overage = remaining_fee % u64::try_from(plan.payments.len()).unwrap(); - for payment in &mut plan.payments { + let this_iter_fee = remaining_fee / u64::try_from(payments.len()).unwrap(); + let mut overage = remaining_fee % u64::try_from(payments.len()).unwrap(); + for payment in &mut payments { let this_payment_fee = this_iter_fee + overage; // Only subtract the overage once overage = 0; @@ -399,7 +415,7 @@ pub trait Network: 'static + Send + Sync + Clone + PartialEq + Eq + Debug { } // If any payment is now below the dust threshold, set its value to 0 so it'll be dropped - for payment in &mut plan.payments { + for payment in &mut payments { if payment.amount < Self::DUST { payment.amount = 0; } @@ -407,8 +423,8 @@ pub trait Network: 'static + Send + Sync + Clone + PartialEq + Eq + Debug { // Note the branch outputs' new values let mut branch_outputs = vec![]; - for (initial_amount, payment) in initial_payment_amounts.into_iter().zip(&plan.payments) { - if payment.address == Self::branch_address(plan.key) { + for (initial_amount, payment) in initial_payment_amounts.into_iter().zip(&payments) { + if payment.address == Self::branch_address(key) { branch_outputs.push(PostFeeBranch { expected: initial_amount, actual: if payment.amount == 0 { None } else { Some(payment.amount) }, @@ -417,27 +433,25 @@ pub trait Network: 'static + Send + Sync + Clone + PartialEq + Eq + Debug { } // Drop payments now worth 0 - let plan_id = plan.id(); - plan.payments = plan - .payments + payments = payments .drain(..) .filter(|payment| { if payment.amount != 0 { true } else { - log::debug!("dropping dust payment from plan {}", hex::encode(&plan_id)); + log::debug!("dropping dust payment from plan {}", hex::encode(plan_id)); false } }) .collect(); // Sanity check the fee was successfully amortized - let new_outputs = plan.payments.iter().map(|payment| payment.amount).sum::(); + let new_outputs = payments.iter().map(|payment| payment.amount).sum::(); assert!((new_outputs + total_fee) <= original_outputs); ( branch_outputs, - if plan.change.is_none() { + if change.is_none() { // If the change is None, this had no effect on the operating costs operating_costs } else { @@ -448,33 +462,37 @@ pub trait Network: 'static + Send + Sync + Clone + PartialEq + Eq + Debug { ) })(); - let Some(tx) = self.signable_transaction(block_number, &plan, fee_rate).await? else { + let Some(tx) = self + .signable_transaction(block_number, &plan_id, &inputs, &payments, &change, fee_rate) + .await? + else { panic!( - "{}. post-amortization plan: {:?}, successfully amoritized fee: {}", + "{}. {}: {}, {}: {:?}, {}: {:?}, {}: {:?}, {}: {}", "signable_transaction returned None for a TX we prior successfully calculated the fee for", - &plan, + "id", + hex::encode(plan_id), + "inputs", + inputs, + "post-amortization payments", + payments, + "change", + change, + "successfully amoritized fee", tx_fee, ) }; - if plan.change.is_some() { - // Now that we've amortized the fee (which may raise the expected change value), grab it - // again - // Then, subtract the TX fee - // - // The first `expected_change` call gets the theoretically expected change from the - // theoretical Plan object, and accordingly doesn't subtract the fee (expecting the payments - // to bare it) - // This call wants the actual value, post-amortization over outputs, and since Plan is - // unaware of the fee, has to manually adjust - let on_chain_expected_change = plan.expected_change() - tx_fee; + if change.is_some() { + let on_chain_expected_change = inputs.iter().map(|input| input.amount()).sum::() - + payments.iter().map(|payment| payment.amount).sum::() - + tx_fee; // If the change value is less than the dust threshold, it becomes an operating cost // This may be slightly inaccurate as dropping payments may reduce the fee, raising the // change above dust // That's fine since it'd have to be in a very precarious state AND then it's over-eager in // tabulating costs if on_chain_expected_change < Self::DUST { - operating_costs += on_chain_expected_change; + operating_costs += theoretical_change_amount; } } diff --git a/processor/src/networks/monero.rs b/processor/src/networks/monero.rs index eb2aadbb4..06b8a7400 100644 --- a/processor/src/networks/monero.rs +++ b/processor/src/networks/monero.rs @@ -34,7 +34,7 @@ pub use serai_client::{ }; use crate::{ - Payment, Plan, additional_key, + Payment, additional_key, networks::{ NetworkError, Block as BlockTrait, OutputType, Output as OutputTrait, Transaction as TransactionTrait, SignableTransaction as SignableTransactionTrait, @@ -206,10 +206,14 @@ impl Monero { scanner } + #[allow(clippy::too_many_arguments)] async fn make_signable_transaction( &self, block_number: usize, - mut plan: Plan, + plan_id: &[u8; 32], + inputs: &[Output], + payments: &[Payment], + change: &Option
, fee_rate: Fee, calculating_fee: bool, ) -> Result, NetworkError> { @@ -234,9 +238,11 @@ impl Monero { // Check a fork hasn't occurred which this processor hasn't been updated for assert_eq!(protocol, self.rpc.get_protocol().await.map_err(|_| NetworkError::ConnectionError)?); - let spendable_outputs = plan.inputs.iter().cloned().map(|input| input.0).collect::>(); + let spendable_outputs = inputs.iter().map(|input| input.0.clone()).collect::>(); - let mut transcript = plan.transcript(); + let mut transcript = + RecommendedTranscript::new(b"Serai Processor Monero Transaction Transcript"); + transcript.append_message(b"plan", plan_id); // All signers need to select the same decoys // All signers use the same height and a seeded RNG to make sure they do so. @@ -254,11 +260,12 @@ impl Monero { // Monero requires at least two outputs // If we only have one output planned, add a dummy payment - let outputs = plan.payments.len() + usize::from(u8::from(plan.change.is_some())); + let mut payments = payments.to_vec(); + let outputs = payments.len() + usize::from(u8::from(change.is_some())); if outputs == 0 { return Ok(None); } else if outputs == 1 { - plan.payments.push(Payment { + payments.push(Payment { address: Address::new( ViewPair::new(EdwardsPoint::generator().0, Zeroizing::new(Scalar::ONE.0)) .address(MoneroNetwork::Mainnet, AddressSpec::Standard), @@ -269,23 +276,22 @@ impl Monero { }); } - let mut payments = vec![]; - for payment in &plan.payments { + let payments = payments + .into_iter() // 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.clone().into(), if calculating_fee { 0 } else { payment.amount })); - } + .map(|payment| (payment.address.into(), if calculating_fee { 0 } else { payment.amount })) + .collect::>(); match MSignableTransaction::new( protocol, // Use the plan ID as the r_seed // This perfectly binds the plan while simultaneously allowing verifying the plan was // executed with no additional communication - Some(Zeroizing::new(plan.id())), + Some(Zeroizing::new(*plan_id)), inputs.clone(), payments, - plan.change.map(|change| Change::fingerprintable(change.into())), + change.clone().map(|change| Change::fingerprintable(change.into())), vec![], fee_rate, ) { @@ -375,15 +381,15 @@ impl Network for Monero { // Monero doesn't require/benefit from tweaking fn tweak_keys(_: &mut ThresholdKeys) {} - fn address(key: EdwardsPoint) -> Self::Address { + fn address(key: EdwardsPoint) -> Address { Self::address_internal(key, EXTERNAL_SUBADDRESS) } - fn branch_address(key: EdwardsPoint) -> Self::Address { + fn branch_address(key: EdwardsPoint) -> Address { Self::address_internal(key, BRANCH_SUBADDRESS) } - fn change_address(key: EdwardsPoint) -> Self::Address { + fn change_address(key: EdwardsPoint) -> Address { Self::address_internal(key, CHANGE_SUBADDRESS) } @@ -404,7 +410,7 @@ impl Network for Monero { ) } - async fn get_outputs(&self, block: &Block, key: EdwardsPoint) -> Vec { + async fn get_outputs(&self, block: &Block, key: EdwardsPoint) -> Vec { let outputs = loop { match Self::scanner(key).scan(&self.rpc, block).await { Ok(outputs) => break outputs, @@ -515,12 +521,15 @@ impl Network for Monero { async fn needed_fee( &self, block_number: usize, - plan: &Plan, + plan_id: &[u8; 32], + inputs: &[Output], + payments: &[Payment], + change: &Option
, fee_rate: Fee, ) -> Result, NetworkError> { Ok( self - .make_signable_transaction(block_number, plan.clone(), fee_rate, true) + .make_signable_transaction(block_number, plan_id, inputs, payments, change, fee_rate, true) .await? .map(|(_, signable)| signable.fee()), ) @@ -529,16 +538,22 @@ impl Network for Monero { async fn signable_transaction( &self, block_number: usize, - plan: &Plan, + plan_id: &[u8; 32], + inputs: &[Output], + payments: &[Payment], + change: &Option
, fee_rate: Fee, ) -> Result, NetworkError> { - Ok(self.make_signable_transaction(block_number, plan.clone(), fee_rate, false).await?.map( - |(transcript, signable)| { - let signable = SignableTransaction { transcript, actual: signable }; - let eventuality = signable.actual.eventuality().unwrap(); - (signable, eventuality) - }, - )) + Ok( + self + .make_signable_transaction(block_number, plan_id, inputs, payments, change, fee_rate, false) + .await? + .map(|(transcript, signable)| { + let signable = SignableTransaction { transcript, actual: signable }; + let eventuality = signable.actual.eventuality().unwrap(); + (signable, eventuality) + }), + ) } async fn attempt_send( @@ -606,7 +621,7 @@ impl Network for Monero { } #[cfg(test)] - async fn test_send(&self, address: Self::Address) -> Block { + async fn test_send(&self, address: Address) -> Block { use zeroize::Zeroizing; use rand_core::OsRng; use monero_serai::wallet::FeePriority; diff --git a/processor/src/plan.rs b/processor/src/plan.rs index c009c8523..7cea2d074 100644 --- a/processor/src/plan.rs +++ b/processor/src/plan.rs @@ -113,16 +113,10 @@ impl Plan { transcript.append_message(b"input", input.id()); } - // Don't transcript the payments as these will change between the intended Plan and the actual - // Plan, once various fee logics have executed - // TODO: Distinguish IntendedPlan and ActualPlan, or make actual payments a distinct field, - // letting us transcript this - /* transcript.domain_separate(b"payments"); for payment in &self.payments { payment.transcript(&mut transcript); } - */ if let Some(change) = &self.change { transcript.append_message(b"change", change.to_string()); @@ -138,11 +132,6 @@ impl Plan { res } - pub fn expected_change(&self) -> u64 { - self.inputs.iter().map(|input| input.amount()).sum::() - - self.payments.iter().map(|payment| payment.amount).sum::() - } - pub fn write(&self, writer: &mut W) -> io::Result<()> { writer.write_all(self.key.to_bytes().as_ref())?;