Skip to content

Commit

Permalink
Don't mutate Plans when signing
Browse files Browse the repository at this point in the history 
This is achieved by not using the Plan struct anymore, yet rather its
decomposition. While less ergonomic, it meets our wants re: safety.
  • Loading branch information
@kayabaNerve
kayabaNerve committed Oct 20, 2023
1 parent 7b6181e commit 5977121
Show file tree
Hide file tree
Showing 4 changed files with 150 additions and 111 deletions.
77 changes: 47 additions & 30 deletions processor/src/networks/bitcoin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -49,7 +49,7 @@ use crate::{
Transaction as TransactionTrait, SignableTransaction as SignableTransactionTrait,
Eventuality as EventualityTrait, EventualitiesTracker, Network,
},
Plan,
Payment,
};

#[derive(Clone, PartialEq, Eq, Debug)]
Expand Down Expand Up @@ -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![];
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::<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,
) {
Expand Down Expand Up @@ -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]))
}
Expand All @@ -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![];
Expand Down Expand Up @@ -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()),
)
Expand All @@ -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| {
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(
Expand Down Expand Up @@ -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);
Expand Down
102 changes: 60 additions & 42 deletions processor/src/networks/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
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 });
}
}
Expand Down Expand Up @@ -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>;

Expand All @@ -328,66 +334,76 @@ 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>;

/// Prepare a SignableTransaction for a transaction.
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 },
});
};

// Amortize the fee over the plan's payments
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 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;
Expand All @@ -399,16 +415,16 @@ 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;
}
}

// 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) },
Expand All @@ -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::<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 {
Expand All @@ -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::<u64>() -
payments.iter().map(|payment| payment.amount).sum::<u64>() -
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;
}
}

Expand Down
Loading

0 comments on commit 5977121

Please sign in to comment.