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

@@ -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<Self>,
+    inputs: &[Output],
+    payments: &[Payment<Self>],
+    change: &Option<Address>,
     fee: Fee,
     calculating_fee: bool,
   ) -> Option<BSignableTransaction> {
-    let mut payments = vec![];
+    let payments = payments
-    for payment in &plan.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
-      payments.push((
-        payment.address.0.clone(),
           if calculating_fee { Self::DUST } else { payment.amount },
-      ));
+        )
-    }
+      })
+      .collect::<Vec<_>>();
 
     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::<NetworkChecked>::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<Self::Output> {
+  async fn get_outputs(&self, block: &Self::Block, key: ProjectivePoint) -> Vec<Output> {
     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<Self>,
+    _: &[u8; 32],
+    inputs: &[Output],
+    payments: &[Payment<Self>],
+    change: &Option<Address>,
     fee_rate: Fee,
   ) -> Result<Option<u64>, 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<Self>,
+    plan_id: &[u8; 32],
+    inputs: &[Output],
+    payments: &[Payment<Self>],
+    change: &Option<Address>,
     fee_rate: Fee,
   ) -> Result<Option<(Self::SignableTransaction, Self::Eventuality)>, NetworkError> {
-    Ok(self.make_signable_transaction(plan, fee_rate, false).await.map(|signable| {
+    Ok(self.make_signable_transaction(inputs, payments, change, fee_rate, false).await.map(
-      let plan_binding_input = *plan.inputs[0].output.outpoint();
+      |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: plan.transcript(), actual: signable },
+          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);

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<N: Network>(plan: &Plan<N>) -> Vec<PostFeeBranch> {
+fn drop_branches<N: Network>(
+  key: <N::Curve as Ciphersuite>::G,
+  payments: &[Payment<N>],
+) -> Vec<PostFeeBranch> {
   let mut branch_outputs = vec![];
-  for payment in &plan.payments {
+  for payment in payments {
-    if payment.address == N::branch_address(plan.key) {
+    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<Self>,
+    plan_id: &[u8; 32],
+    inputs: &[Self::Output],
+    payments: &[Payment<Self>],
+    change: &Option<Self::Address>,
     fee_rate: Self::Fee,
   ) -> Result<Option<u64>, 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<Self>,
+    plan_id: &[u8; 32],
+    inputs: &[Self::Output],
+    payments: &[Payment<Self>],
+    change: &Option<Self::Address>,
     fee_rate: Self::Fee,
   ) -> Result<Option<(Self::SignableTransaction, Self::Eventuality)>, 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<Self>,
+    plan: Plan<Self>,
     fee_rate: Self::Fee,
     operating_costs: u64,
   ) -> Result<PreparedSend<Self>, 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::<u64>() -
+      payments.iter().map(|payment| payment.amount).sum::<u64>();
+
+    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::<u64>();
+      let original_outputs = payments.iter().map(|payment| payment.amount).sum::<u64>();
       // 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::<Vec<_>>();
+        payments.iter().map(|payment| payment.amount).collect::<Vec<_>>();
 
       // 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 this_iter_fee = remaining_fee / u64::try_from(payments.len()).unwrap();
-        let mut overage = remaining_fee % u64::try_from(plan.payments.len()).unwrap();
+        let mut overage = remaining_fee % u64::try_from(payments.len()).unwrap();
-        for payment in &mut plan.payments {
+        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) {
+      for (initial_amount, payment) in initial_payment_amounts.into_iter().zip(&payments) {
-        if payment.address == Self::branch_address(plan.key) {
+        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();
+      payments = payments
-      plan.payments = plan
-        .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::<u64>();
+      let new_outputs = payments.iter().map(|payment| payment.amount).sum::<u64>();
       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() {
+    if change.is_some() {
-      // Now that we've amortized the fee (which may raise the expected change value), grab it
+      let on_chain_expected_change = inputs.iter().map(|input| input.amount()).sum::<u64>() -
-      // again
+        payments.iter().map(|payment| payment.amount).sum::<u64>() -
-      // Then, subtract the TX fee
+        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 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;
       }
     }
 

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<Self>,
+    plan_id: &[u8; 32],
+    inputs: &[Output],
+    payments: &[Payment<Self>],
+    change: &Option<Address>,
     fee_rate: Fee,
     calculating_fee: bool,
   ) -> Result<Option<(RecommendedTranscript, MSignableTransaction)>, 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::<Vec<_>>();
+    let spendable_outputs = inputs.iter().map(|input| input.0.clone()).collect::<Vec<_>>();
 
-    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![];
+    let payments = payments
-    for payment in &plan.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
+      .map(|payment| (payment.address.into(), if calculating_fee { 0 } else { payment.amount }))
-        .push((payment.address.clone().into(), if calculating_fee { 0 } else { payment.amount }));
+      .collect::<Vec<_>>();
-    }
 
     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<Self::Curve>) {}
 
-  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<Self::Output> {
+  async fn get_outputs(&self, block: &Block, key: EdwardsPoint) -> Vec<Output> {
     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<Self>,
+    plan_id: &[u8; 32],
+    inputs: &[Output],
+    payments: &[Payment<Self>],
+    change: &Option<Address>,
     fee_rate: Fee,
   ) -> Result<Option<u64>, 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<Self>,
+    plan_id: &[u8; 32],
+    inputs: &[Output],
+    payments: &[Payment<Self>],
+    change: &Option<Address>,
     fee_rate: Fee,
   ) -> Result<Option<(Self::SignableTransaction, Self::Eventuality)>, NetworkError> {
-    Ok(self.make_signable_transaction(block_number, plan.clone(), fee_rate, false).await?.map(
+    Ok(
-      |(transcript, signable)| {
+      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;

processor/src/plan.rs

@@ -113,16 +113,10 @@ impl<N: Network> Plan<N> {
       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<N: Network> Plan<N> {
     res
   }
 
-  pub fn expected_change(&self) -> u64 {
-    self.inputs.iter().map(|input| input.amount()).sum::<u64>() -
-      self.payments.iter().map(|payment| payment.amount).sum::<u64>()
-  }
-
   pub fn write<W: io::Write>(&self, writer: &mut W) -> io::Result<()> {
     writer.write_all(self.key.to_bytes().as_ref())?;