Always return failure in update_monitor after funding spend

Previously, monitor updates were allowed freely even after a
funding-spend transaction confirmed. This would allow a race
condition where we could receive a payment (including the
counterparty revoking their broadcasted state!) and accept it
without recourse as long as the ChannelMonitor receives the block
first, the full commitment update dance occurs after the block is
connected, and before the ChannelManager receives the block.

Obviously this is an incredibly contrived race given the
counterparty would be risking their full channel balance for it,
but its worth fixing nonetheless as it makes the potential
ChannelMonitor states simpler to reason about.

The test in this commit also tests the behavior changed in the
previous commit.

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -700,6 +700,11 @@ pub(crate) struct ChannelMonitorImpl<Signer: Sign> {
 	// remote monitor out-of-order with regards to the block view.
 	holder_tx_signed: bool,
 
+	// If a spend of the funding output is seen, we set this to true and reject any further
+	// updates. This prevents any further changes in the offchain state no matter the order
+	// of block connection between ChannelMonitors and the ChannelManager.
+	funding_spend_seen: bool,
+
 	funding_spend_confirmed: Option<Txid>,
 	/// The set of HTLCs which have been either claimed or failed on chain and have reached
 	/// the requisite confirmations on the claim/fail transaction (either ANTI_REORG_DELAY or the
@@ -765,6 +770,7 @@ impl<Signer: Sign> PartialEq for ChannelMonitorImpl<Signer> {
 			self.outputs_to_watch != other.outputs_to_watch ||
 			self.lockdown_from_offchain != other.lockdown_from_offchain ||
 			self.holder_tx_signed != other.holder_tx_signed ||
+			self.funding_spend_seen != other.funding_spend_seen ||
 			self.funding_spend_confirmed != other.funding_spend_confirmed ||
 			self.htlcs_resolved_on_chain != other.htlcs_resolved_on_chain
 		{
@@ -940,6 +946,7 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
 			(1, self.funding_spend_confirmed, option),
 			(3, self.htlcs_resolved_on_chain, vec_type),
 			(5, self.pending_monitor_events, vec_type),
+			(7, self.funding_spend_seen, required),
 		});
 
 		Ok(())
@@ -1038,6 +1045,7 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
 
 				lockdown_from_offchain: false,
 				holder_tx_signed: false,
+				funding_spend_seen: false,
 				funding_spend_confirmed: None,
 				htlcs_resolved_on_chain: Vec::new(),
 
@@ -1843,6 +1851,10 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 			}
 		}
 		self.latest_update_id = updates.update_id;
+
+		if ret.is_ok() && self.funding_spend_seen {
+			ret = Err(MonitorUpdateError("Counterparty attempted to update commitment after funding was spent"));
+		}
 		ret
 	}
 
