Attributable failures pre-factor

This commits prepares the persistence layer for the addition of
attribution data. Changes:

- Expand InboundHTLCRemovalReason serialization instead of using the
macro. When attribution data is added, it can't just be serialized
along with the existing fields because it would break backwards
compatibility. Instead the new field needs to go into the tlv block.

- Stop using OnionErrorPacket in the UpdateFailHTLC message. When
attribution data is added to OnionErrorPacket, it would not be
serialized for the wire properly because also here the new field needs
to go in the tlv extension of the message.

- Prepare HTLCFailReasonRepr serialization for that addition of
  attribution data.

Co-authored-by: Matt Corallo <[email protected]>

Author: joostjager

lightning/src/ln/channel.rs

@@ -36,7 +36,7 @@ use crate::ln::interactivetxs::{
 	TX_COMMON_FIELDS_WEIGHT,
 };
 use crate::ln::msgs;
-use crate::ln::msgs::{ClosingSigned, ClosingSignedFeeRange, DecodeError};
+use crate::ln::msgs::{ClosingSigned, ClosingSignedFeeRange, DecodeError, OnionErrorPacket};
 use crate::ln::script::{self, ShutdownScript};
 use crate::ln::channel_state::{ChannelShutdownState, CounterpartyForwardingInfo, InboundHTLCDetails, InboundHTLCStateDetails, OutboundHTLCDetails, OutboundHTLCStateDetails};
 use crate::ln::channelmanager::{self, OpenChannelMessage, PendingHTLCStatus, HTLCSource, SentHTLCId, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentClaimDetails, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
@@ -49,7 +49,7 @@ use crate::ln::chan_utils::{
 	ClosingTransaction, commit_tx_fee_sat,
 };
 use crate::ln::chan_utils;
-use crate::ln::onion_utils::HTLCFailReason;
+use crate::ln::onion_utils::{HTLCFailReason};
 use crate::chain::BestBlock;
 use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, LowerBoundedFeeEstimator, fee_for_weight};
 use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS};
@@ -4722,7 +4722,7 @@ trait FailHTLCContents {
 impl FailHTLCContents for msgs::OnionErrorPacket {
 	type Message = msgs::UpdateFailHTLC;
 	fn to_message(self, htlc_id: u64, channel_id: ChannelId) -> Self::Message {
-		msgs::UpdateFailHTLC { htlc_id, channel_id, reason: self }
+		msgs::UpdateFailHTLC { htlc_id, channel_id, reason: self.data }
 	}
 	fn to_inbound_htlc_state(self) -> InboundHTLCState {
 		InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay(self))
@@ -6041,7 +6041,7 @@ impl<SP: Deref> FundedChannel<SP> where
 										require_commitment = true;
 										match fail_msg {
 											HTLCFailureMsg::Relay(msg) => {
-												htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay(msg.reason.clone()));
+												htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay((&msg).into()));
 												update_fail_htlcs.push(msg)
 											},
 											HTLCFailureMsg::Malformed(msg) => {
@@ -6749,7 +6749,7 @@ impl<SP: Deref> FundedChannel<SP> where
 						update_fail_htlcs.push(msgs::UpdateFailHTLC {
 							channel_id: self.context.channel_id(),
 							htlc_id: htlc.htlc_id,
-							reason: err_packet.clone()
+							reason: err_packet.data.clone(),
 						});
 					},
 					&InboundHTLCRemovalReason::FailMalformed((ref sha256_of_onion, ref failure_code)) => {
@@ -9903,11 +9903,6 @@ fn get_initial_channel_type(config: &UserConfig, their_features: &InitFeatures)
 const SERIALIZATION_VERSION: u8 = 4;
 const MIN_SERIALIZATION_VERSION: u8 = 4;
 
-impl_writeable_tlv_based_enum_legacy!(InboundHTLCRemovalReason,;
-	(0, FailRelay),
-	(1, FailMalformed),
-	(2, Fulfill),
-);
 
 impl Writeable for ChannelUpdateStatus {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
@@ -10037,7 +10032,20 @@ impl<SP: Deref> Writeable for FundedChannel<SP> where SP::Target: SignerProvider
 				},
 				&InboundHTLCState::LocalRemoved(ref removal_reason) => {
 					4u8.write(writer)?;
-					removal_reason.write(writer)?;
+					match removal_reason {
+						InboundHTLCRemovalReason::FailRelay(msgs::OnionErrorPacket { data }) => {
+							0u8.write(writer)?;
+							data.write(writer)?;
+						},
+						InboundHTLCRemovalReason::FailMalformed((hash, code)) => {
+							1u8.write(writer)?;
+							(hash, code).write(writer)?;
+						},
+						InboundHTLCRemovalReason::Fulfill(preimage) => {
+							2u8.write(writer)?;
+							preimage.write(writer)?;
+						},
+					}
 				},
 			}
 		}
