Merge pull request #4653 from wpaulino/pending-splice-persist

Persist negotiated splice candidates on reload

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -2989,15 +2989,6 @@ struct PendingFunding {
 	contributions: Vec<FundingContribution>,
 }
 
-impl_ser_tlv_based!(PendingFunding, {
-	(1, funding_negotiation, upgradable_option),
-	(3, negotiated_candidates, required_vec),
-	(5, sent_funding_txid, option),
-	(7, received_funding_txid, option),
-	(8, last_funding_feerate_sat_per_1000_weight, option),
-	(10, contributions, optional_vec),
-});
-
 #[derive(Debug)]
 enum FundingNegotiation {
 	AwaitingAck {
@@ -3037,6 +3028,57 @@ impl_writeable_tlv_based_enum_upgradable!(FundingNegotiation,
 	unread_variants: AwaitingAck, ConstructingTransaction
 );
 
+struct PendingFundingWriteable<'a> {
+	pending_funding: &'a PendingFunding,
+	reset_funding_negotiation: bool,
+}
+
+impl Writeable for PendingFundingWriteable<'_> {
+	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
+		let funding_negotiation = if self.reset_funding_negotiation {
+			None
+		} else {
+			self.pending_funding.funding_negotiation.as_ref()
+		};
+		debug_assert!(
+			funding_negotiation.is_none()
+				|| matches!(
+					funding_negotiation,
+					Some(FundingNegotiation::AwaitingSignatures { .. })
+				)
+		);
+		let contributions_len = if self.reset_funding_negotiation
+			&& self.pending_funding.funding_negotiation.is_some()
+		{
+			self.pending_funding.contributions.len().saturating_sub(1)
+		} else {
+			self.pending_funding.contributions.len()
+		};
+		write_tlv_fields!(writer, {
+			(1, funding_negotiation, upgradable_option),
+			(3, self.pending_funding.negotiated_candidates, required_vec),
+			(5, self.pending_funding.sent_funding_txid, option),
+			(7, self.pending_funding.received_funding_txid, option),
+			(8, self.pending_funding.last_funding_feerate_sat_per_1000_weight, option),
+			(10, self.pending_funding.contributions[..contributions_len], optional_vec),
+		});
+		Ok(())
+	}
+}
+
+impl Readable for PendingFunding {
+	fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+		Ok(_decode_and_build!(reader, Self, {
+			(1, funding_negotiation, upgradable_option),
+			(3, negotiated_candidates, required_vec),
+			(5, sent_funding_txid, option),
+			(7, received_funding_txid, option),
+			(8, last_funding_feerate_sat_per_1000_weight, option),
+			(10, contributions, optional_vec),
+		}))
+	}
+}
+
 impl FundingNegotiation {
 	fn as_funding(&self) -> Option<&FundingScope> {
 		match self {
@@ -16305,10 +16347,18 @@ impl<SP: SignerProvider> Writeable for FundedChannel<SP> {
 		let holder_commitment_point_next = self.holder_commitment_point.next_point();
 		let holder_commitment_point_pending_next = self.holder_commitment_point.pending_next_point;
 
-		// We don't have to worry about resetting the pending `FundingNegotiation` because we
-		// can only read `FundingNegotiation::AwaitingSignatures` variants anyway.
-		let pending_splice =
-			self.pending_splice.as_ref().filter(|_| !self.should_reset_pending_splice_state(true));
+		// Avoid writing any negotiations that are not at the signing stage yet, as they cannot be
+		// resumed on reestablishment, but keep any already-negotiated candidates.
+		let reset_funding_negotiation = self.should_reset_pending_splice_state(true);
+		let should_persist_pending_splice =
+			!reset_funding_negotiation || !self.pending_funding().is_empty();
+		let pending_splice = should_persist_pending_splice
+			.then(|| ())
+			.and_then(|_| self.pending_splice.as_ref())
+			.map(|pending_funding| PendingFundingWriteable {
+				pending_funding,
+				reset_funding_negotiation,
+			});
 
 		let monitor_pending_tx_signatures =
 			self.context.monitor_pending_tx_signatures.then_some(());

lightning/src/ln/splicing_tests.rs

@@ -1152,6 +1152,55 @@ fn do_test_splice_state_reset_on_disconnect(reload: bool) {
 	lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1);
 }
 
+#[test]
+fn test_reload_resets_splice_negotiation_without_dropping_candidates() {
+	// A reload should abort an in-flight RBF negotiation, but it must not drop the previously
+	// negotiated splice candidate that the monitor is still tracking.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let (persister_0, chain_monitor_0);
+	let node_0;
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let node_id_1 = nodes[1].node.get_our_node_id();
+	let initial_channel_value_sat = 100_000;
+	let (_, _, channel_id, _) =
+		create_announced_chan_between_nodes_with_value(&nodes, 0, 1, initial_channel_value_sat, 0);
+
+	let added_value = Amount::from_sat(50_000);
+	provide_utxo_reserves(&nodes, 2, added_value * 2);
+
+	let funding_contribution = do_initiate_splice_in(&nodes[0], &nodes[1], channel_id, added_value);
+	let (_splice_tx, _) =
+		splice_channel(&nodes[0], &nodes[1], channel_id, funding_contribution.clone());
+
+	let rbf_feerate = FeeRate::from_sat_per_kwu(FEERATE_FLOOR_SATS_PER_KW as u64 + 25);
+	let funding_template = nodes[0].node.splice_channel(&channel_id, &node_id_1).unwrap();
+	assert_eq!(funding_template.min_rbf_feerate(), Some(rbf_feerate));
+	assert!(funding_template.prior_contribution().is_some());
+
+	let rbf_contribution =
+		funding_template.with_prior_contribution(rbf_feerate, FeeRate::MAX).build().unwrap();
+	nodes[0].node.funding_contributed(&channel_id, &node_id_1, rbf_contribution, None).unwrap();
+	complete_rbf_handshake(&nodes[0], &nodes[1]);
+
+	let encoded_monitor_0 = get_monitor!(nodes[0], channel_id).encode();
+	reload_node!(
+		nodes[0],
+		nodes[0].node.encode(),
+		&[&encoded_monitor_0],
+		persister_0,
+		chain_monitor_0,
+		node_0
+	);
+	let _ = get_event!(&nodes[0], Event::SpliceNegotiationFailed);
+
+	let funding_template = nodes[0].node.splice_channel(&channel_id, &node_id_1).unwrap();
+	assert_eq!(funding_template.min_rbf_feerate(), Some(rbf_feerate));
+	assert_eq!(funding_template.prior_contribution().unwrap(), &funding_contribution);
+}
+
 #[test]
 fn test_config_reject_inbound_splices() {
 	// Tests that nodes with `reject_inbound_splices` properly reject inbound splices but still