From 63698ecbbfec96998257424e0b09224182a81239 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 20 Oct 2021 01:35:01 +0000 Subject: [PATCH 1/9] Allow missing-docs on test-only macros --- lightning/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/lib.rs b/lightning/src/lib.rs index e6ecd1f3563..3338803f9a0 100644 --- a/lightning/src/lib.rs +++ b/lightning/src/lib.rs @@ -18,7 +18,7 @@ //! generated/etc. This makes it a good candidate for tight integration into an existing wallet //! instead of having a rather-separate lightning appendage to a wallet. -#![cfg_attr(not(any(feature = "fuzztarget", feature = "_test_utils")), deny(missing_docs))] +#![cfg_attr(not(any(test, feature = "fuzztarget", feature = "_test_utils")), deny(missing_docs))] #![cfg_attr(not(any(test, feature = "fuzztarget", feature = "_test_utils")), forbid(unsafe_code))] #![deny(broken_intra_doc_links)] From 1ce922c631aac879b3569047a8f17e2a5df5e76a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 17 Oct 2021 21:23:51 +0000 Subject: [PATCH 2/9] Prefer fully-specified paths in test macros This avoids macros being context-specific use-dependent. --- lightning-persister/src/lib.rs | 3 +- lightning/src/ln/chanmon_update_fail_tests.rs | 6 +-- lightning/src/ln/functional_test_utils.rs | 49 ++++++++++++------- lightning/src/ln/functional_tests.rs | 3 -- lightning/src/ln/monitor_tests.rs | 7 +-- lightning/src/ln/onion_route_tests.rs | 3 +- lightning/src/ln/payment_tests.rs | 5 +- lightning/src/ln/reorg_tests.rs | 5 +- lightning/src/ln/shutdown_tests.rs | 4 -- 9 files changed, 39 insertions(+), 46 deletions(-) diff --git a/lightning-persister/src/lib.rs b/lightning-persister/src/lib.rs index eba0692485a..b853b5796b6 100644 --- a/lightning-persister/src/lib.rs +++ b/lightning-persister/src/lib.rs @@ -191,8 +191,7 @@ mod tests { use lightning::{check_closed_broadcast, check_closed_event, check_added_monitors}; use lightning::ln::features::InitFeatures; use lightning::ln::functional_test_utils::*; - use lightning::ln::msgs::ErrorAction; - use lightning::util::events::{ClosureReason, Event, MessageSendEventsProvider, MessageSendEvent}; + use lightning::util::events::{ClosureReason, MessageSendEventsProvider}; use lightning::util::test_utils; use std::fs; #[cfg(target_os = "windows")] diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index cb39b64a448..236b1e498f4 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -19,11 +19,10 @@ use bitcoin::network::constants::Network; use chain::channelmonitor::ChannelMonitor; use chain::transaction::OutPoint; use chain::{ChannelMonitorUpdateErr, Listen, Watch}; -use ln::{PaymentPreimage, PaymentHash}; use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure}; use ln::features::InitFeatures; use ln::msgs; -use ln::msgs::{ChannelMessageHandler, ErrorAction, RoutingMessageHandler}; +use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler}; use util::config::UserConfig; use util::enforcing_trait_impls::EnforcingSigner; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose, ClosureReason}; @@ -31,9 +30,6 @@ use util::errors::APIError; use util::ser::{ReadableArgs, Writeable}; use util::test_utils::TestBroadcaster; -use bitcoin::hashes::sha256::Hash as Sha256; -use bitcoin::hashes::Hash; - use ln::functional_test_utils::*; use util::test_utils; diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index bf3e192b252..000c31964d2 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -33,8 +33,6 @@ use bitcoin::blockdata::constants::genesis_block; use bitcoin::blockdata::transaction::{Transaction, TxOut}; use bitcoin::network::constants::Network; -use bitcoin::hashes::sha256::Hash as Sha256; -use bitcoin::hashes::Hash; use bitcoin::hash_types::BlockHash; use bitcoin::secp256k1::key::PublicKey; @@ -337,9 +335,11 @@ pub fn create_chan_between_nodes_with_value<'a, 'b, 'c, 'd>(node_a: &'a Node<'b, (announcement, as_update, bs_update, channel_id, tx) } +#[macro_export] macro_rules! get_revoke_commit_msgs { ($node: expr, $node_id: expr) => { { + use util::events::MessageSendEvent; let events = $node.node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 2); (match events[0] { @@ -401,13 +401,14 @@ macro_rules! get_event { } #[cfg(test)] +#[macro_export] macro_rules! get_htlc_update_msgs { ($node: expr, $node_id: expr) => { { let events = $node.node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); match events[0] { - MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => { + $crate::util::events::MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => { assert_eq!(*node_id, $node_id); (*updates).clone() }, @@ -704,6 +705,7 @@ pub fn update_nodes_with_chan_announce<'a, 'b, 'c, 'd>(nodes: &'a Vec { { @@ -768,6 +770,9 @@ macro_rules! get_closing_signed_broadcast { #[macro_export] macro_rules! check_closed_broadcast { ($node: expr, $with_error_msg: expr) => {{ + use $crate::util::events::MessageSendEvent; + use $crate::ln::msgs::ErrorAction; + let msg_events = $node.node.get_and_clear_pending_msg_events(); assert_eq!(msg_events.len(), if $with_error_msg { 2 } else { 1 }); match msg_events[0] { @@ -795,6 +800,8 @@ macro_rules! check_closed_event { check_closed_event!($node, $events, $reason, false); }; ($node: expr, $events: expr, $reason: expr, $is_check_discard_funding: expr) => {{ + use $crate::util::events::Event; + let events = $node.node.get_and_clear_pending_events(); assert_eq!(events.len(), $events); let expected_reason = $reason; @@ -1003,10 +1010,12 @@ macro_rules! get_payment_preimage_hash { }; ($dest_node: expr, $min_value_msat: expr) => { { + use bitcoin::hashes::Hash as _; let mut payment_count = $dest_node.network_payment_count.borrow_mut(); - let payment_preimage = PaymentPreimage([*payment_count; 32]); + let payment_preimage = $crate::ln::PaymentPreimage([*payment_count; 32]); *payment_count += 1; - let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()); + let payment_hash = $crate::ln::PaymentHash( + bitcoin::hashes::sha256::Hash::hash(&payment_preimage.0[..]).into_inner()); let payment_secret = $dest_node.node.create_inbound_payment_for_hash(payment_hash, $min_value_msat, 7200).unwrap(); (payment_preimage, payment_hash, payment_secret) } @@ -1014,17 +1023,18 @@ macro_rules! get_payment_preimage_hash { } #[cfg(test)] +#[macro_export] macro_rules! get_route_and_payment_hash { ($send_node: expr, $recv_node: expr, $recv_value: expr) => {{ - get_route_and_payment_hash!($send_node, $recv_node, vec![], $recv_value, TEST_FINAL_CLTV) + $crate::get_route_and_payment_hash!($send_node, $recv_node, vec![], $recv_value, TEST_FINAL_CLTV) }}; ($send_node: expr, $recv_node: expr, $last_hops: expr, $recv_value: expr, $cltv: expr) => {{ - let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash!($recv_node, Some($recv_value)); + let (payment_preimage, payment_hash, payment_secret) = $crate::get_payment_preimage_hash!($recv_node, Some($recv_value)); let payee = $crate::routing::router::Payee::from_node_id($recv_node.node.get_our_node_id()) .with_features($crate::ln::features::InvoiceFeatures::known()) .with_route_hints($last_hops); - let scorer = ::util::test_utils::TestScorer::with_fixed_penalty(0); - let route = ::routing::router::get_route( + let scorer = $crate::util::test_utils::TestScorer::with_fixed_penalty(0); + let route = $crate::routing::router::get_route( &$send_node.node.get_our_node_id(), &payee, $send_node.network_graph, Some(&$send_node.node.list_usable_channels().iter().collect::>()), $recv_value, $cltv, $send_node.logger, &scorer @@ -1088,6 +1098,7 @@ macro_rules! expect_payment_received { } #[cfg(test)] +#[macro_export] macro_rules! expect_payment_sent_without_paths { ($node: expr, $expected_payment_preimage: expr) => { expect_payment_sent!($node, $expected_payment_preimage, None::, false); @@ -1097,23 +1108,26 @@ macro_rules! expect_payment_sent_without_paths { } } +#[macro_export] macro_rules! expect_payment_sent { ($node: expr, $expected_payment_preimage: expr) => { - expect_payment_sent!($node, $expected_payment_preimage, None::, true); + $crate::expect_payment_sent!($node, $expected_payment_preimage, None::, true); }; ($node: expr, $expected_payment_preimage: expr, $expected_fee_msat_opt: expr) => { - expect_payment_sent!($node, $expected_payment_preimage, $expected_fee_msat_opt, true); + $crate::expect_payment_sent!($node, $expected_payment_preimage, $expected_fee_msat_opt, true); }; - ($node: expr, $expected_payment_preimage: expr, $expected_fee_msat_opt: expr, $expect_paths: expr) => { + ($node: expr, $expected_payment_preimage: expr, $expected_fee_msat_opt: expr, $expect_paths: expr) => { { + use bitcoin::hashes::Hash as _; let events = $node.node.get_and_clear_pending_events(); - let expected_payment_hash = PaymentHash(Sha256::hash(&$expected_payment_preimage.0).into_inner()); + let expected_payment_hash = $crate::ln::PaymentHash( + bitcoin::hashes::sha256::Hash::hash(&$expected_payment_preimage.0).into_inner()); if $expect_paths { assert!(events.len() > 1); } else { assert_eq!(events.len(), 1); } let expected_payment_id = match events[0] { - Event::PaymentSent { ref payment_id, ref payment_preimage, ref payment_hash, ref fee_paid_msat } => { + $crate::util::events::Event::PaymentSent { ref payment_id, ref payment_preimage, ref payment_hash, ref fee_paid_msat } => { assert_eq!($expected_payment_preimage, *payment_preimage); assert_eq!(expected_payment_hash, *payment_hash); assert!(fee_paid_msat.is_some()); @@ -1127,7 +1141,7 @@ macro_rules! expect_payment_sent { if $expect_paths { for i in 1..events.len() { match events[i] { - Event::PaymentPathSuccessful { payment_id, payment_hash, .. } => { + $crate::util::events::Event::PaymentPathSuccessful { payment_id, payment_hash, .. } => { assert_eq!(payment_id, expected_payment_id); assert_eq!(payment_hash, Some(expected_payment_hash)); }, @@ -1135,16 +1149,17 @@ macro_rules! expect_payment_sent { } } } - } + } } } #[cfg(test)] +#[macro_export] macro_rules! expect_payment_path_successful { ($node: expr) => { let events = $node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { - Event::PaymentPathSuccessful { .. } => {}, + $crate::util::events::Event::PaymentPathSuccessful { .. } => {}, _ => panic!("Unexpected event"), } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index f144dbd9ad6..37c319c5799 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -42,9 +42,6 @@ use bitcoin::blockdata::opcodes; use bitcoin::blockdata::constants::genesis_block; use bitcoin::network::constants::Network; -use bitcoin::hashes::sha256::Hash as Sha256; -use bitcoin::hashes::Hash; - use bitcoin::secp256k1::Secp256k1; use bitcoin::secp256k1::key::{PublicKey,SecretKey}; diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index cfde643bcb9..d836b736a98 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -11,16 +11,13 @@ use chain::channelmonitor::{ANTI_REORG_DELAY, Balance}; use chain::transaction::OutPoint; -use ln::{channel, PaymentPreimage, PaymentHash}; +use ln::channel; use ln::channelmanager::BREAKDOWN_TIMEOUT; use ln::features::InitFeatures; -use ln::msgs::{ChannelMessageHandler, ErrorAction}; +use ln::msgs::ChannelMessageHandler; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason}; use routing::network_graph::NetworkUpdate; -use bitcoin::hashes::sha256::Hash as Sha256; -use bitcoin::hashes::Hash; - use bitcoin::blockdata::script::Builder; use bitcoin::blockdata::opcodes; use bitcoin::secp256k1::Secp256k1; diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index e80cf98d7b9..05b108fbf25 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -12,7 +12,7 @@ //! returned errors decode to the correct thing. use chain::channelmonitor::{CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS}; -use ln::{PaymentPreimage, PaymentHash, PaymentSecret}; +use ln::{PaymentHash, PaymentSecret}; use ln::channelmanager::{HTLCForwardInfo, CLTV_FAR_FAR_AWAY}; use ln::onion_utils; use routing::network_graph::NetworkUpdate; @@ -26,7 +26,6 @@ use util::config::UserConfig; use bitcoin::hash_types::BlockHash; -use bitcoin::hashes::sha256::Hash as Sha256; use bitcoin::hashes::Hash; use bitcoin::secp256k1; diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 7c9ff3199d1..518ccd3b0ef 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -14,11 +14,10 @@ use chain::{ChannelMonitorUpdateErr, Confirm, Listen, Watch}; use chain::channelmonitor::{ANTI_REORG_DELAY, ChannelMonitor, LATENCY_GRACE_PERIOD_BLOCKS}; use chain::transaction::OutPoint; -use ln::{PaymentPreimage, PaymentHash}; use ln::channelmanager::{BREAKDOWN_TIMEOUT, ChannelManager, ChannelManagerReadArgs, PaymentId, PaymentSendFailure}; use ln::features::InitFeatures; use ln::msgs; -use ln::msgs::{ChannelMessageHandler, ErrorAction}; +use ln::msgs::ChannelMessageHandler; use util::events::{ClosureReason, Event, MessageSendEvent, MessageSendEventsProvider}; use util::test_utils; use util::errors::APIError; @@ -26,8 +25,6 @@ use util::enforcing_trait_impls::EnforcingSigner; use util::ser::{ReadableArgs, Writeable}; use io; -use bitcoin::hashes::sha256::Hash as Sha256; -use bitcoin::hashes::Hash; use bitcoin::{Block, BlockHeader, BlockHash}; use prelude::*; diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index 5cd1a890c49..570dc3d0eee 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -12,10 +12,9 @@ use chain::channelmonitor::{ANTI_REORG_DELAY, ChannelMonitor}; use chain::transaction::OutPoint; use chain::{Confirm, Watch}; -use ln::PaymentHash; use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs}; use ln::features::InitFeatures; -use ln::msgs::{ChannelMessageHandler, ErrorAction}; +use ln::msgs::ChannelMessageHandler; use routing::network_graph::NetworkUpdate; use util::enforcing_trait_impls::EnforcingSigner; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason}; @@ -25,8 +24,6 @@ use util::ser::{ReadableArgs, Writeable}; use bitcoin::blockdata::block::{Block, BlockHeader}; use bitcoin::blockdata::script::Builder; use bitcoin::blockdata::opcodes; -use bitcoin::hashes::sha256::Hash as Sha256; -use bitcoin::hashes::Hash; use bitcoin::hash_types::BlockHash; use bitcoin::secp256k1::Secp256k1; diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index db76a2c760c..ffd809bc70d 100644 --- a/lightning/src/ln/shutdown_tests.rs +++ b/lightning/src/ln/shutdown_tests.rs @@ -11,7 +11,6 @@ use chain::keysinterface::KeysInterface; use chain::transaction::OutPoint; -use ln::{PaymentPreimage, PaymentHash}; use ln::channelmanager::PaymentSendFailure; use routing::router::{Payee, get_route}; use routing::network_graph::NetworkUpdate; @@ -28,9 +27,6 @@ use util::config::UserConfig; use bitcoin::blockdata::script::Builder; use bitcoin::blockdata::opcodes; -use bitcoin::hashes::sha256::Hash as Sha256; -use bitcoin::hashes::Hash; - use regex; use core::default::Default; From 37c6c18789151b2a8c7c3b7e4c1d98f7f86e906a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 17 Oct 2021 21:26:04 +0000 Subject: [PATCH 3/9] Continue after a single failure in `ChannelMonitor::update_monitor` `ChannelMonitorUpdate`s may contain multiple updates, including, eg a payment preimage after a commitment transaction update. While such updates are generally not generated today, we shouldn't return early out of the update loop, causing us to miss any updates after an earlier update fails. --- lightning/src/chain/channelmonitor.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 1699e482723..dd33e7d0d02 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1886,12 +1886,17 @@ impl ChannelMonitorImpl { } else if self.latest_update_id + 1 != updates.update_id { panic!("Attempted to apply ChannelMonitorUpdates out of order, check the update_id before passing an update to update_monitor!"); } + let mut ret = Ok(()); for update in updates.updates.iter() { match update { ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs } => { log_trace!(logger, "Updating ChannelMonitor with latest holder commitment transaction info"); if self.lockdown_from_offchain { panic!(); } - self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone())? + if let Err(e) = self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone()) { + log_error!(logger, "Providing latest holder commitment transaction failed/was refused:"); + log_error!(logger, " {}", e.0); + ret = Err(e); + } } ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid, htlc_outputs, commitment_number, their_revocation_point } => { log_trace!(logger, "Updating ChannelMonitor with latest counterparty commitment transaction info"); @@ -1903,7 +1908,11 @@ impl ChannelMonitorImpl { }, ChannelMonitorUpdateStep::CommitmentSecret { idx, secret } => { log_trace!(logger, "Updating ChannelMonitor with commitment secret"); - self.provide_secret(*idx, *secret)? + if let Err(e) = self.provide_secret(*idx, *secret) { + log_error!(logger, "Providing latest counterparty commitment secret failed/was refused:"); + log_error!(logger, " {}", e.0); + ret = Err(e); + } }, ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast } => { log_trace!(logger, "Updating ChannelMonitor: channel force closed, should broadcast: {}", should_broadcast); @@ -1928,7 +1937,7 @@ impl ChannelMonitorImpl { } } self.latest_update_id = updates.update_id; - Ok(()) + ret } pub fn get_latest_update_id(&self) -> u64 { From 25542b8157e95e362e097b73a366da3f8bfe962d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 17 Oct 2021 21:28:50 +0000 Subject: [PATCH 4/9] 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 dd33e7d0d02..2ae4bd3de1d 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 e2e44a2c1a6..cc8c2069801 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); From 87b687962237c3bfc596a0963b1ef2bc2e9ba6f7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 29 Nov 2021 21:36:12 +0000 Subject: [PATCH 5/9] Drop `MonitorUpdateErr` in favor of opaque errors. We don't expect users to ever change behavior based on the string contained in a `MonitorUpdateErr`, except log it, so there's little reason to not just log it ourselves and return a `()` for errors. We do so here, simplifying the callsite in `ChainMonitor` as well. --- lightning/src/chain/chainmonitor.rs | 4 +-- lightning/src/chain/channelmonitor.rs | 44 +++++++++++---------------- 2 files changed, 19 insertions(+), 29 deletions(-) diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index 3c8f9bb561b..4ff3bba11a7 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -639,8 +639,8 @@ where C::Target: chain::Filter, let monitor = &monitor_state.monitor; log_trace!(self.logger, "Updating ChannelMonitor for channel {}", log_funding_info!(monitor)); let update_res = monitor.update_monitor(&update, &self.broadcaster, &self.fee_estimator, &self.logger); - if let Err(e) = &update_res { - log_error!(self.logger, "Failed to update ChannelMonitor for channel {}: {:?}", log_funding_info!(monitor), e); + if update_res.is_err() { + log_error!(self.logger, "Failed to update ChannelMonitor for channel {}.", log_funding_info!(monitor)); } // Even if updating the monitor returns an error, the monitor's state will // still be changed. So, persist the updated monitor despite the error. diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 2ae4bd3de1d..5f456c7bfe8 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -115,14 +115,6 @@ impl Readable for ChannelMonitorUpdate { } } -/// General Err type for ChannelMonitor actions. Generally, this implies that the data provided is -/// inconsistent with the ChannelMonitor being called. eg for ChannelMonitor::update_monitor this -/// means you tried to update a monitor for a different channel or the ChannelMonitorUpdate was -/// corrupted. -/// Contains a developer-readable error message. -#[derive(Clone, Debug)] -pub struct MonitorUpdateError(pub &'static str); - /// An event to be processed by the ChannelManager. #[derive(Clone, PartialEq)] pub enum MonitorEvent { @@ -1052,7 +1044,7 @@ impl ChannelMonitor { } #[cfg(test)] - fn provide_secret(&self, idx: u64, secret: [u8; 32]) -> Result<(), MonitorUpdateError> { + fn provide_secret(&self, idx: u64, secret: [u8; 32]) -> Result<(), &'static str> { self.inner.lock().unwrap().provide_secret(idx, secret) } @@ -1074,12 +1066,10 @@ impl ChannelMonitor { #[cfg(test)] fn provide_latest_holder_commitment_tx( - &self, - holder_commitment_tx: HolderCommitmentTransaction, + &self, holder_commitment_tx: HolderCommitmentTransaction, htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>, - ) -> Result<(), MonitorUpdateError> { - self.inner.lock().unwrap().provide_latest_holder_commitment_tx( - holder_commitment_tx, htlc_outputs) + ) -> Result<(), ()> { + self.inner.lock().unwrap().provide_latest_holder_commitment_tx(holder_commitment_tx, htlc_outputs).map_err(|_| ()) } #[cfg(test)] @@ -1120,7 +1110,7 @@ impl ChannelMonitor { broadcaster: &B, fee_estimator: &F, logger: &L, - ) -> Result<(), MonitorUpdateError> + ) -> Result<(), ()> where B::Target: BroadcasterInterface, F::Target: FeeEstimator, @@ -1695,9 +1685,9 @@ impl ChannelMonitorImpl { /// Inserts a revocation secret into this channel monitor. Prunes old preimages if neither /// needed by holder commitment transactions HTCLs nor by counterparty ones. Unless we haven't already seen /// counterparty commitment transaction's secret, they are de facto pruned (we can use revocation key). - fn provide_secret(&mut self, idx: u64, secret: [u8; 32]) -> Result<(), MonitorUpdateError> { + fn provide_secret(&mut self, idx: u64, secret: [u8; 32]) -> Result<(), &'static str> { if let Err(()) = self.commitment_secrets.provide_secret(idx, secret) { - return Err(MonitorUpdateError("Previous secret did not match new one")); + return Err("Previous secret did not match new one"); } // Prune HTLCs from the previous counterparty commitment tx so we don't generate failure/fulfill @@ -1789,7 +1779,7 @@ impl ChannelMonitorImpl { /// is important that any clones of this channel monitor (including remote clones) by kept /// up-to-date as our holder commitment transaction is updated. /// Panics if set_on_holder_tx_csv has never been called. - fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>) -> Result<(), MonitorUpdateError> { + fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>) -> Result<(), &'static str> { // block for Rust 1.34 compat let mut new_holder_commitment_tx = { let trusted_tx = holder_commitment_tx.trust(); @@ -1812,7 +1802,7 @@ impl ChannelMonitorImpl { mem::swap(&mut new_holder_commitment_tx, &mut self.current_holder_commitment_tx); self.prev_holder_signed_commitment_tx = Some(new_holder_commitment_tx); if self.holder_tx_signed { - return Err(MonitorUpdateError("Latest holder commitment signed has already been signed, update is rejected")); + return Err("Latest holder commitment signed has already been signed, update is rejected"); } Ok(()) } @@ -1876,7 +1866,7 @@ impl ChannelMonitorImpl { self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(self.funding_info.0)); } - pub fn update_monitor(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: &F, logger: &L) -> Result<(), MonitorUpdateError> + pub fn update_monitor(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: &F, logger: &L) -> Result<(), ()> where B::Target: BroadcasterInterface, F::Target: FeeEstimator, L::Target: Logger, @@ -1902,8 +1892,8 @@ impl ChannelMonitorImpl { if self.lockdown_from_offchain { panic!(); } if let Err(e) = self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone()) { log_error!(logger, "Providing latest holder commitment transaction failed/was refused:"); - log_error!(logger, " {}", e.0); - ret = Err(e); + log_error!(logger, " {}", e); + ret = Err(()); } } ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid, htlc_outputs, commitment_number, their_revocation_point } => { @@ -1918,8 +1908,8 @@ impl ChannelMonitorImpl { log_trace!(logger, "Updating ChannelMonitor with commitment secret"); if let Err(e) = self.provide_secret(*idx, *secret) { log_error!(logger, "Providing latest counterparty commitment secret failed/was refused:"); - log_error!(logger, " {}", e.0); - ret = Err(e); + log_error!(logger, " {}", e); + ret = Err(()); } }, ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast } => { @@ -1947,9 +1937,9 @@ 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 + log_error!(logger, "Refusing Channel Monitor Update as counterparty attempted to update commitment after funding was spent"); + Err(()) + } else { ret } } pub fn get_latest_update_id(&self) -> u64 { From 6bcb270ae10472a1dd28a3211e80257b1a2b1fd1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 17 Oct 2021 21:24:53 +0000 Subject: [PATCH 6/9] Make APIError debug output more clear by including the variant --- lightning/src/util/errors.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lightning/src/util/errors.rs b/lightning/src/util/errors.rs index 6495d9de4ed..0c6099f7d5a 100644 --- a/lightning/src/util/errors.rs +++ b/lightning/src/util/errors.rs @@ -66,10 +66,10 @@ pub enum APIError { impl fmt::Debug for APIError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match *self { - APIError::APIMisuseError {ref err} => f.write_str(err), + APIError::APIMisuseError {ref err} => write!(f, "Misuse error: {}", err), APIError::FeeRateTooHigh {ref err, ref feerate} => write!(f, "{} feerate: {}", err, feerate), - APIError::RouteError {ref err} => f.write_str(err), - APIError::ChannelUnavailable {ref err} => f.write_str(err), + APIError::RouteError {ref err} => write!(f, "Route error: {}", err), + APIError::ChannelUnavailable {ref err} => write!(f, "Channel unavailable: {}", err), APIError::MonitorUpdateFailed => f.write_str("Client indicated a channel monitor update failed"), APIError::IncompatibleShutdownScript { ref script } => { write!(f, "Provided a scriptpubkey format not accepted by peer: {}", script) From fa62775f9d2306051f0134a9d6d0183e9f246623 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 17 Oct 2021 21:24:39 +0000 Subject: [PATCH 7/9] Add a simple test for ChainMonitor MonitorUpdate-holding behavior --- lightning/src/chain/chainmonitor.rs | 87 ++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 2 deletions(-) diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index 4ff3bba11a7..ccce772f8a1 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -727,10 +727,16 @@ impl even #[cfg(test)] mod tests { - use ::{check_added_monitors, get_local_commitment_txn}; + use bitcoin::BlockHeader; + use ::{check_added_monitors, check_closed_broadcast, check_closed_event, expect_payment_sent}; + use ::{get_local_commitment_txn, get_route_and_payment_hash, unwrap_send_err}; + use chain::{ChannelMonitorUpdateErr, Confirm, Watch}; + use chain::channelmonitor::LATENCY_GRACE_PERIOD_BLOCKS; + use ln::channelmanager::PaymentSendFailure; use ln::features::InitFeatures; use ln::functional_test_utils::*; - use util::events::MessageSendEventsProvider; + use util::errors::APIError; + use util::events::{ClosureReason, MessageSendEventsProvider}; use util::test_utils::{OnRegisterOutput, TxOutReference}; /// Tests that in-block dependent transactions are processed by `block_connected` when not @@ -775,4 +781,81 @@ mod tests { nodes[1].node.get_and_clear_pending_msg_events(); nodes[1].node.get_and_clear_pending_events(); } + + fn do_chainsync_pauses_events(block_timeout: bool) { + // When a chainsync monitor update occurs, any MonitorUpdates should be held before being + // passed upstream to a `ChannelManager` via `Watch::release_pending_monitor_events`. This + // tests that behavior, as well as some ways it might go wrong. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + let channel = create_announced_chan_between_nodes( + &nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + + // Get a route for later and rebalance the channel somewhat + send_payment(&nodes[0], &[&nodes[1]], 10_000_000); + let (route, second_payment_hash, _, second_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 100_000); + + // First route a payment that we will claim on chain and give the recipient the preimage. + let payment_preimage = route_payment(&nodes[0], &[&nodes[1]], 1_000_000).0; + nodes[1].node.claim_funds(payment_preimage); + nodes[1].node.get_and_clear_pending_msg_events(); + check_added_monitors!(nodes[1], 1); + let remote_txn = get_local_commitment_txn!(nodes[1], channel.2); + assert_eq!(remote_txn.len(), 2); + + // Temp-fail the block connection which will hold the channel-closed event + chanmon_cfgs[0].persister.chain_sync_monitor_persistences.lock().unwrap().clear(); + chanmon_cfgs[0].persister.set_update_ret(Err(ChannelMonitorUpdateErr::TemporaryFailure)); + + // Connect B's 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() }; + nodes[0].chain_monitor.chain_monitor.transactions_confirmed(&new_header, + &[(0, &remote_txn[0]), (1, &remote_txn[1])], nodes[0].best_block_info().1 + 1); + assert!(nodes[0].chain_monitor.release_pending_monitor_events().is_empty()); + nodes[0].chain_monitor.chain_monitor.best_block_updated(&new_header, nodes[0].best_block_info().1 + 1); + assert!(nodes[0].chain_monitor.release_pending_monitor_events().is_empty()); + + // 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). + chanmon_cfgs[0].persister.set_update_ret(Ok(())); + unwrap_send_err!(nodes[0].node.send_payment(&route, second_payment_hash, &Some(second_payment_secret)), + true, APIError::ChannelUnavailable { ref err }, + assert!(err.contains("ChannelMonitor storage failure"))); + check_added_monitors!(nodes[0], 2); // After the failure we generate a close-channel monitor update + check_closed_broadcast!(nodes[0], true); + check_closed_event!(nodes[0], 1, ClosureReason::ProcessingError { err: "ChannelMonitor storage failure".to_string() }); + + // However, as the ChainMonitor is still waiting for the original persistence to complete, + // it won't yet release the MonitorEvents. + assert!(nodes[0].chain_monitor.release_pending_monitor_events().is_empty()); + + if block_timeout { + // After three blocks, pending MontiorEvents should be released either way. + let latest_header = BlockHeader { + version: 2, time: 0, bits: 0, nonce: 0, + prev_blockhash: nodes[0].best_block_info().0, + merkle_root: Default::default() }; + nodes[0].chain_monitor.chain_monitor.best_block_updated(&latest_header, nodes[0].best_block_info().1 + LATENCY_GRACE_PERIOD_BLOCKS); + } else { + for (funding_outpoint, update_ids) in chanmon_cfgs[0].persister.chain_sync_monitor_persistences.lock().unwrap().iter() { + for update_id in update_ids { + nodes[0].chain_monitor.chain_monitor.channel_monitor_updated(*funding_outpoint, *update_id).unwrap(); + } + } + } + + expect_payment_sent!(nodes[0], payment_preimage); + } + + #[test] + fn chainsync_pauses_events() { + do_chainsync_pauses_events(false); + do_chainsync_pauses_events(true); + } } From 9fe0cf19f6dba5895bcb21c7158c04fbc90c1fb6 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 19 Oct 2021 23:44:29 +0000 Subject: [PATCH 8/9] Add a test for MonitorEvent holding when they complete out-of-order --- lightning/src/chain/chainmonitor.rs | 83 +++++++++++++++++++++++++++-- lightning/src/util/test_utils.rs | 6 +++ 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index ccce772f8a1..fc7e50a1cb2 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -728,15 +728,17 @@ impl even #[cfg(test)] mod tests { use bitcoin::BlockHeader; - use ::{check_added_monitors, check_closed_broadcast, check_closed_event, expect_payment_sent}; - use ::{get_local_commitment_txn, get_route_and_payment_hash, unwrap_send_err}; + use ::{check_added_monitors, check_closed_broadcast, check_closed_event}; + use ::{expect_payment_sent, expect_payment_sent_without_paths, expect_payment_path_successful, get_event_msg}; + use ::{get_htlc_update_msgs, get_local_commitment_txn, get_revoke_commit_msgs, get_route_and_payment_hash, unwrap_send_err}; use chain::{ChannelMonitorUpdateErr, Confirm, Watch}; use chain::channelmonitor::LATENCY_GRACE_PERIOD_BLOCKS; use ln::channelmanager::PaymentSendFailure; use ln::features::InitFeatures; use ln::functional_test_utils::*; + use ln::msgs::ChannelMessageHandler; use util::errors::APIError; - use util::events::{ClosureReason, MessageSendEventsProvider}; + use util::events::{ClosureReason, MessageSendEvent, MessageSendEventsProvider}; use util::test_utils::{OnRegisterOutput, TxOutReference}; /// Tests that in-block dependent transactions are processed by `block_connected` when not @@ -782,6 +784,81 @@ mod tests { nodes[1].node.get_and_clear_pending_events(); } + #[test] + fn test_async_ooo_offchain_updates() { + // Test that if we have multiple offchain updates being persisted and they complete + // out-of-order, the ChainMonitor waits until all have completed before informing the + // ChannelManager. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + + // Route two payments to be claimed at the same time. + let payment_preimage_1 = route_payment(&nodes[0], &[&nodes[1]], 1_000_000).0; + let payment_preimage_2 = route_payment(&nodes[0], &[&nodes[1]], 1_000_000).0; + + chanmon_cfgs[1].persister.offchain_monitor_updates.lock().unwrap().clear(); + chanmon_cfgs[1].persister.set_update_ret(Err(ChannelMonitorUpdateErr::TemporaryFailure)); + + nodes[1].node.claim_funds(payment_preimage_1); + check_added_monitors!(nodes[1], 1); + nodes[1].node.claim_funds(payment_preimage_2); + check_added_monitors!(nodes[1], 1); + + chanmon_cfgs[1].persister.set_update_ret(Ok(())); + + let persistences = chanmon_cfgs[1].persister.offchain_monitor_updates.lock().unwrap().clone(); + assert_eq!(persistences.len(), 1); + let (funding_txo, updates) = persistences.iter().next().unwrap(); + assert_eq!(updates.len(), 2); + + // Note that updates is a HashMap so the ordering here is actually random. This shouldn't + // fail either way but if it fails intermittently it's depending on the ordering of updates. + let mut update_iter = updates.iter(); + nodes[1].chain_monitor.chain_monitor.channel_monitor_updated(*funding_txo, update_iter.next().unwrap().clone()).unwrap(); + assert!(nodes[1].chain_monitor.release_pending_monitor_events().is_empty()); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + nodes[1].chain_monitor.chain_monitor.channel_monitor_updated(*funding_txo, update_iter.next().unwrap().clone()).unwrap(); + + // Now manually walk the commitment signed dance - because we claimed two payments + // back-to-back it doesn't fit into the neat walk commitment_signed_dance does. + + let updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]); + expect_payment_sent_without_paths!(nodes[0], payment_preimage_1); + nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &updates.commitment_signed); + check_added_monitors!(nodes[0], 1); + let (as_first_raa, as_first_update) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + + nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_first_raa); + check_added_monitors!(nodes[1], 1); + let bs_second_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_first_update); + check_added_monitors!(nodes[1], 1); + let bs_first_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id()); + + nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_second_updates.update_fulfill_htlcs[0]); + expect_payment_sent_without_paths!(nodes[0], payment_preimage_2); + nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_second_updates.commitment_signed); + check_added_monitors!(nodes[0], 1); + nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_first_raa); + expect_payment_path_successful!(nodes[0]); + check_added_monitors!(nodes[0], 1); + let (as_second_raa, as_second_update) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + + nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_second_raa); + check_added_monitors!(nodes[1], 1); + nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_second_update); + check_added_monitors!(nodes[1], 1); + let bs_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id()); + + nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_raa); + expect_payment_path_successful!(nodes[0]); + check_added_monitors!(nodes[0], 1); + } + fn do_chainsync_pauses_events(block_timeout: bool) { // When a chainsync monitor update occurs, any MonitorUpdates should be held before being // passed upstream to a `ChannelManager` via `Watch::release_pending_monitor_events`. This diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index cc8c2069801..6442c9cfa27 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -172,6 +172,9 @@ pub struct TestPersister { /// When we get an update_persisted_channel call with no ChannelMonitorUpdate, we insert the /// MonitorUpdateId here. pub chain_sync_monitor_persistences: Mutex>>, + /// When we get an update_persisted_channel call *with* a ChannelMonitorUpdate, we insert the + /// MonitorUpdateId here. + pub offchain_monitor_updates: Mutex>>, } impl TestPersister { pub fn new() -> Self { @@ -179,6 +182,7 @@ impl TestPersister { update_ret: Mutex::new(Ok(())), next_update_ret: Mutex::new(None), chain_sync_monitor_persistences: Mutex::new(HashMap::new()), + offchain_monitor_updates: Mutex::new(HashMap::new()), } } @@ -206,6 +210,8 @@ impl chainmonitor::Persist for TestPersiste } if update.is_none() { self.chain_sync_monitor_persistences.lock().unwrap().entry(funding_txo).or_insert(HashSet::new()).insert(update_id); + } else { + self.offchain_monitor_updates.lock().unwrap().entry(funding_txo).or_insert(HashSet::new()).insert(update_id); } ret } From 1b88f1638e5d743dc827b5f0e1ccd35f00b69907 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 28 Nov 2021 17:13:38 +0000 Subject: [PATCH 9/9] Add trivial test of monitor update failure during block connection --- lightning/src/chain/chainmonitor.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index fc7e50a1cb2..9db666bb03d 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -935,4 +935,27 @@ mod tests { do_chainsync_pauses_events(false); do_chainsync_pauses_events(true); } + + #[test] + fn update_during_chainsync_fails_channel() { + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + + chanmon_cfgs[0].persister.chain_sync_monitor_persistences.lock().unwrap().clear(); + chanmon_cfgs[0].persister.set_update_ret(Err(ChannelMonitorUpdateErr::PermanentFailure)); + + connect_blocks(&nodes[0], 1); + // Before processing events, the ChannelManager will still think the Channel is open and + // there won't be any ChannelMonitorUpdates + assert_eq!(nodes[0].node.list_channels().len(), 1); + check_added_monitors!(nodes[0], 0); + // ... however once we get events once, the channel will close, creating a channel-closed + // ChannelMonitorUpdate. + check_closed_broadcast!(nodes[0], true); + check_closed_event!(nodes[0], 1, ClosureReason::ProcessingError { err: "Failed to persist ChannelMonitor update during chain sync".to_string() }); + check_added_monitors!(nodes[0], 1); + } }