Ensure build_commitment_tx and next_local/remote_tx_fee agree on the fee

Author: valentinewallace

lightning/src/ln/channel.rs

@@ -42,6 +42,8 @@ use std;
 use std::default::Default;
 use std::{cmp,mem,fmt};
 use std::ops::Deref;
+#[cfg(any(test, feature = "fuzztarget"))]
+use std::sync::Mutex;
 use bitcoin::hashes::hex::ToHex;
 
 #[cfg(test)]
@@ -407,6 +409,24 @@ pub(super) struct Channel<ChanSigner: ChannelKeys> {
 	commitment_secrets: CounterpartyCommitmentSecrets,
 
 	network_sync: UpdateStatus,
+
+	// We save these values so we can make sure `next_local_commit_tx_fee_msat` and
+	// `next_remote_commit_tx_fee_msat` properly predict what the next commitment transaction fee will
+	// be, by comparing the cached values to the fee of the tranaction generated by
+	// `build_commitment_transaction`.
+	#[cfg(any(test, feature = "fuzztarget"))]
+	next_local_commitment_tx_fee_info_cached: Mutex<Option<CommitmentTxInfoCached>>,
+	#[cfg(any(test, feature = "fuzztarget"))]
+	next_remote_commitment_tx_fee_info_cached: Mutex<Option<CommitmentTxInfoCached>>,
+}
+
+#[cfg(any(test, feature = "fuzztarget"))]
+struct CommitmentTxInfoCached {
+	fee: u64,
+	total_pending_htlcs: usize,
+	next_holder_htlc_id: u64,
+	next_counterparty_htlc_id: u64,
+	feerate: u32,
 }
 
 pub const OUR_MAX_HTLCS: u16 = 50; //TODO
@@ -578,6 +598,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			commitment_secrets: CounterpartyCommitmentSecrets::new(),
 
 			network_sync: UpdateStatus::Fresh,
+
+			#[cfg(any(test, feature = "fuzztarget"))]
+			next_local_commitment_tx_fee_info_cached: Mutex::new(None),
+			#[cfg(any(test, feature = "fuzztarget"))]
+			next_remote_commitment_tx_fee_info_cached: Mutex::new(None),
 		})
 	}
 
@@ -811,6 +836,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			commitment_secrets: CounterpartyCommitmentSecrets::new(),
 
 			network_sync: UpdateStatus::Fresh,
+
+			#[cfg(any(test, feature = "fuzztarget"))]
+			next_local_commitment_tx_fee_info_cached: Mutex::new(None),
+			#[cfg(any(test, feature = "fuzztarget"))]
+			next_remote_commitment_tx_fee_info_cached: Mutex::new(None),
 		};
 
 		Ok(chan)
@@ -1768,7 +1798,32 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			}
 		}
 
-		self.commit_tx_fee_msat(included_htlcs + addl_htlcs)
+		let num_htlcs = included_htlcs + addl_htlcs;
+		let res = self.commit_tx_fee_msat(num_htlcs);
+		#[cfg(any(test, feature = "fuzztarget"))]
+		{
+			let mut fee = res;
+			if fee_spike_buffer_htlc.is_some() {
+				fee = self.commit_tx_fee_msat(num_htlcs - 1);
+			}
+			let total_pending_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len()
+				+ self.holding_cell_htlc_updates.len();
+			let commitment_tx_info = CommitmentTxInfoCached {
+				fee,
+				total_pending_htlcs,
+				next_holder_htlc_id: match htlc.origin {
+					HTLCInitiator::LocalOffered => self.next_holder_htlc_id + 1,
+					HTLCInitiator::RemoteOffered => self.next_holder_htlc_id,
+				},
+				next_counterparty_htlc_id: match htlc.origin {
+					HTLCInitiator::LocalOffered => self.next_counterparty_htlc_id,
+					HTLCInitiator::RemoteOffered => self.next_counterparty_htlc_id + 1,
+				},
+				feerate: self.feerate_per_kw,
+			};
+			*self.next_local_commitment_tx_fee_info_cached.lock().unwrap() = Some(commitment_tx_info);
+		}
+		res
 	}
 
 	// Get the commitment tx fee for the remote's next commitment transaction based on the number of
@@ -1816,11 +1871,36 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			match htlc.state {
 				OutboundHTLCState::Committed => included_htlcs += 1,
 				OutboundHTLCState::RemoteRemoved {..} => included_htlcs += 1,
+				OutboundHTLCState::LocalAnnounced { .. } => included_htlcs += 1,
 				_ => {},
 			}
 		}
 
