From 25542b8157e95e362e097b73a366da3f8bfe962d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 17 Oct 2021 21:28:50 +0000 Subject: [PATCH] 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. --- lightning/src/chain/channelmonitor.rs | 131 +++++++++++++++++++++++++- lightning/src/util/test_utils.rs | 11 +++ 2 files changed, 137 insertions(+), 5 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index dd33e7d0d..2ae4bd3de 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -695,6 +695,11 @@ pub(crate) struct ChannelMonitorImpl { // 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, /// 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 PartialEq for ChannelMonitorImpl { 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 Writeable for ChannelMonitorImpl { (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 ChannelMonitor { 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 ChannelMonitorImpl { } } 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 ChannelMonitorImpl { 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> 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> 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> 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)>::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::>(); + 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(); diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index e2e44a2c1..cc8c20698 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -91,6 +91,7 @@ impl keysinterface::KeysInterface for OnlyReadsKeysInterface { pub struct TestChainMonitor<'a> { pub added_monitors: Mutex)>>, + pub monitor_updates: Mutex>>, pub latest_monitor_update_id: Mutex>, pub chain_monitor: chainmonitor::ChainMonitor>, 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, 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 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>, pub blocks: Arc>>, } + +impl TestBroadcaster { + pub fn new(blocks: Arc>>) -> 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); -- 2.39.5