← Watch feed
ln/refactor: remove claimable htlc from fail_htlc macro
Public commit record
73/100 · Adequate
What the developer wrote
Authored by Carla Kirk-Cohen
ln/refactor: remove claimable htlc from fail_htlc macro
In the commit that follows we're going to need to take ownership
of our htlc before this macro is used, so we pull out the information
we need in advance.
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context
What changed, and why it matters
This commit is a pure internal code cleanup in the Lightning payment handling logic. It moves some data preparation earlier in the code and simplifies a helper macro so that a later commit can take ownership of an HTLC (a payment packet) before the macro is called. There is no functional change to how payments are accepted or rejected, and no security bug is fixed or introduced here.
Recommended action
No security action needed. Treat as a normal refactoring commit. Review the following commit in the series for actual functional or security relevance.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The change refactors ChannelManager::claim_funds_from_hop in lightning/src/ln/channelmanager.rs. It hoists the construction of value, htlc_source, and claimable_htlc before the fail_htlc! macro definition, then removes the $htlc: expr parameter from that macro. The macro now captures value and htlc_source from the surrounding scope instead of reading them from the passed-in HTLC. All call sites are updated from fail_htlc!(claimable_htlc, payment_hash) to fail_htlc!(payment_hash). The commit message explicitly states this is preparation for a following commit that needs to take ownership of the HTLC before the macro is used. No behavior changes are visible in the diff.
Changed components
lightning/src/ln/channelmanager.rsInspect captured patch +35 / −38
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cd1eb39..562316f 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -8366,14 +8366,28 @@ impl<
panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive");
},
};
+ // We differentiate the received value from the sender intended value
+ // if possible so that we don't prematurely mark MPP payments complete
+ // if routing nodes overpay
+ let value = incoming_amt_msat.unwrap_or(outgoing_amt_msat);
+ let htlc_source = HTLCSource::PreviousHopData(HTLCPreviousHopData {
+ prev_outbound_scid_alias: prev_hop.prev_outbound_scid_alias,
+ user_channel_id: prev_hop.user_channel_id,
+ counterparty_node_id: prev_hop.counterparty_node_id,
+ channel_id: prev_channel_id,
+ outpoint: prev_funding_outpoint,
+ htlc_id: prev_hop.htlc_id,
+ incoming_packet_shared_secret: prev_hop.incoming_packet_shared_secret,
+ phantom_shared_secret,
+ trampoline_shared_secret,
+ blinded_failure,
+ cltv_expiry: Some(cltv_expiry),
+ });
let claimable_htlc = ClaimableHTLC {
mpp_part: MppPart {
prev_hop,
cltv_expiry,
- // We differentiate the received value from the sender intended value
- // if possible so that we don't prematurely mark MPP payments complete
- // if routing nodes overpay
- value: incoming_amt_msat.unwrap_or(outgoing_amt_msat),
+ value,
sender_intended_value: outgoing_amt_msat,
timer_ticks: 0,
total_value_received: None,
@@ -8385,31 +8399,14 @@ impl<
let mut committed_to_claimable = false;
macro_rules! fail_htlc {
- ($htlc: expr, $payment_hash: expr) => {
+ ($payment_hash: expr) => {
debug_assert!(!committed_to_claimable);
let err_data = invalid_payment_err_data(
- $htlc.mpp_part.value,
+ value,
self.best_block.read().unwrap().height,
);
- let counterparty_node_id = $htlc.mpp_part.prev_hop.counterparty_node_id;
- let incoming_packet_shared_secret =
- $htlc.mpp_part.prev_hop.incoming_packet_shared_secret;
- let prev_outbound_scid_alias =
- $htlc.mpp_part.prev_hop.prev_outbound_scid_alias;
failed_forwards.push((
- HTLCSource::PreviousHopData(HTLCPreviousHopData {
- prev_outbound_scid_alias,
- user_channel_id: $htlc.mpp_part.prev_hop.user_channel_id,
- counterparty_node_id,
- channel_id: prev_channel_id,
- outpoint: prev_funding_outpoint,
- htlc_id: $htlc.mpp_part.prev_hop.htlc_id,
- incoming_packet_shared_secret,
- phantom_shared_secret,
- trampoline_shared_secret,
- blinded_failure,
- cltv_expiry: Some(cltv_expiry),
- }),
+ htlc_source,
payment_hash,
HTLCFailReason::reason(
LocalHTLCFailureReason::IncorrectPaymentDetails,
@@ -8436,7 +8433,7 @@ impl<
let is_keysend = $purpose.is_keysend();
let mut claimable_payments = self.claimable_payments.lock().unwrap();
if claimable_payments.pending_claiming_payments.contains_key(&payment_hash) {
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
}
let ref mut claimable_payment = claimable_payments.claimable_payments
.entry(payment_hash)
@@ -8452,12 +8449,12 @@ impl<
if $purpose != claimable_payment.purpose {
let log_keysend = |keysend| if keysend { "keysend" } else { "non-keysend" };
log_trace!(self.logger, "Failing new {} HTLC with payment_hash {} as we already had an existing {} HTLC with the same payment hash", log_keysend(is_keysend), &payment_hash, log_keysend(!is_keysend));
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
}
let onions_compatible =
claimable_payment.onion_fields.check_merge(&mut onion_fields);
if onions_compatible.is_err() {
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
}
let mut total_intended_recvd_value =
claimable_htlc.mpp_part.sender_intended_value;
@@ -8472,11 +8469,11 @@ impl<
// The condition determining whether an MPP is complete must
// match exactly the condition used in `timer_tick_occurred`
if total_intended_recvd_value >= msgs::MAX_VALUE_MSAT {
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
} else if total_intended_recvd_value - claimable_htlc.mpp_part.sender_intended_value >= total_mpp_value {
log_trace!(self.logger, "Failing HTLC with payment_hash {} as payment is already claimable",
&payment_hash);
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
} else if total_intended_recvd_value >= total_mpp_value {
#[allow(unused_assignments)] {
committed_to_claimable = true;
@@ -8537,7 +8534,7 @@ impl<
Ok(result) => result,
Err(()) => {
log_trace!(self.logger, "Failing new HTLC with payment_hash {} as payment verification failed", &payment_hash);
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
},
};
if let Some(min_final_cltv_expiry_delta) = min_final_cltv_expiry_delta {
@@ -8547,12 +8544,12 @@ impl<
if (cltv_expiry as u64) < expected_min_expiry_height {
log_trace!(self.logger, "Failing new HTLC with payment_hash {} as its CLTV expiry was too soon (had {}, earliest expected {})",
&payment_hash, cltv_expiry, expected_min_expiry_height);
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
}
}
payment_preimage
} else {
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
}
} else {
None
@@ -8568,7 +8565,7 @@ impl<
let purpose = match from_parts_res {
Ok(purpose) => purpose,
Err(()) => {
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
},
};
check_total_value!(purpose);
@@ -8585,7 +8582,7 @@ impl<
false,
"We checked that payment_data is Some above"
);
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
},
};
@@ -8604,13 +8601,13 @@ impl<
verified_invreq.amount_msats()
{
if payment_data.total_msat < invreq_amt_msat {
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
}
}
verified_invreq
},
None => {
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
},
};
let payment_purpose_context =
@@ -8626,12 +8623,12 @@ impl<
match from_parts_res {
Ok(purpose) => purpose,
Err(()) => {
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
},
}
} else if payment_context.is_some() {
log_trace!(self.logger, "Failing new HTLC with payment_hash {}: received a keysend payment to a non-async payments context {:#?}", payment_hash, payment_context);
- fail_htlc!(claimable_htlc, payment_hash);
+ fail_htlc!(payment_hash);
} else {
events::PaymentPurpose::SpontaneousPayment(keysend_preimage)
};
Risk score
Our methodology →Why this scored 15/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.