From d6bc1c1ea362eacde98db698c5416e98accea5ae Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Thu, 19 Oct 2023 23:16:01 -0400 Subject: [PATCH] Explicitly only adjust operating costs when plan.change.is_some() The existing code should've mostly handled this fine. Only a single edge case (TX fee reduction on no-change Plans) would cause an improper increase in operating costs. --- processor/src/networks/bitcoin.rs | 37 +++++++++++++++++------------- processor/src/networks/monero.rs | 38 ++++++++++++++++++------------- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/processor/src/networks/bitcoin.rs b/processor/src/networks/bitcoin.rs index 1738137b..3e70bd6e 100644 --- a/processor/src/networks/bitcoin.rs +++ b/processor/src/networks/bitcoin.rs @@ -565,7 +565,8 @@ impl Network for Bitcoin { // 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 + plan.expected_change(), + operating_costs: operating_costs + + if plan.change.is_some() { plan.expected_change() } else { 0 }, }); } }; @@ -575,21 +576,25 @@ impl Network for Bitcoin { 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; + 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 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(); diff --git a/processor/src/networks/monero.rs b/processor/src/networks/monero.rs index e4baab86..da6a9b91 100644 --- a/processor/src/networks/monero.rs +++ b/processor/src/networks/monero.rs @@ -530,32 +530,38 @@ impl Network for Monero { // 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 + plan.expected_change(), + operating_costs: operating_costs + + if plan.change.is_some() { plan.expected_change() } else { 0 }, }); } }; let AmortizeFeeRes { post_fee_branches, mut operating_costs } = amortize_fee(&mut plan, operating_costs, tx_fee); + let plan_change_is_some = plan.change.is_some(); 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; + 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 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 signable = SignableTransaction { transcript, actual: signable };