From: Antoine Riard <ariard@student.42.fr>
Date: Fri, 27 Mar 2020 21:53:52 +0000 (-0400)
Subject: Prevent any update of local commitment transaction once signed
X-Git-Tag: v0.0.12~84^2~14
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=e46e183084ed858f41aa304acd78503aea1a96ed;p=rust-lightning

Prevent any update of local commitment transaction once signed

To prevent any unsafe state discrepancy between offchain and onchain,
once local commitment transaction has been signed due to an event
(either block height for HTLC-timeout or channel force-closure), don't
allow any further update of local commitment transaction view
to avoid delivery of revocation secret to counterparty for the
aformentionned signed transaction.
---

diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
index 0ef269c8c..a5e0c1460 100644
--- a/lightning/src/ln/chan_utils.rs
+++ b/lightning/src/ln/chan_utils.rs
@@ -522,9 +522,18 @@ pub struct LocalCommitmentTransaction {
 impl LocalCommitmentTransaction {
 	#[cfg(test)]
 	pub fn dummy() -> Self {
+		let dummy_input = TxIn {
+			previous_output: OutPoint {
+				txid: Default::default(),
+				vout: 0,
+			},
+			script_sig: Default::default(),
+			sequence: 0,
+			witness: vec![vec![], vec![], vec![]]
+		};
 		Self { tx: Transaction {
 			version: 2,
-			input: Vec::new(),
+			input: vec![dummy_input],
 			output: Vec::new(),
 			lock_time: 0,
 		} }
diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs
index 70c92143a..2fa416edb 100644
--- a/lightning/src/ln/channelmonitor.rs
+++ b/lightning/src/ln/channelmonitor.rs
@@ -1241,10 +1241,18 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		if self.their_to_self_delay.is_none() {
 			return Err(MonitorUpdateError("Got a local commitment tx info update before we'd set basic information about the channel"));
 		}
+		// Returning a monitor error before updating tracking points means in case of using
+		// a concurrent watchtower implementation for same channel, if this one doesn't
+		// reject update as we do, you MAY have the latest local valid commitment tx onchain
+		// for which you want to spend outputs. We're NOT robust again this scenario right
+		// now but we should consider it later.
+		if let Err(_) = self.onchain_tx_handler.provide_latest_local_tx(commitment_tx.clone()) {
+			return Err(MonitorUpdateError("Local commitment signed has already been signed, no further update of LOCAL commitment transaction is allowed"));
+		}
 		self.prev_local_signed_commitment_tx = self.current_local_signed_commitment_tx.take();
 		self.current_local_signed_commitment_tx = Some(LocalSignedTx {
 			txid: commitment_tx.txid(),
-			tx: commitment_tx.clone(),
+			tx: commitment_tx,
 			revocation_key: local_keys.revocation_key,
 			a_htlc_key: local_keys.a_htlc_key,
 			b_htlc_key: local_keys.b_htlc_key,
@@ -1253,7 +1261,6 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			feerate_per_kw,
 			htlc_outputs,
 		});
-		self.onchain_tx_handler.provide_latest_local_tx(commitment_tx);
 		Ok(())
 	}
 
diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs
index 4c935ea65..d0d1f29e6 100644
--- a/lightning/src/ln/onchaintx.rs
+++ b/lightning/src/ln/onchaintx.rs
@@ -725,8 +725,17 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 		}
 	}
 
-	pub(super) fn provide_latest_local_tx(&mut self, tx: LocalCommitmentTransaction) {
+	pub(super) fn provide_latest_local_tx(&mut self, tx: LocalCommitmentTransaction) -> Result<(), ()> {
+		// To prevent any unsafe state discrepancy between offchain and onchain, once local
+		// commitment transaction has been signed due to an event (either block height for
+		// HTLC-timeout or channel force-closure), don't allow any further update of local
+		// commitment transaction view to avoid delivery of revocation secret to counterparty
+		// for the aformentionned signed transaction.
+		if let Some(ref local_commitment) = self.local_commitment {
+			if local_commitment.has_local_sig() { return Err(()) }
+		}
 		self.prev_local_commitment = self.local_commitment.take();
 		self.local_commitment = Some(tx);
+		Ok(())
 	}
 }