From: Matt Corallo <git@bluematt.me>
Date: Sun, 8 Jul 2018 20:31:48 +0000 (-0400)
Subject: Clarify ChannelMonitor remote commitment tracking and fix a race
X-Git-Tag: v0.0.12~392^2~2
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=9df30535757d90ad85ca4ff07850f085c7e2b65c;p=rust-lightning

Clarify ChannelMonitor remote commitment tracking and fix a race
---

diff --git a/src/ln/channelmonitor.rs b/src/ln/channelmonitor.rs
index 0daedeac9..61b0f4557 100644
--- a/src/ln/channelmonitor.rs
+++ b/src/ln/channelmonitor.rs
@@ -130,6 +130,7 @@ enum KeyStorage {
 
 #[derive(Clone)]
 struct LocalSignedTx {
+	/// txid of the transaction in tx, just used to make comparison faster
 	txid: Sha256dHash,
 	tx: Transaction,
 	revocation_key: PublicKey,
@@ -155,9 +156,16 @@ pub struct ChannelMonitor {
 
 	old_secrets: [([u8; 32], u64); 49],
 	remote_claimable_outpoints: HashMap<Sha256dHash, Vec<HTLCOutputInCommitment>>,
-	remote_htlc_outputs_on_chain: Mutex<HashMap<Sha256dHash, u64>>,
-	//hash to commitment number mapping use to determine the state of transaction owning it
-	// (revoked/non-revoked) and so lightnen pruning
+	/// We cannot identify HTLC-Success or HTLC-Timeout transactions by themselves on the chain.
+	/// Nor can we figure out their commitment numbers without the commitment transaction they are
+	/// spending. Thus, in order to claim them via revocation key, we track all the remote
+	/// commitment transactions which we find on-chain, mapping them to the commitment number which
+	/// can be used to derive the revocation key and claim the transactions.
+	remote_commitment_txn_on_chain: Mutex<HashMap<Sha256dHash, u64>>,
+	/// Cache used to make pruning of payment_preimages faster.
+	/// Maps payment_hash values to commitment numbers for remote transactions for non-revoked
+	/// remote transactions (ie should remain pretty small).
+	/// Serialized to disk but should generally not be sent to Watchtowers.
 	remote_hash_commitment_number: HashMap<[u8; 32], u64>,
 
 	// We store two local commitment transactions to avoid any race conditions where we may update
@@ -188,7 +196,7 @@ impl Clone for ChannelMonitor {
 
 			old_secrets: self.old_secrets.clone(),
 			remote_claimable_outpoints: self.remote_claimable_outpoints.clone(),
-			remote_htlc_outputs_on_chain: Mutex::new((*self.remote_htlc_outputs_on_chain.lock().unwrap()).clone()),
+			remote_commitment_txn_on_chain: Mutex::new((*self.remote_commitment_txn_on_chain.lock().unwrap()).clone()),
 			remote_hash_commitment_number: self.remote_hash_commitment_number.clone(),
 
 			prev_local_signed_commitment_tx: self.prev_local_signed_commitment_tx.clone(),
@@ -221,7 +229,7 @@ impl ChannelMonitor {
 
 			old_secrets: [([0; 32], 1 << 48); 49],
 			remote_claimable_outpoints: HashMap::new(),
-			remote_htlc_outputs_on_chain: Mutex::new(HashMap::new()),
+			remote_commitment_txn_on_chain: Mutex::new(HashMap::new()),
 			remote_hash_commitment_number: HashMap::new(),
 
 			prev_local_signed_commitment_tx: None,
@@ -591,10 +599,10 @@ impl ChannelMonitor {
 				}
 			}
 
-			if !inputs.is_empty() || !txn_to_broadcast.is_empty() {
+			if !inputs.is_empty() || !txn_to_broadcast.is_empty() { // ie we're confident this is actually ours
 				// We're definitely a remote commitment transaction!
 				// TODO: Register commitment_txid with the ChainWatchInterface!
-				self.remote_htlc_outputs_on_chain.lock().unwrap().insert(commitment_txid, commitment_number);
+				self.remote_commitment_txn_on_chain.lock().unwrap().insert(commitment_txid, commitment_number);
 			}
 			if inputs.is_empty() { return txn_to_broadcast; } // Nothing to be done...probably a false positive/local tx
 
@@ -619,6 +627,15 @@ impl ChannelMonitor {
 
 			txn_to_broadcast.push(spend_tx);
 		} else if let Some(per_commitment_data) = per_commitment_option {
+			// While this isn't useful yet, there is a potential race where if a counterparty
+			// revokes a state at the same time as the commitment transaction for that state is
+			// confirmed, and the watchtower receives the block before the user, the user could
+			// upload a new ChannelMonitor with the revocation secret but the watchtower has
+			// already processed the block, resulting in the remote_commitment_txn_on_chain entry
+			// not being generated by the above conditional. Thus, to be safe, we go ahead and
+			// insert it here.
+			self.remote_commitment_txn_on_chain.lock().unwrap().insert(commitment_txid, commitment_number);
+
 			if let Some(revocation_points) = self.their_cur_revocation_points {
 				let revocation_point_option =
 					if revocation_points.0 == commitment_number { Some(&revocation_points.1) }
@@ -723,7 +740,7 @@ impl ChannelMonitor {
 				}
 			}
 		} else {
-			//TODO: For each input check if its in our remote_htlc_outputs_on_chain map!
+			//TODO: For each input check if its in our remote_commitment_txn_on_chain map!
 		}
 
 		txn_to_broadcast