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

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -1492,7 +1492,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 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,
@@ -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

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();

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();

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

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());
 

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

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.