diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 914257dc0ed..c31faa0749f 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1492,7 +1492,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 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, @@ -5258,7 +5266,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/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 1c81ec0331a..ed5892708a8 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -13271,7 +13271,22 @@ where } } 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 31346c6b78b..5dff0edc7ed 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -3692,7 +3692,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 947ec4a1304..5e834f89513 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); @@ -2518,11 +2520,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 ); @@ -2717,7 +2719,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 9a26feb08c9..abd8a2fba3a 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -1036,8 +1036,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 5d4f1540fb9..b794d9533a0 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 00000000000..fc53469c6f6 --- /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.