Clarify ChannelMonitor remote commitment tracking and fix a race

diff --git a/src/ln/channelmonitor.rs b/src/ln/channelmonitor.rs
index 0daedeac955482d6f1b4936f68ce3538ca8599ca..61b0f4557d39f95abbb83dcbf1fc5116caa8f54c 100644 (file)
--- 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