From ba264323f83495ae3cc862cb06c27311fe20f1bd Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 15 Sep 2024 17:24:19 +0000 Subject: [PATCH] Doc the on-upgrade `ChannelMonitor` startup persistence semantics Because the new startup `ChannelMonitor` persistence semantics rely on new information stored in `ChannelMonitor` only for claims made in the upgraded code, users upgrading from previous version of LDK must apply the old `ChannelMonitor` persistence semantics at least once (as the old code will be used to handle partial claims). --- lightning/src/chain/channelmonitor.rs | 14 +++++++++++-- lightning/src/ln/channel.rs | 4 +++- lightning/src/ln/channelmanager.rs | 21 +++++++++++++++++-- lightning/src/ln/functional_tests.rs | 5 ++++- lightning/src/ln/monitor_tests.rs | 12 ++++++----- lightning/src/ln/reload_tests.rs | 6 ++++-- lightning/src/ln/reorg_tests.rs | 4 ++-- .../matt-persist-preimage-on-upgrade.txt | 8 +++++++ 8 files changed, 59 insertions(+), 15 deletions(-) create mode 100644 pending_changelog/matt-persist-preimage-on-upgrade.txt diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 6513e7b8e..2d76c09f1 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1501,7 +1501,15 @@ impl ChannelMonitor { /// This is used to provide payment preimage(s) out-of-band during startup without updating the /// off-chain state with a new commitment transaction. - pub(crate) fn provide_payment_preimage( + /// + /// It is used only for legacy (created prior to LDK 0.1) pending payments on upgrade, and the + /// flow that uses it assumes that this [`ChannelMonitor`] is persisted prior to the + /// [`ChannelManager`] being persisted (as the state necessary to call this method again is + /// removed from the [`ChannelManager`] and thus a persistence inversion would imply we do not + /// get the preimage back into this [`ChannelMonitor`] on startup). + /// + /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager + pub(crate) fn provide_payment_preimage_unsafe_legacy( &self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, @@ -5279,7 +5287,9 @@ mod tests { preimages_slice_to_htlc_outputs!(preimages[15..20]), 281474976710654, dummy_key, &logger); for &(ref preimage, ref hash) in preimages.iter() { let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&fee_estimator); - monitor.provide_payment_preimage(hash, preimage, &broadcaster, &bounded_fee_estimator, &logger); + monitor.provide_payment_preimage_unsafe_legacy( + hash, preimage, &broadcaster, &bounded_fee_estimator, &logger + ); } // Now provide a secret, pruning preimages 10-15 diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 701889626..cd9d5bf8b 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4027,12 +4027,14 @@ impl Channel where /// Claims an HTLC while we're disconnected from a peer, dropping the [`ChannelMonitorUpdate`] /// entirely. /// + /// This is only used for payments received prior to LDK 0.1. + /// /// The [`ChannelMonitor`] for this channel MUST be updated out-of-band with the preimage /// provided (i.e. without calling [`crate::chain::Watch::update_channel`]). /// /// The HTLC claim will end up in the holding cell (because the caller must ensure the peer is /// disconnected). - pub fn claim_htlc_while_disconnected_dropping_mon_update + pub fn claim_htlc_while_disconnected_dropping_mon_update_legacy (&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage, logger: &L) where L::Target: Logger { // Assert that we'll add the HTLC claim to the holding cell in `get_update_fulfill_htlc` diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index dc0716511..f31c23b5a 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -13284,11 +13284,28 @@ where let peer_state = &mut *peer_state_lock; if let Some(ChannelPhase::Funded(channel)) = peer_state.channel_by_id.get_mut(&previous_channel_id) { let logger = WithChannelContext::from(&channel_manager.logger, &channel.context, Some(payment_hash)); - channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &&logger); + channel.claim_htlc_while_disconnected_dropping_mon_update_legacy( + claimable_htlc.prev_hop.htlc_id, payment_preimage, &&logger + ); } } if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) { - previous_hop_monitor.provide_payment_preimage(&payment_hash, &payment_preimage, &channel_manager.tx_broadcaster, &channel_manager.fee_estimator, &channel_manager.logger); + // Note that this is unsafe as we no longer require the + // `ChannelMonitor`s to be re-persisted prior to this + // `ChannelManager` being persisted after we get started running. + // If this `ChannelManager` gets persisted first then we crash, we + // won't have the `claimable_payments` entry we need to re-enter + // this code block, causing us to not re-apply the preimage to this + // `ChannelMonitor`. + // + // We should never be here with modern payment claims, however, as + // they should always include the HTLC list. Instead, this is only + // for nodes during upgrade, and we explicitly require the old + // persistence semantics on upgrade in the release notes. + previous_hop_monitor.provide_payment_preimage_unsafe_legacy( + &payment_hash, &payment_preimage, &channel_manager.tx_broadcaster, + &channel_manager.fee_estimator, &channel_manager.logger + ); } } let mut pending_events = channel_manager.pending_events.lock().unwrap(); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 42a57b114..ee73b30ba 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -3702,7 +3702,10 @@ fn test_force_close_fail_back() { // Now check that if we add the preimage to ChannelMonitor it broadcasts our HTLC-Success.. { get_monitor!(nodes[2], payment_event.commitment_msg.channel_id) - .provide_payment_preimage(&our_payment_hash, &our_payment_preimage, &node_cfgs[2].tx_broadcaster, &LowerBoundedFeeEstimator::new(node_cfgs[2].fee_estimator), &node_cfgs[2].logger); + .provide_payment_preimage_unsafe_legacy( + &our_payment_hash, &our_payment_preimage, &node_cfgs[2].tx_broadcaster, + &LowerBoundedFeeEstimator::new(node_cfgs[2].fee_estimator), &node_cfgs[2].logger + ); } mine_transaction(&nodes[2], &commitment_tx); let mut node_txn = nodes[2].tx_broadcaster.txn_broadcast(); diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index dfc6c02b6..4aad8f569 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -1883,8 +1883,10 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) { // Cheat by giving A's ChannelMonitor the preimage to the to-be-claimed HTLC so that we have an // HTLC-claim transaction on the to-be-revoked state. - get_monitor!(nodes[0], chan_id).provide_payment_preimage(&claimed_payment_hash, &claimed_payment_preimage, - &node_cfgs[0].tx_broadcaster, &LowerBoundedFeeEstimator::new(node_cfgs[0].fee_estimator), &nodes[0].logger); + get_monitor!(nodes[0], chan_id).provide_payment_preimage_unsafe_legacy( + &claimed_payment_hash, &claimed_payment_preimage, &node_cfgs[0].tx_broadcaster, + &LowerBoundedFeeEstimator::new(node_cfgs[0].fee_estimator), &nodes[0].logger + ); // Now get the latest commitment transaction from A and then update the fee to revoke it let as_revoked_txn = get_local_commitment_txn!(nodes[0], chan_id); @@ -2507,11 +2509,11 @@ fn do_test_yield_anchors_events(have_htlcs: bool) { } if have_htlcs { - get_monitor!(nodes[0], chan_id).provide_payment_preimage( + get_monitor!(nodes[0], chan_id).provide_payment_preimage_unsafe_legacy( &payment_hash_2.unwrap(), &payment_preimage_2.unwrap(), &node_cfgs[0].tx_broadcaster, &LowerBoundedFeeEstimator::new(node_cfgs[0].fee_estimator), &nodes[0].logger ); - get_monitor!(nodes[1], chan_id).provide_payment_preimage( + get_monitor!(nodes[1], chan_id).provide_payment_preimage_unsafe_legacy( &payment_hash_1.unwrap(), &payment_preimage_1.unwrap(), &node_cfgs[1].tx_broadcaster, &LowerBoundedFeeEstimator::new(node_cfgs[1].fee_estimator), &nodes[1].logger ); @@ -2706,7 +2708,7 @@ fn test_anchors_aggregated_revoked_htlc_tx() { for chan_id in [chan_a.2, chan_b.2].iter() { let monitor = get_monitor!(nodes[1], chan_id); for payment in [payment_a, payment_b, payment_c, payment_d].iter() { - monitor.provide_payment_preimage( + monitor.provide_payment_preimage_unsafe_legacy( &payment.1, &payment.0, &node_cfgs[1].tx_broadcaster, &LowerBoundedFeeEstimator::new(node_cfgs[1].fee_estimator), &nodes[1].logger ); diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 40f7c785b..c32fca6bd 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -1041,8 +1041,10 @@ fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_ht check_added_monitors!(nodes[2], 1); if claim_htlc { - get_monitor!(nodes[2], chan_id_2).provide_payment_preimage(&payment_hash, &payment_preimage, - &nodes[2].tx_broadcaster, &LowerBoundedFeeEstimator(nodes[2].fee_estimator), &nodes[2].logger); + get_monitor!(nodes[2], chan_id_2).provide_payment_preimage_unsafe_legacy( + &payment_hash, &payment_preimage, &nodes[2].tx_broadcaster, + &LowerBoundedFeeEstimator(nodes[2].fee_estimator), &nodes[2].logger + ); } assert!(nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty()); diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index a05e40300..c629c5bbc 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -674,7 +674,7 @@ fn test_htlc_preimage_claim_holder_commitment_after_counterparty_commitment_reor // Provide the preimage now, such that we only claim from the holder commitment (since it's // currently confirmed) and not the counterparty's. - get_monitor!(nodes[1], chan_id).provide_payment_preimage( + get_monitor!(nodes[1], chan_id).provide_payment_preimage_unsafe_legacy( &payment_hash, &payment_preimage, &nodes[1].tx_broadcaster, &LowerBoundedFeeEstimator(nodes[1].fee_estimator), &nodes[1].logger ); @@ -749,7 +749,7 @@ fn test_htlc_preimage_claim_prev_counterparty_commitment_after_current_counterpa // Provide the preimage now, such that we only claim from the previous commitment (since it's // currently confirmed) and not the latest. - get_monitor!(nodes[1], chan_id).provide_payment_preimage( + get_monitor!(nodes[1], chan_id).provide_payment_preimage_unsafe_legacy( &payment_hash, &payment_preimage, &nodes[1].tx_broadcaster, &LowerBoundedFeeEstimator(nodes[1].fee_estimator), &nodes[1].logger ); diff --git a/pending_changelog/matt-persist-preimage-on-upgrade.txt b/pending_changelog/matt-persist-preimage-on-upgrade.txt new file mode 100644 index 000000000..fc53469c6 --- /dev/null +++ b/pending_changelog/matt-persist-preimage-on-upgrade.txt @@ -0,0 +1,8 @@ +# Backwards Compatibility + * The `ChannelManager` deserialization semantics no longer require that + `ChannelMonitor`s be re-persisted after `(BlockHash, ChannelManager)::read` + is called prior to normal node operation. This applies to upgraded nodes + only *after* a startup with the old semantics completes at least once. IOW, + you must deserialize the `ChannelManager` with upgraded LDK, persist the + `ChannelMonitor`s then continue to normal startup once, and thereafter you + may skip the `ChannelMonitor` persistence step. -- 2.39.5