From f492a192ff6c076985bfb665c99f4589604cd6ae Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 26 May 2021 15:47:29 +0000 Subject: [PATCH] Add assertions to ensure we don't use an invalid package_amount This somewhat cleans up the public API of PackageSolvingData to make it harder to get an invalid amount and use it, adding further debug assertion to check it at test-time. --- lightning/src/chain/onchaintx.rs | 6 ++---- lightning/src/chain/package.rs | 17 +++++++++++------ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 04de721f..9bbc2a4a 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -345,10 +345,9 @@ impl OnchainTxHandler { // Compute new height timer to decide when we need to regenerate a new bumped version of the claim tx (if we // didn't receive confirmation of it before, or not enough reorg-safe depth on top of it). let new_timer = Some(cached_request.get_height_timer(height)); - let amt = cached_request.package_amount(); if cached_request.is_malleable() { let predicted_weight = cached_request.package_weight(&self.destination_script); - if let Some((output_value, new_feerate)) = cached_request.compute_package_output(predicted_weight, amt, fee_estimator, logger) { + if let Some((output_value, new_feerate)) = cached_request.compute_package_output(predicted_weight, fee_estimator, logger) { assert!(new_feerate != 0); let transaction = cached_request.finalize_package(self, output_value, self.destination_script.clone(), logger).unwrap(); @@ -360,8 +359,7 @@ impl OnchainTxHandler { // Note: Currently, amounts of holder outputs spending witnesses aren't used // as we can't malleate spending package to increase their feerate. This // should change with the remaining anchor output patchset. - debug_assert!(amt == 0); - if let Some(transaction) = cached_request.finalize_package(self, amt, self.destination_script.clone(), logger) { + if let Some(transaction) = cached_request.finalize_package(self, 0, self.destination_script.clone(), logger) { return Some((None, 0, transaction)); } } diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 311937a1..9e92e203 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -274,8 +274,8 @@ impl PackageSolvingData { // Note: Currently, amounts of holder outputs spending witnesses aren't used // as we can't malleate spending package to increase their feerate. This // should change with the remaining anchor output patchset. - PackageSolvingData::HolderHTLCOutput(..) => { 0 }, - PackageSolvingData::HolderFundingOutput(..) => { 0 }, + PackageSolvingData::HolderHTLCOutput(..) => { unreachable!() }, + PackageSolvingData::HolderFundingOutput(..) => { unreachable!() }, }; amt } @@ -288,8 +288,8 @@ impl PackageSolvingData { // Note: Currently, weights of holder outputs spending witnesses aren't used // as we can't malleate spending package to increase their feerate. This // should change with the remaining anchor output patchset. - PackageSolvingData::HolderHTLCOutput(..) => { debug_assert!(false); 0 }, - PackageSolvingData::HolderFundingOutput(..) => { debug_assert!(false); 0 }, + PackageSolvingData::HolderHTLCOutput(..) => { unreachable!() }, + PackageSolvingData::HolderFundingOutput(..) => { unreachable!() }, }; weight } @@ -577,7 +577,9 @@ impl PackageTemplate { } self.height_timer = cmp::min(self.height_timer, merge_from.height_timer); } - pub(crate) fn package_amount(&self) -> u64 { + /// Gets the amount of all outptus being spent by this package, only valid for malleable + /// packages. + fn package_amount(&self) -> u64 { let mut amounts = 0; for (_, outp) in self.inputs.iter() { amounts += outp.amount(); @@ -628,6 +630,7 @@ impl PackageTemplate { return Some(bumped_tx); }, PackageMalleability::Untractable => { + debug_assert_eq!(value, 0, "value is ignored for non-malleable packages, should be zero to ensure callsites are correct"); if let Some((outpoint, outp)) = self.inputs.first() { if let Some(final_tx) = outp.get_finalized_tx(outpoint, onchain_handler) { log_trace!(logger, "Adding claiming input for outpoint {}:{}", outpoint.txid, outpoint.vout); @@ -653,10 +656,12 @@ impl PackageTemplate { current_height + LOW_FREQUENCY_BUMP_INTERVAL } /// Returns value in satoshis to be included as package outgoing output amount and feerate with which package finalization should be done. - pub(crate) fn compute_package_output(&self, predicted_weight: usize, input_amounts: u64, fee_estimator: &F, logger: &L) -> Option<(u64, u64)> + pub(crate) fn compute_package_output(&self, predicted_weight: usize, fee_estimator: &F, logger: &L) -> Option<(u64, u64)> where F::Target: FeeEstimator, L::Target: Logger, { + debug_assert!(self.malleability == PackageMalleability::Malleable, "The package output is fixed for non-malleable packages"); + let input_amounts = self.package_amount(); // If old feerate is 0, first iteration of this claim, use normal fee calculation if self.feerate_previous != 0 { if let Some((new_fee, feerate)) = feerate_bump(predicted_weight, input_amounts, self.feerate_previous, fee_estimator, logger) { -- 2.30.2