-		self.commit_tx_fee_msat(included_htlcs + addl_htlcs)
+		let num_htlcs = included_htlcs + addl_htlcs;
+		let res = self.commit_tx_fee_msat(num_htlcs);
+		#[cfg(any(test, feature = "fuzztarget"))]
+		{
+			let mut fee = res;
+			if fee_spike_buffer_htlc.is_some() {
+				fee = self.commit_tx_fee_msat(num_htlcs - 1);
+			}
+			let total_pending_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
+			let commitment_tx_info = CommitmentTxInfoCached {
+				fee,
+				total_pending_htlcs,
+				next_holder_htlc_id: match htlc.origin {
+					HTLCInitiator::LocalOffered => self.next_holder_htlc_id + 1,
+					HTLCInitiator::RemoteOffered => self.next_holder_htlc_id,
+				},
+				next_counterparty_htlc_id: match htlc.origin {
+					HTLCInitiator::LocalOffered => self.next_counterparty_htlc_id,
+					HTLCInitiator::RemoteOffered => self.next_counterparty_htlc_id + 1,
+				},
+				feerate: self.feerate_per_kw,
+			};
+			*self.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = Some(commitment_tx_info);
+		}
+		res
 	}
 
 	pub fn update_add_htlc<F, L: Deref>(&mut self, msg: &msgs::UpdateAddHTLC, mut pending_forward_status: PendingHTLCStatus, create_pending_htlc_status: F, logger: &L) -> Result<(), ChannelError>
@@ -2057,15 +2137,31 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			(commitment_tx.1, htlcs_cloned, commitment_tx.0, commitment_txid)
 		};
 
+		let total_fee = feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;
 		//If channel fee was updated by funder confirm funder can afford the new fee rate when applied to the current local commitment transaction
 		if update_fee {
-			let total_fee = feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;
-
 			let counterparty_reserve_we_require = Channel::<ChanSigner>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis);
 			if self.channel_value_satoshis - self.value_to_self_msat / 1000 < total_fee + counterparty_reserve_we_require {
 				return Err((None, ChannelError::Close("Funding remote cannot afford proposed new fee".to_owned())));
 			}
 		}
+		#[cfg(any(test, feature = "fuzztarget"))]
+		{
+			if self.is_outbound() {
+				let projected_commit_tx_info = self.next_local_commitment_tx_fee_info_cached.lock().unwrap().take();
+				*self.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None;
+				if let Some(info) = projected_commit_tx_info {
+					let total_pending_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len()
+						+ self.holding_cell_htlc_updates.len();
+					if info.total_pending_htlcs == total_pending_htlcs
+						&& info.next_holder_htlc_id == self.next_holder_htlc_id
+						&& info.next_counterparty_htlc_id == self.next_counterparty_htlc_id
+						&& info.feerate == self.feerate_per_kw {
+							assert_eq!(total_fee, info.fee / 1000);
+						}
+				}
+			}
+		}
 
 		if msg.htlc_signatures.len() != num_htlcs {
 			return Err((None, ChannelError::Close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), num_htlcs))));
@@ -2331,6 +2427,12 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			return Err(ChannelError::Close("Received an unexpected revoke_and_ack".to_owned()));
 		}
 
+		#[cfg(any(test, feature = "fuzztarget"))]
+		{
+			*self.next_local_commitment_tx_fee_info_cached.lock().unwrap() = None;
+			*self.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None;
+		}
+
 		self.commitment_secrets.provide_secret(self.cur_counterparty_commitment_transaction_number + 1, msg.per_commitment_secret)
 			.map_err(|_| ChannelError::Close("Previous secrets did not match new one".to_owned()))?;
 		self.latest_monitor_update_id += 1;
@@ -3961,6 +4063,24 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		let counterparty_commitment_txid = counterparty_commitment_tx.0.trust().txid();
 		let (signature, htlc_signatures);
 
+		#[cfg(any(test, feature = "fuzztarget"))]
+		{
+			if !self.is_outbound() {
+				let projected_commit_tx_info = self.next_remote_commitment_tx_fee_info_cached.lock().unwrap().take();
+				*self.next_local_commitment_tx_fee_info_cached.lock().unwrap() = None;
+				if let Some(info) = projected_commit_tx_info {
+					let total_pending_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
+					if info.total_pending_htlcs == total_pending_htlcs
+						&& info.next_holder_htlc_id == self.next_holder_htlc_id
+						&& info.next_counterparty_htlc_id == self.next_counterparty_htlc_id
+						&& info.feerate == self.feerate_per_kw {
+							let actual_fee = self.commit_tx_fee_msat(counterparty_commitment_tx.1);
+							assert_eq!(actual_fee, info.fee);
+						}
+				}
+			}
+		}
+
 		{
 			let mut htlcs = Vec::with_capacity(counterparty_commitment_tx.2.len());
 			for &(ref htlc, _) in counterparty_commitment_tx.2.iter() {
@@ -4551,6 +4671,11 @@ impl<'a, ChanSigner: ChannelKeys, K: Deref> ReadableArgs<&'a K> for Channel<Chan
 			commitment_secrets,
 
 			network_sync: UpdateStatus::Fresh,
+
+			#[cfg(any(test, feature = "fuzztarget"))]
+			next_local_commitment_tx_fee_info_cached: Mutex::new(None),
+			#[cfg(any(test, feature = "fuzztarget"))]
+			next_remote_commitment_tx_fee_info_cached: Mutex::new(None),
 		})
 	}
 }