← Watch feed
Require `htlc_value_satoshis` in [pending] `HTLCUpdate`s
Public commit record
85/100 · Strong
What the developer wrote
Authored by Matt Corallo
Require `htlc_value_satoshis` in [pending] `HTLCUpdate`s
In 0.0.100 we started tracking the amounts being claimed in
`OnchainEvent::HTLCUpdate` and then also in
`MonitorEvent::HTLCUpdate`'s `HTLCUpdate`. It was always set, but
stored as an `Option` to support further downgrade. Because these
objects time out after `ANTI_REORG_DELAY` (6) blocks, there's not
really much reason to keep supporting backwards compatibility to
upgrade with such objects without an amount.
In 0.0.115, we started providing the amount in `PaymentForwarded`.
For whatever reason, despite the event only being generated in
cases where we had amounts, the field was added as an `Option`.
Still, in 0.0.118 we started generating them from both off-chain
and on-chain claims. For off-chain claims it was always set, but
for claims which originated from on-chain claims, the amounts came
from the `MonitorEvent::HTLCUpdate` and thus were always an
`Option`. If we no longer care about `MonitorEvent::HTLCUpdate`'s
without a claim amount, we no longer need to worry about
`Event::PaymentForwarded` either. Thus, we make it required here
as well.
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode
What changed, and why it matters
This commit removes the 'Option' wrapper from several HTLC amount fields, making them required instead of optional. It is a cleanup/refactoring change that simplifies the code by assuming the amount is always known. The commit message frames this as removing legacy downgrade support, not as fixing a security bug. There is no direct evidence in the diff of an exploitable vulnerability.
Recommended action
Treat as a compatibility/refactoring commit rather than a security fix. Reviewers should verify that all code paths that construct these objects indeed always populate the amount, and that the backward-compatibility break is acceptable for the supported upgrade window. No immediate security patch action is indicated by the commit itself.
Security signals we found
01
Removal of Option wrapper for financial amount fields
02
Serialization format change from optional to required TLV fields
03
Loss of backward compatibility with older serialized monitor/channel state
04
Potential deserialization failure if old state missing required amount fields
Evidence from the diff
The patch changes htlc_value_satoshis in HTLCUpdate and OnchainEvent::HTLCUpdate, and outbound_amount_forwarded_msat in Event::PaymentForwarded, from Option<u64> to u64. Serialization tags are changed from option to required. Callers are updated to pass bare u64 values instead of Some(...). The change is motivated by the observation that these values are always populated in current code paths, so the optional type is unnecessary. The commit removes downgrade compatibility for old serialized objects that lacked these amounts.
Changed components
lightning/src/chain/channelmonitor.rslightning/src/events/mod.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_tests.rsInspect captured patch +24 / −28
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index a2412bb..24c1031 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -254,11 +254,11 @@ pub struct HTLCUpdate {
pub(crate) payment_hash: PaymentHash,
pub(crate) payment_preimage: Option<PaymentPreimage>,
pub(crate) source: HTLCSource,
- pub(crate) htlc_value_satoshis: Option<u64>,
+ pub(crate) htlc_value_satoshis: u64,
}
impl_ser_tlv_based!(HTLCUpdate, {
(0, payment_hash, required),
- (1, htlc_value_satoshis, option),
+ (1, htlc_value_satoshis, required),
(2, source, required),
(4, payment_preimage, option),
});
@@ -529,7 +529,7 @@ enum OnchainEvent {
HTLCUpdate {
source: HTLCSource,
payment_hash: PaymentHash,
- htlc_value_satoshis: Option<u64>,
+ htlc_value_satoshis: u64,
/// None in the second case, above, ie when there is no relevant output in the commitment
/// transaction which appeared on chain.
commitment_tx_output_idx: Option<u32>,
@@ -614,7 +614,7 @@ impl MaybeReadable for OnchainEventEntry {
impl_writeable_tlv_based_enum_upgradable!(OnchainEvent,
(0, HTLCUpdate) => {
(0, source, required),
- (1, htlc_value_satoshis, option),
+ (1, htlc_value_satoshis, required),
(2, payment_hash, required),
(3, commitment_tx_output_idx, option),
},
@@ -2688,7 +2688,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
debug_assert!(htlc_spend_tx_opt.is_none());
htlc_spend_tx_opt = event.transaction.as_ref();
debug_assert!(holder_timeout_spend_pending.is_none());
- debug_assert_eq!(htlc_value_satoshis.unwrap(), htlc.amount_msat / 1000);
+ debug_assert_eq!(htlc_value_satoshis, htlc.amount_msat / 1000);
holder_timeout_spend_pending = Some(event.confirmation_threshold());
},
OnchainEvent::HTLCSpendConfirmation { commitment_tx_output_idx, preimage, .. }
@@ -3343,7 +3343,7 @@ macro_rules! fail_unbroadcast_htlcs {
event: OnchainEvent::HTLCUpdate {
source: (**source).clone(),
payment_hash: htlc.payment_hash.clone(),
- htlc_value_satoshis: Some(htlc.amount_msat / 1000),
+ htlc_value_satoshis: htlc.amount_msat / 1000,
commitment_tx_output_idx: None,
},
};
@@ -4506,7 +4506,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
if self.counterparty_fulfilled_htlcs.get(&SentHTLCId::from_source(source)).is_some() {
continue;
}
- let htlc_value_satoshis = Some(amount_msat / 1000);
+ let htlc_value_satoshis = amount_msat / 1000;
let logger = WithContext::from(logger, None, None, Some(payment_hash));
// Defensively mark the HTLC as failed back so the expiry-based failure
// path in `block_connected` doesn't generate a duplicate `HTLCUpdate`
@@ -5936,7 +5936,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
source: source.clone(),
payment_preimage: None,
payment_hash: htlc.payment_hash,
- htlc_value_satoshis: Some(htlc.amount_msat / 1000),
+ htlc_value_satoshis: htlc.amount_msat / 1000,
}));
}
}
@@ -6353,7 +6353,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
source,
payment_preimage: Some(payment_preimage),
payment_hash,
- htlc_value_satoshis: Some(amount_msat / 1000),
+ htlc_value_satoshis: amount_msat / 1000,
}));
}
} else if offered_preimage_claim {
@@ -6377,7 +6377,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
source,
payment_preimage: Some(payment_preimage),
payment_hash,
- htlc_value_satoshis: Some(amount_msat / 1000),
+ htlc_value_satoshis: amount_msat / 1000,
}));
}
} else {
@@ -6398,7 +6398,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
event: OnchainEvent::HTLCUpdate {
source,
payment_hash,
- htlc_value_satoshis: Some(amount_msat / 1000),
+ htlc_value_satoshis: amount_msat / 1000,
commitment_tx_output_idx: Some(input.previous_output.vout),
},
};
diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs
index 1f1a358..ad493a1 100644
--- a/lightning/src/events/mod.rs
+++ b/lightning/src/events/mod.rs
@@ -1525,7 +1525,7 @@ pub enum Event {
/// The final amount forwarded, in milli-satoshis, after the fee is deducted.
///
/// The caveat described above the `total_fee_earned_msat` field applies here as well.
- outbound_amount_forwarded_msat: Option<u64>,
+ outbound_amount_forwarded_msat: u64,
},
/// Used to indicate that a channel with the given `channel_id` is being opened and pending
/// confirmation on-chain.
@@ -2225,7 +2225,7 @@ impl Writeable for Event {
(1, Some(legacy_prev.channel_id), option),
(2, claim_from_onchain_tx, required),
(3, Some(legacy_next.channel_id), option),
- (5, outbound_amount_forwarded_msat, option),
+ (5, outbound_amount_forwarded_msat, required),
(7, skimmed_fee_msat, option),
(9, legacy_prev.user_channel_id, option),
(11, legacy_next.user_channel_id, option),
@@ -2763,7 +2763,7 @@ impl MaybeReadable for Event {
let mut total_fee_earned_msat = None;
let mut skimmed_fee_msat = None;
let mut claim_from_onchain_tx = false;
- let mut outbound_amount_forwarded_msat = None;
+ let mut outbound_amount_forwarded_msat = 0;
let mut prev_htlcs = vec![];
let mut next_htlcs = vec![];
read_tlv_fields!(reader, {
@@ -2771,7 +2771,7 @@ impl MaybeReadable for Event {
(1, prev_channel_id_legacy, option),
(2, claim_from_onchain_tx, required),
(3, next_channel_id_legacy, option),
- (5, outbound_amount_forwarded_msat, option),
+ (5, outbound_amount_forwarded_msat, required),
(7, skimmed_fee_msat, option),
(9, prev_user_channel_id_legacy, option),
(11, next_user_channel_id_legacy, option),
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index b86168a..93dfd1c 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -10459,7 +10459,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
fn claim_funds_internal(
&self, source: HTLCSource, payment_preimage: PaymentPreimage,
- forwarded_htlc_value_msat: Option<u64>, skimmed_fee_msat: Option<u64>, from_onchain: bool,
+ forwarded_htlc_value_msat: u64, skimmed_fee_msat: Option<u64>, from_onchain: bool,
next_channel_counterparty_node_id: PublicKey, next_channel_outpoint: OutPoint,
next_channel_id: ChannelId, next_user_channel_id: Option<u128>,
attribution_data: Option<AttributionData>, send_timestamp: Option<Duration>,
@@ -10527,12 +10527,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
payment_preimage,
|htlc_claim_value_msat: Option<u64>| -> Option<events::Event> {
let total_fee_earned_msat =
- if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
- if let Some(claimed_htlc_value) = htlc_claim_value_msat {
- Some(claimed_htlc_value - forwarded_htlc_value)
- } else {
- None
- }
+ if let Some(claimed_htlc_value) = htlc_claim_value_msat {
+ Some(claimed_htlc_value - forwarded_htlc_value_msat)
} else {
None
};
@@ -13103,7 +13099,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
self.claim_funds_internal(
htlc_source,
msg.payment_preimage.clone(),
- Some(forwarded_htlc_value),
+ forwarded_htlc_value,
skimmed_fee_msat,
false,
*counterparty_node_id,
@@ -14122,7 +14118,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
self.claim_funds_internal(
htlc_update.source,
preimage,
- htlc_update.htlc_value_satoshis.map(|v| v * 1000),
+ htlc_update.htlc_value_satoshis * 1000,
None,
true,
counterparty_node_id,
@@ -21183,7 +21179,7 @@ impl<
channel_manager.claim_funds_internal(
source,
preimage,
- Some(downstream_value),
+ downstream_value,
None,
downstream_closed,
downstream_node_id,
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 826b077..b84a486 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -1508,7 +1508,7 @@ pub fn test_htlc_on_chain_success() {
assert_eq!(prev_htlcs[0].channel_id, chan_id);
assert_eq!(claim_from_onchain_tx, true);
assert_eq!(next_htlcs[0].channel_id, chan_2.2);
- assert_eq!(outbound_amount_forwarded_msat, Some(3000000));
+ assert_eq!(outbound_amount_forwarded_msat, 3000000);
},
_ => panic!(),
}
@@ -1525,7 +1525,7 @@ pub fn test_htlc_on_chain_success() {
assert_eq!(prev_htlcs[0].channel_id, chan_id);
assert_eq!(claim_from_onchain_tx, true);
assert_eq!(next_htlcs[0].channel_id, chan_2.2);
- assert_eq!(outbound_amount_forwarded_msat, Some(3000000));
+ assert_eq!(outbound_amount_forwarded_msat, 3000000);
},
_ => panic!(),
}
@@ -4046,7 +4046,7 @@ pub fn test_onchain_to_onchain_claim() {
assert_eq!(prev_htlcs[0].channel_id, chan_1.2);
assert_eq!(claim_from_onchain_tx, true);
assert_eq!(next_htlcs[0].channel_id, chan_2.2);
- assert_eq!(outbound_amount_forwarded_msat, Some(3000000));
+ assert_eq!(outbound_amount_forwarded_msat, 3000000);
},
_ => panic!("Unexpected event"),
}
Risk score
Our methodology →Why this scored 25/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.