From: Matt Corallo <git@bluematt.me>
Date: Fri, 15 Jul 2022 16:18:42 +0000 (+0000)
Subject: Construct all ChannelMonitor mutexes in the same function
X-Git-Tag: v0.0.110~8^2~2
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=625cda108c9f5be5443e8c42007bb987261c5270;p=rust-lightning

Construct all ChannelMonitor mutexes in the same function

When we add lockorder detection based on mutex construction site
rather than mutex instance in the next commit, ChannelMonitor's
PartialEq implementation causes spurious failures. This is caused
by the lockorder detection logic considering the ChannelMonitor
inner mutex to be two distinct mutexes - one when monitors are
deserialized and one when monitors are created fresh. Instead, we
attempt to tell the lockorder detection logic that they are the
same by ensuring they're constructed in the same place - in this
case a util method.
---

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 80cd9cb9d..8dd3d4b43 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -965,6 +965,13 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
 }
 
 impl<Signer: Sign> ChannelMonitor<Signer> {
+	/// For lockorder enforcement purposes, we need to have a single site which constructs the
+	/// `inner` mutex, otherwise cases where we lock two monitors at the same time (eg in our
+	/// PartialEq implementation) we may decide a lockorder violation has occurred.
+	fn from_impl(imp: ChannelMonitorImpl<Signer>) -> Self {
+		ChannelMonitor { inner: Mutex::new(imp) }
+	}
+
 	pub(crate) fn new(secp_ctx: Secp256k1<secp256k1::All>, keys: Signer, shutdown_script: Option<Script>,
 	                  on_counterparty_tx_csv: u16, destination_script: &Script, funding_info: (OutPoint, Script),
 	                  channel_parameters: &ChannelTransactionParameters,
@@ -1012,60 +1019,58 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
 		let mut outputs_to_watch = HashMap::new();
 		outputs_to_watch.insert(funding_info.0.txid, vec![(funding_info.0.index as u32, funding_info.1.clone())]);
 
-		ChannelMonitor {
-			inner: Mutex::new(ChannelMonitorImpl {
-				latest_update_id: 0,
-				commitment_transaction_number_obscure_factor,
+		Self::from_impl(ChannelMonitorImpl {
+			latest_update_id: 0,
+			commitment_transaction_number_obscure_factor,
 
-				destination_script: destination_script.clone(),
-				broadcasted_holder_revokable_script: None,
-				counterparty_payment_script,
-				shutdown_script,
+			destination_script: destination_script.clone(),
+			broadcasted_holder_revokable_script: None,
+			counterparty_payment_script,
+			shutdown_script,
 
-				channel_keys_id,
-				holder_revocation_basepoint,
-				funding_info,
-				current_counterparty_commitment_txid: None,
-				prev_counterparty_commitment_txid: None,
+			channel_keys_id,
+			holder_revocation_basepoint,
+			funding_info,
+			current_counterparty_commitment_txid: None,
+			prev_counterparty_commitment_txid: None,
 
-				counterparty_commitment_params,
-				funding_redeemscript,
-				channel_value_satoshis,
-				their_cur_per_commitment_points: None,
+			counterparty_commitment_params,
+			funding_redeemscript,
+			channel_value_satoshis,
+			their_cur_per_commitment_points: None,
 
-				on_holder_tx_csv: counterparty_channel_parameters.selected_contest_delay,
+			on_holder_tx_csv: counterparty_channel_parameters.selected_contest_delay,
 
-				commitment_secrets: CounterpartyCommitmentSecrets::new(),
-				counterparty_claimable_outpoints: HashMap::new(),
-				counterparty_commitment_txn_on_chain: HashMap::new(),
-				counterparty_hash_commitment_number: HashMap::new(),
+			commitment_secrets: CounterpartyCommitmentSecrets::new(),
+			counterparty_claimable_outpoints: HashMap::new(),
+			counterparty_commitment_txn_on_chain: HashMap::new(),
+			counterparty_hash_commitment_number: HashMap::new(),
 
-				prev_holder_signed_commitment_tx: None,
-				current_holder_commitment_tx: holder_commitment_tx,
-				current_counterparty_commitment_number: 1 << 48,
-				current_holder_commitment_number,
+			prev_holder_signed_commitment_tx: None,
+			current_holder_commitment_tx: holder_commitment_tx,
+			current_counterparty_commitment_number: 1 << 48,
+			current_holder_commitment_number,
 
-				payment_preimages: HashMap::new(),
-				pending_monitor_events: Vec::new(),
-				pending_events: Vec::new(),
+			payment_preimages: HashMap::new(),
+			pending_monitor_events: Vec::new(),
+			pending_events: Vec::new(),
 
-				onchain_events_awaiting_threshold_conf: Vec::new(),
-				outputs_to_watch,
+			onchain_events_awaiting_threshold_conf: Vec::new(),
+			outputs_to_watch,
 
-				onchain_tx_handler,
+			onchain_tx_handler,
 
-				lockdown_from_offchain: false,
-				holder_tx_signed: false,
-				funding_spend_seen: false,
-				funding_spend_confirmed: None,
-				htlcs_resolved_on_chain: Vec::new(),
+			lockdown_from_offchain: false,
+			holder_tx_signed: false,
+			funding_spend_seen: false,
+			funding_spend_confirmed: None,
+			htlcs_resolved_on_chain: Vec::new(),
 
-				best_block,
-				counterparty_node_id: Some(counterparty_node_id),
+			best_block,
+			counterparty_node_id: Some(counterparty_node_id),
 
-				secp_ctx,
-			}),
-		}
+			secp_ctx,
+		})
 	}
 
 	#[cfg(test)]
@@ -3361,60 +3366,58 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
 		let mut secp_ctx = Secp256k1::new();
 		secp_ctx.seeded_randomize(&keys_manager.get_secure_random_bytes());
 
-		Ok((best_block.block_hash(), ChannelMonitor {
-			inner: Mutex::new(ChannelMonitorImpl {
-				latest_update_id,
-				commitment_transaction_number_obscure_factor,
+		Ok((best_block.block_hash(), ChannelMonitor::from_impl(ChannelMonitorImpl {
+			latest_update_id,
+			commitment_transaction_number_obscure_factor,
 
-				destination_script,
-				broadcasted_holder_revokable_script,
-				counterparty_payment_script,
-				shutdown_script,
+			destination_script,
+			broadcasted_holder_revokable_script,
+			counterparty_payment_script,
+			shutdown_script,
 
-				channel_keys_id,
-				holder_revocation_basepoint,
-				funding_info,
-				current_counterparty_commitment_txid,
-				prev_counterparty_commitment_txid,
+			channel_keys_id,
+			holder_revocation_basepoint,
+			funding_info,
+			current_counterparty_commitment_txid,
+			prev_counterparty_commitment_txid,
 
-				counterparty_commitment_params,
-				funding_redeemscript,
-				channel_value_satoshis,
-				their_cur_per_commitment_points,
+			counterparty_commitment_params,
+			funding_redeemscript,
+			channel_value_satoshis,
+			their_cur_per_commitment_points,
 
-				on_holder_tx_csv,
+			on_holder_tx_csv,
 
-				commitment_secrets,
-				counterparty_claimable_outpoints,
-				counterparty_commitment_txn_on_chain,
-				counterparty_hash_commitment_number,
+			commitment_secrets,
+			counterparty_claimable_outpoints,
+			counterparty_commitment_txn_on_chain,
+			counterparty_hash_commitment_number,
 
-				prev_holder_signed_commitment_tx,
-				current_holder_commitment_tx,
-				current_counterparty_commitment_number,
-				current_holder_commitment_number,
+			prev_holder_signed_commitment_tx,
+			current_holder_commitment_tx,
+			current_counterparty_commitment_number,
+			current_holder_commitment_number,
 
-				payment_preimages,
-				pending_monitor_events: pending_monitor_events.unwrap(),
-				pending_events,
+			payment_preimages,
+			pending_monitor_events: pending_monitor_events.unwrap(),
+			pending_events,
 
-				onchain_events_awaiting_threshold_conf,
-				outputs_to_watch,
+			onchain_events_awaiting_threshold_conf,
+			outputs_to_watch,
 
-				onchain_tx_handler,
+			onchain_tx_handler,
 
-				lockdown_from_offchain,
-				holder_tx_signed,
-				funding_spend_seen: funding_spend_seen.unwrap(),
-				funding_spend_confirmed,
-				htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(),
+			lockdown_from_offchain,
+			holder_tx_signed,
+			funding_spend_seen: funding_spend_seen.unwrap(),
+			funding_spend_confirmed,
+			htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(),
 
-				best_block,
-				counterparty_node_id,
+			best_block,
+			counterparty_node_id,
 
-				secp_ctx,
-			}),
-		}))
+			secp_ctx,
+		})))
 	}
 }