Always return failure in `update_monitor` after funding spend
authorMatt Corallo <git@bluematt.me>
Sun, 17 Oct 2021 21:28:50 +0000 (21:28 +0000)
committerMatt Corallo <git@bluematt.me>
Mon, 6 Dec 2021 18:39:37 +0000 (18:39 +0000)
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.

lightning/src/chain/channelmonitor.rs
lightning/src/util/test_utils.rs

index dd33e7d0d02ea4d6f1e33d99ea0ddac75bfcf3c6..2ae4bd3de1d042f638e353ca1c680476efbe1b27 100644 (file)
@@ -695,6 +695,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
@@ -760,6 +765,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
                {
@@ -935,6 +941,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(())
@@ -1033,6 +1040,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(),
 
@@ -1937,6 +1945,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
        }
 
@@ -2366,6 +2378,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                        let mut balance_spendable_csv = None;
                                        log_info!(logger, "Channel {} closed by funding output spend in txid {}.",
                                                log_bytes!(self.funding_info.0.to_channel_id()), 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() {
@@ -3215,10 +3228,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();
@@ -3268,6 +3283,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(),
 
@@ -3281,6 +3297,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};
@@ -3289,24 +3306,128 @@ 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() });
+
+               // Build a new ChannelMonitorUpdate which contains both the failing commitment tx update
+               // and provides the claim preimages for the two pending HTLCs. The first update generates
+               // an error, but the point of this test is to ensure the later updates are still applied.
+               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();
index e2e44a2c1a631e5adc8138b6f3ae15965753a83c..cc8c206980176bd030ca4de08645583fcb3e58d1 100644 (file)
@@ -91,6 +91,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,
@@ -103,6 +104,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,
@@ -132,6 +134,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);
@@ -211,6 +215,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);