Merge build_commitment_transaction local/generated_by_local

We observe the only case where these parameters use to diverge
was at funding signature, at which point we shouldn't have any
pending HTLCs. We can merge them as a `local` commitment transaction
is always generated at the request of your counterparty and
vice-versa.

Author: Antoine Riard

lightning/src/ln/channel.rs

@@ -833,7 +833,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	/// Note that below-dust HTLCs are included in the third return value, but not the second, and
 	/// sources are provided only for outbound HTLCs in the third return value.
 	#[inline]
-	fn build_commitment_transaction<L: Deref>(&self, commitment_number: u64, keys: &TxCreationKeys, local: bool, generated_by_local: bool, feerate_per_kw: u32, logger: &L) -> (Transaction, usize, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>) where L::Target: Logger {
+	fn build_commitment_transaction<L: Deref>(&self, commitment_number: u64, keys: &TxCreationKeys, building_for_counterparty: bool, feerate_per_kw: u32, logger: &L) -> (Transaction, usize, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>) where L::Target: Logger {
 		let obscured_commitment_transaction_number = self.get_commitment_transaction_number_obscure_factor() ^ (INITIAL_COMMITMENT_NUMBER - commitment_number);
 
 		let txins = {
@@ -850,12 +850,12 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		let mut txouts: Vec<(TxOut, Option<(HTLCOutputInCommitment, Option<&HTLCSource>)>)> = Vec::with_capacity(self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len() + 2);
 		let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new();
 
-		let broadcaster_dust_limit_satoshis = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis };
+		let broadcaster_dust_limit_satoshis = if !building_for_counterparty { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis };
 		let mut counterparty_htlc_total_msat = 0;
 		let mut holder_htlc_total_msat = 0;
 		let mut holder_value_msat_offset = 0;
 
-		log_trace!(logger, "Building commitment transaction number {} (really {} xor {}) for {}, generated by {} with fee {}...", commitment_number, (INITIAL_COMMITMENT_NUMBER - commitment_number), self.get_commitment_transaction_number_obscure_factor(), if local { "us" } else { "remote" }, if generated_by_local { "us" } else { "remote" }, feerate_per_kw);
+		log_trace!(logger, "Building commitment transaction number {} (really {} xor {}) for {}, generated by {} with fee {}...", commitment_number, (INITIAL_COMMITMENT_NUMBER - commitment_number), self.get_commitment_transaction_number_obscure_factor(), if !building_for_counterparty { "us" } else { "remote" }, if !building_for_counterparty { "us" } else { "remote" }, feerate_per_kw);
 
 		macro_rules! get_htlc_in_commitment {
 			($htlc: expr, $offered: expr) => {
@@ -871,7 +871,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 		macro_rules! add_htlc_output {
 			($htlc: expr, $outbound: expr, $source: expr, $state_name: expr) => {
-				if $outbound == local { // "offered HTLC output"
+				if $outbound == !building_for_counterparty { // "offered HTLC output"
 					let htlc_in_tx = get_htlc_in_commitment!($htlc, true);
 					if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + (feerate_per_kw as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) {
 						log_trace!(logger, "   ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, log_bytes!($htlc.payment_hash.0), $htlc.amount_msat);
@@ -899,13 +899,15 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			}
 		}
 
+		// If we initiate the commitment update, inbound HTLC flowing from our counterparty
+		// to us
 		for ref htlc in self.pending_inbound_htlcs.iter() {
 			let (include, state_name) = match htlc.state {
-				InboundHTLCState::RemoteAnnounced(_) => (!generated_by_local, "RemoteAnnounced"),
-				InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => (!generated_by_local, "AwaitingRemoteRevokeToAnnounce"),
+				InboundHTLCState::RemoteAnnounced(_) => (!building_for_counterparty, "RemoteAnnounced"),
+				InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => (!building_for_counterparty, "AwaitingRemoteRevokeToAnnounce"),
 				InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => (true, "AwaitingAnnouncedRemoteRevoke"),
 				InboundHTLCState::Committed => (true, "Committed"),
-				InboundHTLCState::LocalRemoved(_) => (!generated_by_local, "LocalRemoved"),
+				InboundHTLCState::LocalRemoved(_) => (!building_for_counterparty, "LocalRemoved"),
 			};
 
 			if include {
@@ -915,7 +917,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 				log_trace!(logger, "   ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, log_bytes!(htlc.payment_hash.0), htlc.amount_msat, state_name);
 				match &htlc.state {
 					&InboundHTLCState::LocalRemoved(ref reason) => {
-						if generated_by_local {
+						if building_for_counterparty {
 							if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
 								holder_value_msat_offset += htlc.amount_msat as i64;
 							}
@@ -928,10 +930,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 		for ref htlc in self.pending_outbound_htlcs.iter() {
 			let (include, state_name) = match htlc.state {
-				OutboundHTLCState::LocalAnnounced(_) => (generated_by_local, "LocalAnnounced"),
+				OutboundHTLCState::LocalAnnounced(_) => (building_for_counterparty, "LocalAnnounced"),
 				OutboundHTLCState::Committed => (true, "Committed"),
-				OutboundHTLCState::RemoteRemoved(_) => (generated_by_local, "RemoteRemoved"),
-				OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => (generated_by_local, "AwaitingRemoteRevokeToRemove"),
+				OutboundHTLCState::RemoteRemoved(_) => (building_for_counterparty, "RemoteRemoved"),
+				OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => (building_for_counterparty, "AwaitingRemoteRevokeToRemove"),
 				OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => (false, "AwaitingRemovedRemoteRevoke"),
 			};
 
@@ -945,7 +947,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 						holder_value_msat_offset -= htlc.amount_msat as i64;
 					},
 					OutboundHTLCState::RemoteRemoved(None) => {
-						if !generated_by_local {
+						if !building_for_counterparty {
 							holder_value_msat_offset -= htlc.amount_msat as i64;
 						}
 					},
@@ -967,7 +969,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		{
 			// Make sure that the to_self/to_remote is always either past the appropriate
 			// channel_reserve *or* it is making progress towards it.
-			let mut broadcaster_max_commitment_tx_output = if generated_by_local {
+			let mut broadcaster_max_commitment_tx_output = if !building_for_counterparty {
 				self.holder_max_commitment_tx_output.lock().unwrap()
 			} else {
 				self.counterparty_max_commitment_tx_output.lock().unwrap()
@@ -985,21 +987,21 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			(holder_value_msat / 1000, counterparty_value_msat / 1000 - total_fee as i64)
 		};
 
-		let (value_to_a, value_to_b) = if local { (holder_value, counterparty_value) } else { (counterparty_value, holder_value) };
+		let (value_to_a, value_to_b) = if !building_for_counterparty { (holder_value, counterparty_value) } else { (counterparty_value, holder_value) };
 
 		if value_to_a >= (broadcaster_dust_limit_satoshis as i64) {
-			log_trace!(logger, "   ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a);
+			log_trace!(logger, "   ...including {} output with value {}", if !building_for_counterparty { "to_local" } else { "to_remote" }, value_to_a);
 			txouts.push((TxOut {
 				script_pubkey: chan_utils::get_revokeable_redeemscript(&keys.revocation_key,
-				                                                       if local { self.counterparty_selected_contest_delay } else { self.holder_selected_contest_delay },
+				                                                       if !building_for_counterparty { self.counterparty_selected_contest_delay } else { self.holder_selected_contest_delay },
 				                                                       &keys.broadcaster_delayed_payment_key).to_v0_p2wsh(),
 				value: value_to_a as u64
 			}, None));
 		}
 
 		if value_to_b >= (broadcaster_dust_limit_satoshis as i64) {
-			log_trace!(logger, "   ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b);
-			let static_payment_pk = if local {
+			log_trace!(logger, "   ...including {} output with value {}", if !building_for_counterparty { "to_remote" } else { "to_local" }, value_to_b);
+			let static_payment_pk = if !building_for_counterparty {
 				self.counterparty_pubkeys.as_ref().unwrap().payment_point
 			} else {
 				self.holder_keys.pubkeys().payment_point
@@ -1477,7 +1479,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		let funding_script = self.get_funding_redeemscript();
 
 		let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?;
-		let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, self.feerate_per_kw, logger).0;
+		let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, false, self.feerate_per_kw, logger).0;
 		let sighash = hash_to_message!(&bip143::SigHashCache::new(&initial_commitment_tx).signature_hash(0, &funding_script, self.channel_value_satoshis, SigHashType::All)[..]);
 
 		// They sign the "our" commitment transaction...
@@ -1487,9 +1489,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		let tx = HolderCommitmentTransaction::new_missing_holder_sig(initial_commitment_tx, sig.clone(), &self.holder_keys.pubkeys().funding_pubkey, self.counterparty_funding_pubkey(), keys, self.feerate_per_kw, Vec::new());
 
 		let counterparty_keys = self.build_remote_transaction_keys()?;
-		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, self.feerate_per_kw, logger).0;
-		let pre_remote_keys = PreCalculatedTxCreationKeys::new(counterparty_keys);
-		let counterparty_signature = self.holder_keys.sign_counterparty_commitment(self.feerate_per_kw, &counterparty_initial_commitment_tx, &pre_remote_keys, &Vec::new(), &self.secp_ctx)
+		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, true, self.feerate_per_kw, logger).0;
+		let pre_counterparty_keys = PreCalculatedTxCreationKeys::new(counterparty_keys);
+		let counterparty_signature = self.holder_keys.sign_counterparty_commitment(self.feerate_per_kw, &counterparty_initial_commitment_tx, &pre_counterparty_keys, &Vec::new(), &self.secp_ctx)
 				.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0;
 
 		// We sign "counterparty" commitment transaction, allowing them to broadcast the tx if they wish.
@@ -1578,10 +1580,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		let funding_script = self.get_funding_redeemscript();
 
 		let counterparty_keys = self.build_remote_transaction_keys()?;
-		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, self.feerate_per_kw, logger).0;
+		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, true, self.feerate_per_kw, logger).0;
 
 		let holder_keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?;
-		let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &holder_keys, true, false, self.feerate_per_kw, logger).0;
+		let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &holder_keys, false, self.feerate_per_kw, logger).0;
 		let sighash = hash_to_message!(&bip143::SigHashCache::new(&initial_commitment_tx).signature_hash(0, &funding_script, self.channel_value_satoshis, SigHashType::All)[..]);
 
 		let counterparty_funding_pubkey = &self.counterparty_pubkeys.as_ref().unwrap().funding_pubkey;
@@ -1977,7 +1979,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		};
 
 		let mut commitment_tx = {
-			let mut commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, feerate_per_kw, logger);
+			let mut commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, false, feerate_per_kw, logger);
 			let htlcs_cloned: Vec<_> = commitment_tx.2.drain(..).map(|htlc| (htlc.0, htlc.1.map(|h| h.clone()))).collect();
 			(commitment_tx.0, commitment_tx.1, htlcs_cloned)
 		};
@@ -2063,9 +2065,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			}
 		}
 		for htlc in self.pending_outbound_htlcs.iter_mut() {
-			if let Some(fail_reason) = if let &mut OutboundHTLCState::RemoteRemoved(ref mut fail_reason) = &mut htlc.state {
-				Some(fail_reason.take())
-			} else { None } {
+			if let Some(fail_reason) = if let &mut OutboundHTLCState::RemoteRemoved(ref mut fail_reason) = &mut htlc.state { Some(fail_reason.take())} else { None } {
 				htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(fail_reason);
 				need_commitment = true;
 			}
@@ -3518,9 +3518,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	/// If an Err is returned, it is a ChannelError::Close (for get_outbound_funding_created)
 	fn get_outbound_funding_created_signature<L: Deref>(&mut self, logger: &L) -> Result<Signature, ChannelError> where L::Target: Logger {
 		let counterparty_keys = self.build_remote_transaction_keys()?;
-		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, self.feerate_per_kw, logger).0;
-		let pre_remote_keys = PreCalculatedTxCreationKeys::new(counterparty_keys);
-		Ok(self.holder_keys.sign_counterparty_commitment(self.feerate_per_kw, &counterparty_initial_commitment_tx, &pre_remote_keys, &Vec::new(), &self.secp_ctx)
+		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, true, self.feerate_per_kw, logger).0;
+		let pre_counterparty_keys = PreCalculatedTxCreationKeys::new(counterparty_keys);
+		Ok(self.holder_keys.sign_counterparty_commitment(self.feerate_per_kw, &counterparty_initial_commitment_tx, &pre_counterparty_keys, &Vec::new(), &self.secp_ctx)
 				.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0)
 	}
 
@@ -3863,7 +3863,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		}
 
 		let counterparty_keys = self.build_remote_transaction_keys()?;
-		let counterparty_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, feerate_per_kw, logger);
+		let counterparty_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, true, feerate_per_kw, logger);
 		let (signature, htlc_signatures);
 
 		{
@@ -4685,7 +4685,7 @@ mod tests {
 				$( { $htlc_idx: expr, $counterparty_htlc_sig_hex: expr, $htlc_sig_hex: expr, $htlc_tx_hex: expr } ), *
 			} ) => { {
 				unsigned_tx = {
-					let mut res = chan.build_commitment_transaction(0xffffffffffff - 42, &keys, true, false, chan.feerate_per_kw, &logger);
+					let mut res = chan.build_commitment_transaction(0xffffffffffff - 42, &keys, false, chan.feerate_per_kw, &logger);
 					let htlcs = res.2.drain(..)
 						.filter_map(|(htlc, _)| if htlc.transaction_output_index.is_some() { Some(htlc) } else { None })
 						.collect();