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.
This commit is contained in:
Luke Parker
2023-10-19 23:16:01 -04:00
parent 7b2dec63ce
commit d6bc1c1ea3
2 changed files with 43 additions and 32 deletions

View File

@@ -565,7 +565,8 @@ impl Network for Bitcoin {
// This plan expects a change output valued at 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 // Since we can no longer create this change output, it becomes an operating cost
// TODO: Look at input restoration to reduce this 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,22 +576,26 @@ impl Network for Bitcoin {
let signable = signable(&plan, Some(tx_fee)).unwrap(); 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 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 // Then, subtract the TX fee
// //
// The first `expected_change` call gets the theoretically expected change from the theoretical // The first `expected_change` call gets the theoretically expected change from the
// Plan object, and accordingly doesn't subtract the fee (expecting the payments to bare it) // theoretical Plan object, and accordingly doesn't subtract the fee (expecting the payments
// This call wants the actual value, and since Plan is unaware of the fee, has to manually // to bare it)
// adjust // 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; 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 // 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 // This may be slightly inaccurate as dropping payments may reduce the fee, raising the
// above dust // change above dust
// That's fine since it'd have to be in a very precarious state AND then it's over-eager in // That's fine since it'd have to be in a very precarious state AND then it's over-eager in
// tabulating costs // tabulating costs
if on_chain_expected_change < Self::DUST { if on_chain_expected_change < Self::DUST {
operating_costs += on_chain_expected_change; operating_costs += on_chain_expected_change;
} }
}
let plan_binding_input = *plan.inputs[0].output.outpoint(); let plan_binding_input = *plan.inputs[0].output.outpoint();
let outputs = signable.outputs().to_vec(); let outputs = signable.outputs().to_vec();

View File

@@ -530,33 +530,39 @@ impl Network for Monero {
// This plan expects a change output valued at 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 // Since we can no longer create this change output, it becomes an operating cost
// TODO: Look at input restoration to reduce this 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 } = let AmortizeFeeRes { post_fee_branches, mut operating_costs } =
amortize_fee(&mut plan, operating_costs, tx_fee); 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 plan_expected_change = plan.expected_change();
let signable = signable(plan, Some(tx_fee))?.unwrap(); 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 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 // Then, subtract the TX fee
// //
// The first `expected_change` call gets the theoretically expected change from the theoretical // The first `expected_change` call gets the theoretically expected change from the
// Plan object, and accordingly doesn't subtract the fee (expecting the payments to bare it) // theoretical Plan object, and accordingly doesn't subtract the fee (expecting the payments
// This call wants the actual value, and since Plan is unaware of the fee, has to manually // to bare it)
// adjust // 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; 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 // 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 // This may be slightly inaccurate as dropping payments may reduce the fee, raising the
// above dust // change above dust
// That's fine since it'd have to be in a very precarious state AND then it's over-eager in // That's fine since it'd have to be in a very precarious state AND then it's over-eager in
// tabulating costs // tabulating costs
if on_chain_expected_change < Self::DUST { if on_chain_expected_change < Self::DUST {
operating_costs += on_chain_expected_change; operating_costs += on_chain_expected_change;
} }
}
let signable = SignableTransaction { transcript, actual: signable }; let signable = SignableTransaction { transcript, actual: signable };
let eventuality = signable.actual.eventuality().unwrap(); let eventuality = signable.actual.eventuality().unwrap();