@@ -10116,7 +10124,7 @@ impl<SP: Deref> Writeable for FundedChannel<SP> where SP::Target: SignerProvider
 				&HTLCUpdateAwaitingACK::FailHTLC { ref htlc_id, ref err_packet } => {
 					2u8.write(writer)?;
 					htlc_id.write(writer)?;
-					err_packet.write(writer)?;
+					err_packet.data.write(writer)?;
 				}
 				&HTLCUpdateAwaitingACK::FailMalformedHTLC {
 					htlc_id, failure_code, sha256_of_onion
@@ -10125,10 +10133,9 @@ impl<SP: Deref> Writeable for FundedChannel<SP> where SP::Target: SignerProvider
 					// `::FailHTLC` variant and write the real malformed error as an optional TLV.
 					malformed_htlcs.push((htlc_id, failure_code, sha256_of_onion));
 
-					let dummy_err_packet = msgs::OnionErrorPacket { data: Vec::new() };
 					2u8.write(writer)?;
 					htlc_id.write(writer)?;
-					dummy_err_packet.write(writer)?;
+					Vec::<u8>::new().write(writer)?;
 				}
 			}
 		}
@@ -10374,7 +10381,17 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel
 						InboundHTLCState::AwaitingAnnouncedRemoteRevoke(resolution)
 					},
 					3 => InboundHTLCState::Committed,
-					4 => InboundHTLCState::LocalRemoved(Readable::read(reader)?),
+					4 => {
+						let reason = match <u8 as Readable>::read(reader)? {
+							0 => InboundHTLCRemovalReason::FailRelay(msgs::OnionErrorPacket {
+								data: Readable::read(reader)?,
+							}),
+							1 => InboundHTLCRemovalReason::FailMalformed(Readable::read(reader)?),
+							2 => InboundHTLCRemovalReason::Fulfill(Readable::read(reader)?),
+							_ => return Err(DecodeError::InvalidValue),
+						};
+						InboundHTLCState::LocalRemoved(reason)
+					},
 					_ => return Err(DecodeError::InvalidValue),
 				},
 			});
@@ -10430,7 +10447,9 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel
 				},
 				2 => HTLCUpdateAwaitingACK::FailHTLC {
 					htlc_id: Readable::read(reader)?,
-					err_packet: Readable::read(reader)?,
+					err_packet: OnionErrorPacket {
+						data: Readable::read(reader)?,
+					},
 				},
 				_ => return Err(DecodeError::InvalidValue),
 			});

lightning/src/ln/channelmanager.rs

@@ -4427,11 +4427,12 @@ where
 		} else {
 			(err_code, &res.0[..])
 		};
+		let failure = HTLCFailReason::reason(err_code, err_data.to_vec())
+		.get_encrypted_failure_packet(shared_secret, &None);
 		HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
 			channel_id: msg.channel_id,
 			htlc_id: msg.htlc_id,
-			reason: HTLCFailReason::reason(err_code, err_data.to_vec())
-				.get_encrypted_failure_packet(shared_secret, &None),
+			reason: failure.data.clone(),
 		})
 	}
 
@@ -4455,11 +4456,12 @@ where
 							}
 						))
 					}
+					let failure = HTLCFailReason::reason($err_code, $data.to_vec())
+						.get_encrypted_failure_packet(&shared_secret, &None);
 					return PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
 						channel_id: msg.channel_id,
 						htlc_id: msg.htlc_id,
-						reason: HTLCFailReason::reason($err_code, $data.to_vec())
-							.get_encrypted_failure_packet(&shared_secret, &None),
+						reason: failure.data,
 					}));
 				}
 			}
