From: Matt Corallo <git@bluematt.me>
Date: Wed, 26 May 2021 15:47:29 +0000 (+0000)
Subject: Add assertions to ensure we don't use an invalid package_amount
X-Git-Tag: v0.0.98~13^2~9
X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=f492a192ff6c076985bfb665c99f4589604cd6ae;p=rust-lightning

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.
---

diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index 04de721f9..9bbc2a4ab 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -345,10 +345,9 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 		// 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<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 			// 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 311937a12..9e92e2031 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<F: Deref, L: Deref>(&self, predicted_weight: usize, input_amounts: u64, fee_estimator: &F, logger: &L) -> Option<(u64, u64)>
+	pub(crate) fn compute_package_output<F: Deref, L: Deref>(&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) {