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).

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 6513e7b8e68d6497f91f0f708c3c9d37d38a6325..2d76c09f1bb17782a16f3e420ab76e4866798a0c 100644 (file)
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -1501,7 +1501,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
        /// 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<B: Deref, F: Deref, L: Deref>(
+       ///
+       /// 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<B: Deref, F: Deref, L: Deref>(
                &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 7018896267b457fc068a36e19c61d0b363c318cc..cd9d5bf8b2c33878c5863a84d357f0b265914188 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -4027,12 +4027,14 @@ impl<SP: Deref> Channel<SP> 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<L: Deref>
+       pub fn claim_htlc_while_disconnected_dropping_mon_update_legacy<L: Deref>
                (&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 dc07165113e9692ad7a1ccbdd0d6451377df0cb9..f31c23b5a6711b759a541027faad7c76c618ad97 100644 (file)
--- 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 42a57b114cb7767fe2abfc83f224606f9645e8d5..ee73b30baefae7bfa916116b53ea95411829f6f0 100644 (file)
--- 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 dfc6c02b61a1769b29a810d02d8347d69513657e..4aad8f569fc2e7ac969fd7a60511c4f657944acc 100644 (file)
--- 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 40f7c785bc5221c1f016061c2466491b6712b88b..c32fca6bd75375d21fed1730d4e7f510ce044d3e 100644 (file)
--- 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 a05e40300fc1113f6768ade9dca7600dc18bd28d..c629c5bbce62a012ef3b6bf02ee1466551f7bdcf 100644 (file)
--- 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 (file)
index 0000000..fc53469
--- /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.