@@ -5798,7 +5800,7 @@ where
 				let failure = match htlc_fail {
 					HTLCFailureMsg::Relay(fail_htlc) => HTLCForwardInfo::FailHTLC {
 						htlc_id: fail_htlc.htlc_id,
-						err_packet: fail_htlc.reason,
+						err_packet: (&fail_htlc).into(),
 					},
 					HTLCFailureMsg::Malformed(fail_malformed_htlc) => HTLCForwardInfo::FailMalformedHTLC {
 						htlc_id: fail_malformed_htlc.htlc_id,
@@ -13108,19 +13110,18 @@ impl Writeable for HTLCForwardInfo {
 				FAIL_HTLC_VARIANT_ID.write(w)?;
 				write_tlv_fields!(w, {
 					(0, htlc_id, required),
-					(2, err_packet, required),
+					(2, err_packet.data, required),
 				});
 			},
 			Self::FailMalformedHTLC { htlc_id, failure_code, sha256_of_onion } => {
 				// Since this variant was added in 0.0.119, write this as `::FailHTLC` with an empty error
 				// packet so older versions have something to fail back with, but serialize the real data as
 				// optional TLVs for the benefit of newer versions.
 				FAIL_HTLC_VARIANT_ID.write(w)?;
-				let dummy_err_packet = msgs::OnionErrorPacket { data: Vec::new() };
 				write_tlv_fields!(w, {
 					(0, htlc_id, required),
 					(1, failure_code, required),
-					(2, dummy_err_packet, required),
+					(2, Vec::<u8>::new(), required),
 					(3, sha256_of_onion, required),
 				});
 			},
@@ -13150,7 +13151,9 @@ impl Readable for HTLCForwardInfo {
 				} else {
 					Self::FailHTLC {
 						htlc_id: _init_tlv_based_struct_field!(htlc_id, required),
-						err_packet: _init_tlv_based_struct_field!(err_packet, required),
+						err_packet: crate::ln::msgs::OnionErrorPacket {
+							data: _init_tlv_based_struct_field!(err_packet, required),
+						},
 					}
 				}
 			},
@@ -14851,7 +14854,7 @@ mod tests {
 	use crate::events::{Event, HTLCDestination, ClosureReason};
 	use crate::ln::types::ChannelId;
 	use crate::types::payment::{PaymentPreimage, PaymentHash, PaymentSecret};
-	use crate::ln::channelmanager::{create_recv_pending_htlc_info, inbound_payment, ChannelConfigOverrides, HTLCForwardInfo, InterceptId, PaymentId, RecipientOnionFields};
+	use crate::ln::channelmanager::{RAACommitmentOrder, create_recv_pending_htlc_info, inbound_payment, ChannelConfigOverrides, HTLCForwardInfo, InterceptId, PaymentId, RecipientOnionFields};
 	use crate::ln::functional_test_utils::*;
 	use crate::ln::msgs::{self, BaseMessageHandler, ChannelMessageHandler, AcceptChannel, ErrorAction, MessageSendEvent};
 	use crate::ln::outbound_payment::Retry;
@@ -15070,8 +15073,8 @@ mod tests {
 		let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
 		create_announced_chan_between_nodes(&nodes, 0, 1);
-	
-		// Since we do not send peer storage, we manually simulate receiving a dummy 
+
+		// Since we do not send peer storage, we manually simulate receiving a dummy
 		// `PeerStorage` from the channel partner.
 		nodes[0].node.handle_peer_storage(nodes[1].node.get_our_node_id(), msgs::PeerStorage{data: vec![0; 100]});
 
@@ -16280,7 +16283,7 @@ mod tests {
 		let mut nodes = create_network(1, &node_cfg, &chanmgrs);
 
 		let dummy_failed_htlc = |htlc_id| {
-			HTLCForwardInfo::FailHTLC { htlc_id, err_packet: msgs::OnionErrorPacket { data: vec![42] }, }
+			HTLCForwardInfo::FailHTLC { htlc_id, err_packet: msgs::OnionErrorPacket { data: vec![42] } }
 		};
 		let dummy_malformed_htlc = |htlc_id| {
 			HTLCForwardInfo::FailMalformedHTLC { htlc_id, failure_code: 0x4000, sha256_of_onion: [0; 32] }

lightning/src/ln/functional_tests.rs

@@ -7057,7 +7057,7 @@ pub fn test_update_fulfill_htlc_bolt2_update_fail_htlc_before_commitment() {
 	let update_msg = msgs::UpdateFailHTLC{
 		channel_id: chan.2,
 		htlc_id: 0,
-		reason: msgs::OnionErrorPacket { data: Vec::new()},
+		reason: Vec::new(),
 	};
 
 	nodes[0].node.handle_update_fail_htlc(nodes[1].node.get_our_node_id(), &update_msg);

lightning/src/ln/msgs.rs

@@ -728,7 +728,7 @@ pub struct UpdateFulfillHTLC {
 /// A [`peer_storage`] message that can be sent to or received from a peer.
 ///
 /// This message is used to distribute backup data to peers.
-/// If data is lost or corrupted, users can retrieve it through [`PeerStorageRetrieval`]  
+/// If data is lost or corrupted, users can retrieve it through [`PeerStorageRetrieval`]
 /// to recover critical information, such as channel states, for fund recovery.
 ///
 /// [`peer_storage`] is used to send our own encrypted backup data to a peer.
@@ -743,7 +743,7 @@ pub struct PeerStorage {
 /// A [`peer_storage_retrieval`] message that can be sent to or received from a peer.
 ///
 /// This message is sent to peers for whom we store backup data.
-/// If we receive this message, it indicates that the peer had stored our backup data.  
+/// If we receive this message, it indicates that the peer had stored our backup data.
 /// This data can be used for fund recovery in case of data loss.
 ///
 /// [`peer_storage_retrieval`] is used to send the most recent backup of the peer.
@@ -764,7 +764,7 @@ pub struct UpdateFailHTLC {
 	pub channel_id: ChannelId,
 	/// The HTLC ID
 	pub htlc_id: u64,
-	pub(crate) reason: OnionErrorPacket,
+	pub(crate) reason: Vec<u8>,
 }
 
 /// An [`update_fail_malformed_htlc`] message to be sent to or received from a peer.
@@ -2300,6 +2300,14 @@ pub(crate) struct OnionErrorPacket {
 	pub(crate) data: Vec<u8>,
 }
 
+impl From<&UpdateFailHTLC> for OnionErrorPacket {
+	fn from(msg: &UpdateFailHTLC) -> Self {
+		OnionErrorPacket {
+			data: msg.reason.clone(),
+		}
+	}
+}
+
 impl fmt::Display for DecodeError {
 	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
 		match *self {
@@ -2946,13 +2954,6 @@ impl_writeable_msg!(PeerStorageRetrieval, {
 	data
 }, {});
 
-// Note that this is written as a part of ChannelManager objects, and thus cannot change its
-// serialization format in a way which assumes we know the total serialized length/message end
-// position.
-impl_writeable!(OnionErrorPacket, {
-	data
-});
-
 // Note that this is written as a part of ChannelManager objects, and thus cannot change its
 // serialization format in a way which assumes we know the total serialized length/message end
 // position.
@@ -4698,13 +4699,10 @@ mod tests {
 
 	#[test]
 	fn encoding_update_fail_htlc() {
-		let reason = OnionErrorPacket {
-			data: [1; 32].to_vec(),
-		};
 		let update_fail_htlc = msgs::UpdateFailHTLC {
 			channel_id: ChannelId::from_bytes([2; 32]),
 			htlc_id: 2316138423780173,
-			reason
+			reason: [1; 32].to_vec()
 		};
 		let encoded_value = update_fail_htlc.encode();
 		let target_value = <Vec<u8>>::from_hex("020202020202020202020202020202020202020202020202020202020202020200083a840000034d00200101010101010101010101010101010101010101010101010101010101010101").unwrap();

lightning/src/ln/onion_payment.rs

@@ -289,7 +289,7 @@ where
 	).map_err(|e| {
 		let (err_code, err_data) = match e {
 			HTLCFailureMsg::Malformed(m) => (m.failure_code, Vec::new()),
-			HTLCFailureMsg::Relay(r) => (0x4000 | 22, r.reason.data),
+			HTLCFailureMsg::Relay(r) => (0x4000 | 22, r.reason),
 		};
 		let msg = "Failed to decode update add htlc onion";
 		InboundHTLCErr { msg, err_code, err_data }
@@ -400,11 +400,12 @@ where
 		}
 
 		log_info!(logger, "Failed to accept/forward incoming HTLC: {}", message);
+		let failure = HTLCFailReason::reason(err_code, data.to_vec())
+			.get_encrypted_failure_packet(&shared_secret, &None);
 		return Err(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
 			channel_id: msg.channel_id,
 			htlc_id: msg.htlc_id,
-			reason: HTLCFailReason::reason(err_code, data.to_vec())
-				.get_encrypted_failure_packet(&shared_secret, &None),
+			reason: failure.data,
 		}));
 	};