diff --git a/coins/bitcoin/src/wallet/send.rs b/coins/bitcoin/src/wallet/send.rs index 066ae5579..e24dbe94c 100644 --- a/coins/bitcoin/src/wallet/send.rs +++ b/coins/bitcoin/src/wallet/send.rs @@ -26,14 +26,14 @@ use crate::{ #[rustfmt::skip] // https://github.com/bitcoin/bitcoin/blob/306ccd4927a2efe325c8d84be1bdb79edeb29b04/src/policy/policy.h#L27 -const MAX_STANDARD_TX_WEIGHT: u64 = 400_000; +pub const MAX_STANDARD_TX_WEIGHT: u64 = 400_000; #[rustfmt::skip] // https://github.com/bitcoin/bitcoin/blob/306ccd4927a2efe325c8d84be1bdb79edeb29b04/src/policy/policy.cpp#L26-L63 // As the above notes, a lower amount may not be considered dust if contained in a SegWit output // This doesn't bother with delineation due to how marginal these values are, and because it isn't // worth the complexity to implement differentation -const DUST: u64 = 546; +pub const DUST: u64 = 546; #[rustfmt::skip] // The constant is from: @@ -44,7 +44,7 @@ const DUST: u64 = 546; // https://github.com/bitcoin/bitcoin/blob/296735f7638749906243c9e203df7bd024493806/src/net_processing.cpp#L5721-L5732 // And then the fee itself is fee per thousand units, not fee per unit // https://github.com/bitcoin/bitcoin/blob/306ccd4927a2efe325c8d84be1bdb79edeb29b04/src/policy/feerate.cpp#L23-L37 -const MIN_FEE_PER_KILO_VSIZE: u64 = 1000; +pub const MIN_FEE_PER_KILO_VSIZE: u64 = 1000; #[derive(Clone, PartialEq, Eq, Debug, Error)] pub enum TransactionError { diff --git a/processor/src/multisigs/mod.rs b/processor/src/multisigs/mod.rs index 78bff5062..e511a8d8a 100644 --- a/processor/src/multisigs/mod.rs +++ b/processor/src/multisigs/mod.rs @@ -745,9 +745,8 @@ impl MultisigManager { res.push((key, id, tx, eventuality)); } - // TODO: If the TX is None, restore its inputs to the scheduler - // Otherwise, if the TX had a change output, dropping its inputs would burn funds - // Are there exceptional cases upon rotation? + // TODO: If the TX is None, restore its inputs to the scheduler for efficiency's sake + // If this TODO is removed, also reduce the operating costs } res }; diff --git a/processor/src/multisigs/scanner.rs b/processor/src/multisigs/scanner.rs index 39b271c97..caaa9c4b6 100644 --- a/processor/src/multisigs/scanner.rs +++ b/processor/src/multisigs/scanner.rs @@ -553,7 +553,9 @@ impl Scanner { // TODO: These lines are the ones which will cause a really long-lived lock acquisiton for output in network.get_outputs(&block, key).await { assert_eq!(output.key(), key); - outputs.push(output); + if output.amount() >= N::DUST { + outputs.push(output); + } } for (id, (block_number, tx)) in network diff --git a/processor/src/multisigs/scheduler.rs b/processor/src/multisigs/scheduler.rs index f2d41c09b..6a69ddf81 100644 --- a/processor/src/multisigs/scheduler.rs +++ b/processor/src/multisigs/scheduler.rs @@ -435,6 +435,7 @@ impl Scheduler { let mut remainder = diff - (per_payment * payments_len); for payment in payments.iter_mut() { + // TODO: This usage of saturating_sub is invalid as we *need* to subtract this value payment.amount = payment.amount.saturating_sub(per_payment + remainder); // Only subtract the remainder once remainder = 0; diff --git a/processor/src/networks/bitcoin.rs b/processor/src/networks/bitcoin.rs index 83b35f860..1738137b0 100644 --- a/processor/src/networks/bitcoin.rs +++ b/processor/src/networks/bitcoin.rs @@ -562,21 +562,35 @@ impl Network for Bitcoin { return Ok(PreparedSend { tx: None, post_fee_branches: drop_branches(&plan), - // We expected a change output of sum(inputs) - sum(outputs) + // 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 - operating_costs: operating_costs + - plan.inputs.iter().map(|input| input.amount()).sum::() - - plan.payments.iter().map(|payment| payment.amount).sum::(), + // TODO: Look at input restoration to reduce this operating cost + operating_costs: operating_costs + plan.expected_change(), }); } }; - let AmortizeFeeRes { post_fee_branches, operating_costs } = + let AmortizeFeeRes { post_fee_branches, mut operating_costs } = amortize_fee(&mut plan, operating_costs, tx_fee); let signable = signable(&plan, Some(tx_fee)).unwrap(); - // TODO: If the change output was dropped by Bitcoin, increase operating costs + // 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, 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; + } let plan_binding_input = *plan.inputs[0].output.outpoint(); let outputs = signable.outputs().to_vec(); diff --git a/processor/src/networks/monero.rs b/processor/src/networks/monero.rs index 8958abf4d..e4baab86d 100644 --- a/processor/src/networks/monero.rs +++ b/processor/src/networks/monero.rs @@ -527,22 +527,38 @@ impl Network for Monero { return Ok(PreparedSend { tx: None, post_fee_branches: drop_branches(&plan), - // We expected a change output of sum(inputs) - sum(outputs) + // 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 - operating_costs: operating_costs + - plan.inputs.iter().map(|input| input.amount()).sum::() - - plan.payments.iter().map(|payment| payment.amount).sum::(), + // TODO: Look at input restoration to reduce this operating cost + operating_costs: operating_costs + plan.expected_change(), }); } }; - let AmortizeFeeRes { post_fee_branches, operating_costs } = + let AmortizeFeeRes { post_fee_branches, mut operating_costs } = amortize_fee(&mut plan, operating_costs, tx_fee); + let plan_expected_change = plan.expected_change(); + + let signable = signable(plan, Some(tx_fee))?.unwrap(); + + // 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, 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; + } - // TODO: If the change output was dropped by Monero, increase operating costs - - let signable = - SignableTransaction { transcript, actual: signable(plan, Some(tx_fee))?.unwrap() }; + let signable = SignableTransaction { transcript, actual: signable }; let eventuality = signable.actual.eventuality().unwrap(); Ok(PreparedSend { tx: Some((signable, eventuality)), post_fee_branches, operating_costs }) } diff --git a/processor/src/plan.rs b/processor/src/plan.rs index 7cea2d074..f85372e1f 100644 --- a/processor/src/plan.rs +++ b/processor/src/plan.rs @@ -132,6 +132,11 @@ 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())?; diff --git a/processor/src/tests/literal/mod.rs b/processor/src/tests/literal/mod.rs index c98913cdd..d09583455 100644 --- a/processor/src/tests/literal/mod.rs +++ b/processor/src/tests/literal/mod.rs @@ -1,6 +1,17 @@ #[cfg(feature = "bitcoin")] mod bitcoin { - use crate::networks::Bitcoin; + use crate::networks::{Network, Bitcoin}; + + #[test] + fn test_dust_constant() { + struct IsTrue; + trait True {} + impl True for IsTrue {} + fn check() { + core::hint::black_box(()); + } + check::= bitcoin_serai::wallet::DUST }>>(); + } async fn bitcoin() -> Bitcoin { let bitcoin = Bitcoin::new("http://serai:seraidex@127.0.0.1:18443".to_string()).await;