@@ -2271,6 +2283,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 				if prevout.txid == self.funding_info.0.txid && prevout.vout == self.funding_info.0.index as u32 {
 					let mut balance_spendable_csv = None;
 					log_info!(logger, "Channel closed by funding output spend in txid {}.", log_bytes!(tx.txid()));
+					self.funding_spend_seen = true;
 					if (tx.input[0].sequence >> 8*3) as u8 == 0x80 && (tx.lock_time >> 8*3) as u8 == 0x20 {
 						let (mut new_outpoints, new_outputs) = self.check_spend_counterparty_transaction(&tx, height, &logger);
 						if !new_outputs.1.is_empty() {
@@ -3120,10 +3133,12 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
 
 		let mut funding_spend_confirmed = None;
 		let mut htlcs_resolved_on_chain = Some(Vec::new());
+		let mut funding_spend_seen = Some(false);
 		read_tlv_fields!(reader, {
 			(1, funding_spend_confirmed, option),
 			(3, htlcs_resolved_on_chain, vec_type),
 			(5, pending_monitor_events, vec_type),
+			(7, funding_spend_seen, option),
 		});
 
 		let mut secp_ctx = Secp256k1::new();
@@ -3173,6 +3188,7 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
 
 				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(),
 
@@ -3186,6 +3202,7 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
 
 #[cfg(test)]
 mod tests {
+	use bitcoin::blockdata::block::BlockHeader;
 	use bitcoin::blockdata::script::{Script, Builder};
 	use bitcoin::blockdata::opcodes;
 	use bitcoin::blockdata::transaction::{Transaction, TxIn, TxOut, SigHashType};
@@ -3194,24 +3211,125 @@ mod tests {
 	use bitcoin::hashes::Hash;
 	use bitcoin::hashes::sha256::Hash as Sha256;
 	use bitcoin::hashes::hex::FromHex;
-	use bitcoin::hash_types::Txid;
+	use bitcoin::hash_types::{BlockHash, Txid};
 	use bitcoin::network::constants::Network;
+	use bitcoin::secp256k1::key::{SecretKey,PublicKey};
+	use bitcoin::secp256k1::Secp256k1;
+
 	use hex;
-	use chain::BestBlock;
+
+	use super::ChannelMonitorUpdateStep;
+	use ::{check_added_monitors, check_closed_broadcast, check_closed_event, check_spends, get_local_commitment_txn, get_monitor, get_route_and_payment_hash, unwrap_send_err};
+	use chain::{BestBlock, Confirm};
 	use chain::channelmonitor::ChannelMonitor;
 	use chain::package::{WEIGHT_OFFERED_HTLC, WEIGHT_RECEIVED_HTLC, WEIGHT_REVOKED_OFFERED_HTLC, WEIGHT_REVOKED_RECEIVED_HTLC, WEIGHT_REVOKED_OUTPUT};
 	use chain::transaction::OutPoint;
+	use chain::keysinterface::InMemorySigner;
 	use ln::{PaymentPreimage, PaymentHash};
 	use ln::chan_utils;
 	use ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, ChannelTransactionParameters, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters};
+	use ln::channelmanager::PaymentSendFailure;
+	use ln::features::InitFeatures;
+	use ln::functional_test_utils::*;
 	use ln::script::ShutdownScript;
+	use util::errors::APIError;
+	use util::events::{ClosureReason, MessageSendEventsProvider};
 	use util::test_utils::{TestLogger, TestBroadcaster, TestFeeEstimator};
-	use bitcoin::secp256k1::key::{SecretKey,PublicKey};
-	use bitcoin::secp256k1::Secp256k1;
+	use util::ser::{ReadableArgs, Writeable};
 	use sync::{Arc, Mutex};
-	use chain::keysinterface::InMemorySigner;
+	use io;
 	use prelude::*;
 
+	fn do_test_funding_spend_refuses_updates(use_local_txn: bool) {
+		// Previously, monitor updates were allowed freely even after a funding-spend transaction
+		// confirmed. This would allow a race condition where we could receive a payment (including
+		// the counterparty revoking their broadcasted state!) and accept it without recourse as
+		// long as the ChannelMonitor receives the block first, the full commitment update dance
+		// occurs after the block is connected, and before the ChannelManager receives the block.
+		// Obviously this is an incredibly contrived race given the counterparty would be risking
+		// their full channel balance for it, but its worth fixing nonetheless as it makes the
+		// potential ChannelMonitor states simpler to reason about.
+		//
+		// This test checks said behavior, as well as ensuring a ChannelMonitorUpdate with multiple
+		// updates is handled correctly in such conditions.
+		let chanmon_cfgs = create_chanmon_cfgs(3);
+		let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+		let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+		let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+		let channel = create_announced_chan_between_nodes(
+			&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
+		create_announced_chan_between_nodes(
+			&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
+
+		// Rebalance somewhat
+		send_payment(&nodes[0], &[&nodes[1]], 10_000_000);
+
+		// First route two payments for testing at the end
+		let payment_preimage_1 = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000).0;
+		let payment_preimage_2 = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000).0;
+
+		let local_txn = get_local_commitment_txn!(nodes[1], channel.2);
+		assert_eq!(local_txn.len(), 1);
+		let remote_txn = get_local_commitment_txn!(nodes[0], channel.2);
+		assert_eq!(remote_txn.len(), 3); // Commitment and two HTLC-Timeouts
+		check_spends!(remote_txn[1], remote_txn[0]);
+		check_spends!(remote_txn[2], remote_txn[0]);
+		let broadcast_tx = if use_local_txn { &local_txn[0] } else { &remote_txn[0] };
+
+		// Connect a commitment transaction, but only to the ChainMonitor/ChannelMonitor. The
+		// channel is now closed, but the ChannelManager doesn't know that yet.
+		let new_header = BlockHeader {
+			version: 2, time: 0, bits: 0, nonce: 0,
+			prev_blockhash: nodes[0].best_block_info().0,
+			merkle_root: Default::default() };
+		let conf_height = nodes[0].best_block_info().1 + 1;
+		nodes[1].chain_monitor.chain_monitor.transactions_confirmed(&new_header,
+			&[(0, broadcast_tx)], conf_height);
+
+		let (_, pre_update_monitor) = <(BlockHash, ChannelMonitor<InMemorySigner>)>::read(
+						&mut io::Cursor::new(&get_monitor!(nodes[1], channel.2).encode()),
+						&nodes[1].keys_manager.backing).unwrap();
+
+		// If the ChannelManager tries to update the channel, however, the ChainMonitor will pass
+		// the update through to the ChannelMonitor which will refuse it (as the channel is closed).
+		let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 100_000);
+		unwrap_send_err!(nodes[1].node.send_payment(&route, payment_hash, &Some(payment_secret)),
+			true, APIError::ChannelUnavailable { ref err },
+			assert!(err.contains("ChannelMonitor storage failure")));
+		check_added_monitors!(nodes[1], 2); // After the failure we generate a close-channel monitor update
+		check_closed_broadcast!(nodes[1], true);
+		check_closed_event!(nodes[1], 1, ClosureReason::ProcessingError { err: "ChannelMonitor storage failure".to_string() });
+
+		let monitor_updates = nodes[1].chain_monitor.monitor_updates.lock().unwrap();
+		let mut replay_update = monitor_updates.get(&channel.2).unwrap().iter().rev().skip(1).next().unwrap().clone();
+		assert_eq!(replay_update.updates.len(), 1);
+		if let ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { .. } = replay_update.updates[0] {
+		} else { panic!(); }
+		replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage: payment_preimage_1 });
+		replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage: payment_preimage_2 });
+
+		let broadcaster = TestBroadcaster::new(Arc::clone(&nodes[1].blocks));
+		assert!(
+			pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &&chanmon_cfgs[1].fee_estimator, &nodes[1].logger)
+			.is_err());
+		// Even though we error'd on the first update, we should still have generated an HTLC claim
+		// transaction
+		let txn_broadcasted = broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
+		assert!(txn_broadcasted.len() >= 2);
+		let htlc_txn = txn_broadcasted.iter().filter(|tx| {
+			assert_eq!(tx.input.len(), 1);
+			tx.input[0].previous_output.txid == broadcast_tx.txid()
+		}).collect::<Vec<_>>();
+		assert_eq!(htlc_txn.len(), 2);
+		check_spends!(htlc_txn[0], broadcast_tx);
+		check_spends!(htlc_txn[1], broadcast_tx);
+	}
+	#[test]
+	fn test_funding_spend_refuses_updates() {
+		do_test_funding_spend_refuses_updates(true);
+		do_test_funding_spend_refuses_updates(false);
+	}
+
 	#[test]
 	fn test_prune_preimages() {
 		let secp_ctx = Secp256k1::new();

lightning/src/util/test_utils.rs

@@ -89,6 +89,7 @@ impl keysinterface::KeysInterface for OnlyReadsKeysInterface {
 
 pub struct TestChainMonitor<'a> {
 	pub added_monitors: Mutex<Vec<(OutPoint, channelmonitor::ChannelMonitor<EnforcingSigner>)>>,
+	pub monitor_updates: Mutex<HashMap<[u8; 32], Vec<channelmonitor::ChannelMonitorUpdate>>>,
 	pub latest_monitor_update_id: Mutex<HashMap<[u8; 32], (OutPoint, u64, MonitorUpdateId)>>,
 	pub chain_monitor: chainmonitor::ChainMonitor<EnforcingSigner, &'a TestChainSource, &'a chaininterface::BroadcasterInterface, &'a TestFeeEstimator, &'a TestLogger, &'a chainmonitor::Persist<EnforcingSigner>>,
 	pub keys_manager: &'a TestKeysInterface,
@@ -101,6 +102,7 @@ impl<'a> TestChainMonitor<'a> {
 	pub fn new(chain_source: Option<&'a TestChainSource>, broadcaster: &'a chaininterface::BroadcasterInterface, logger: &'a TestLogger, fee_estimator: &'a TestFeeEstimator, persister: &'a chainmonitor::Persist<EnforcingSigner>, keys_manager: &'a TestKeysInterface) -> Self {
 		Self {
 			added_monitors: Mutex::new(Vec::new()),
+			monitor_updates: Mutex::new(HashMap::new()),
 			latest_monitor_update_id: Mutex::new(HashMap::new()),
 			chain_monitor: chainmonitor::ChainMonitor::new(chain_source, broadcaster, logger, fee_estimator, persister),
 			keys_manager,
@@ -130,6 +132,8 @@ impl<'a> chain::Watch<EnforcingSigner> for TestChainMonitor<'a> {
 		assert!(channelmonitor::ChannelMonitorUpdate::read(
 				&mut io::Cursor::new(&w.0)).unwrap() == update);
 
+		self.monitor_updates.lock().unwrap().entry(funding_txo.to_channel_id()).or_insert(Vec::new()).push(update.clone());
+
 		if let Some(exp) = self.expect_channel_force_closed.lock().unwrap().take() {
 			assert_eq!(funding_txo.to_channel_id(), exp.0);
 			assert_eq!(update.updates.len(), 1);
@@ -209,6 +213,13 @@ pub struct TestBroadcaster {
 	pub txn_broadcasted: Mutex<Vec<Transaction>>,
 	pub blocks: Arc<Mutex<Vec<(BlockHeader, u32)>>>,
 }
+
+impl TestBroadcaster {
+	pub fn new(blocks: Arc<Mutex<Vec<(BlockHeader, u32)>>>) -> TestBroadcaster {
+		TestBroadcaster { txn_broadcasted: Mutex::new(Vec::new()), blocks }
+	}
+}
+
 impl chaininterface::BroadcasterInterface for TestBroadcaster {
 	fn broadcast_transaction(&self, tx: &Transaction) {
 		assert!(tx.lock_time < 1_500_000_000);