Skip to content

Reconstruct ChannelManager forwarded HTLCs maps from Channels #4227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+271 −53
Draft

Reconstruct ChannelManager forwarded HTLCs maps from Channels #4227

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
68cdc2d
Store inbound committed update_adds in Channel
valentinewallace Nov 13, 2025
d023f37
Extract method to dedup pre-decode update_add
valentinewallace Nov 17, 2025
9a7c3f5
Support rebuilding decode_update_add_htlcs from Channels
valentinewallace Nov 17, 2025
1e86521
Rebuild manager forwarded htlcs maps from Channels
valentinewallace Nov 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 49 additions & 26 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,9 @@ enum InboundHTLCState {
/// channel (before it can then get forwarded and/or removed).
/// Implies AwaitingRemoteRevoke.
AwaitingAnnouncedRemoteRevoke(InboundHTLCResolution),
Committed,
Committed {
Copy link
Contributor

@joostjager joostjager Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you touch this, maybe add some comments on the Committed state and the update_add_htlc_opt field.

update_add_htlc_opt: Option<msgs::UpdateAddHTLC>,
},
/// Removed by us and a new commitment_signed was sent (if we were AwaitingRemoteRevoke when we
/// created it we would have put it in the holding cell instead). When they next revoke_and_ack
/// we'll drop it.
Expand All @@ -234,7 +236,7 @@ impl From<&InboundHTLCState> for Option<InboundHTLCStateDetails> {
Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToAdd),
InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) =>
Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToAdd),
InboundHTLCState::Committed =>
InboundHTLCState::Committed { .. } =>
Some(InboundHTLCStateDetails::Committed),
InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay(_)) =>
Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail),
Expand All @@ -253,7 +255,7 @@ impl fmt::Display for InboundHTLCState {
InboundHTLCState::RemoteAnnounced(_) => write!(f, "RemoteAnnounced"),
InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => write!(f, "AwaitingRemoteRevokeToAnnounce"),
InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => write!(f, "AwaitingAnnouncedRemoteRevoke"),
InboundHTLCState::Committed => write!(f, "Committed"),
InboundHTLCState::Committed { .. } => write!(f, "Committed"),
InboundHTLCState::LocalRemoved(_) => write!(f, "LocalRemoved"),
}
}
Expand All @@ -265,7 +267,7 @@ impl InboundHTLCState {
InboundHTLCState::RemoteAnnounced(_) => !generated_by_local,
InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => !generated_by_local,
InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => true,
InboundHTLCState::Committed => true,
InboundHTLCState::Committed { .. } => true,
InboundHTLCState::LocalRemoved(_) => !generated_by_local,
}
}
Expand Down Expand Up @@ -293,7 +295,7 @@ impl InboundHTLCState {
},
InboundHTLCResolution::Resolved { .. } => false,
},
InboundHTLCState::Committed | InboundHTLCState::LocalRemoved(_) => false,
InboundHTLCState::Committed { .. } | InboundHTLCState::LocalRemoved(_) => false,
}
}
}
Expand Down Expand Up @@ -4091,7 +4093,7 @@ where

if self.pending_inbound_htlcs.iter()
.any(|htlc| match htlc.state {
InboundHTLCState::Committed => false,
InboundHTLCState::Committed { .. } => false,
// An HTLC removal from the local node is pending on the remote commitment.
InboundHTLCState::LocalRemoved(_) => true,
// An HTLC add from the remote node is pending on the local commitment.
Expand Down Expand Up @@ -4520,7 +4522,7 @@ where
(InboundHTLCState::RemoteAnnounced(..), _) => true,
(InboundHTLCState::AwaitingRemoteRevokeToAnnounce(..), _) => true,
(InboundHTLCState::AwaitingAnnouncedRemoteRevoke(..), _) => true,
(InboundHTLCState::Committed, _) => true,
(InboundHTLCState::Committed { .. }, _) => true,
(InboundHTLCState::LocalRemoved(..), true) => true,
(InboundHTLCState::LocalRemoved(..), false) => false,
})
Expand Down Expand Up @@ -7303,7 +7305,7 @@ where
payment_preimage_arg
);
match htlc.state {
InboundHTLCState::Committed => {},
InboundHTLCState::Committed { .. } => {},
InboundHTLCState::LocalRemoved(ref reason) => {
if let &InboundHTLCRemovalReason::Fulfill(_, _) = reason {
} else {
Expand Down Expand Up @@ -7396,7 +7398,7 @@ where

{
let htlc = &mut self.context.pending_inbound_htlcs[pending_idx];
if let InboundHTLCState::Committed = htlc.state {
if let InboundHTLCState::Committed { .. } = htlc.state {
} else {
debug_assert!(
false,
Expand Down Expand Up @@ -7531,7 +7533,7 @@ where
for (idx, htlc) in self.context.pending_inbound_htlcs.iter().enumerate() {
if htlc.htlc_id == htlc_id_arg {
match htlc.state {
InboundHTLCState::Committed => {},
InboundHTLCState::Committed { .. } => {},
InboundHTLCState::LocalRemoved(_) => {
return Err(ChannelError::Ignore(format!("HTLC {} was already resolved", htlc.htlc_id)));
},
Expand Down Expand Up @@ -8690,7 +8692,7 @@ where
false
};
if swap {
let mut state = InboundHTLCState::Committed;
let mut state = InboundHTLCState::Committed { update_add_htlc_opt: None };
mem::swap(&mut state, &mut htlc.state);

if let InboundHTLCState::AwaitingRemoteRevokeToAnnounce(resolution) = state {
Expand Down Expand Up @@ -8729,14 +8731,19 @@ where
PendingHTLCStatus::Forward(forward_info) => {
log_trace!(logger, " ...promoting inbound AwaitingAnnouncedRemoteRevoke {} to Committed, attempting to forward", &htlc.payment_hash);
to_forward_infos.push((forward_info, htlc.htlc_id));
htlc.state = InboundHTLCState::Committed;
// TODO: this is currently unreachable, so is it okay? will we lose a forward?
Copy link
Contributor

@joostjager joostjager Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it unreachable?

htlc.state = InboundHTLCState::Committed {
Copy link
Contributor

@joostjager joostjager Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be left out completely because we already swapping this state into the htlc?

update_add_htlc_opt: None,
};
},
}
},
InboundHTLCResolution::Pending { update_add_htlc } => {
log_trace!(logger, " ...promoting inbound AwaitingAnnouncedRemoteRevoke {} to Committed", &htlc.payment_hash);
pending_update_adds.push(update_add_htlc);
htlc.state = InboundHTLCState::Committed;
pending_update_adds.push(update_add_htlc.clone());
htlc.state = InboundHTLCState::Committed {
update_add_htlc_opt: Some(update_add_htlc),
Copy link
Contributor

@joostjager joostjager Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add a comment here explaining why we store the messages?

};
},
}
}
Expand Down Expand Up @@ -9266,7 +9273,7 @@ where
// in response to it yet, so don't touch it.
true
},
InboundHTLCState::Committed => true,
InboundHTLCState::Committed { .. } => true,
InboundHTLCState::LocalRemoved(_) => {
// We (hopefully) sent a commitment_signed updating this HTLC (which we can
// re-transmit if needed) and they may have even sent a revoke_and_ack back
Expand Down Expand Up @@ -14467,6 +14474,7 @@ where
}
}
let mut removed_htlc_attribution_data: Vec<&Option<AttributionData>> = Vec::new();
let mut inbound_committed_update_adds: Vec<Option<msgs::UpdateAddHTLC>> = Vec::new();
(self.context.pending_inbound_htlcs.len() as u64 - dropped_inbound_htlcs).write(writer)?;
for htlc in self.context.pending_inbound_htlcs.iter() {
if let &InboundHTLCState::RemoteAnnounced(_) = &htlc.state {
Expand All @@ -14486,8 +14494,9 @@ where
2u8.write(writer)?;
htlc_resolution.write(writer)?;
},
&InboundHTLCState::Committed => {
&InboundHTLCState::Committed { ref update_add_htlc_opt } => {
3u8.write(writer)?;
inbound_committed_update_adds.push(update_add_htlc_opt.clone());
},
&InboundHTLCState::LocalRemoved(ref removal_reason) => {
4u8.write(writer)?;
Expand Down Expand Up @@ -14860,6 +14869,7 @@ where
(69, holding_cell_held_htlc_flags, optional_vec), // Added in 0.2
(71, holder_commitment_point_previous_revoked, option), // Added in 0.3
(73, holder_commitment_point_last_revoked, option), // Added in 0.3
(75, inbound_committed_update_adds, optional_vec),
});

Ok(())
Expand Down Expand Up @@ -14943,7 +14953,7 @@ where
};
InboundHTLCState::AwaitingAnnouncedRemoteRevoke(resolution)
},
3 => InboundHTLCState::Committed,
3 => InboundHTLCState::Committed { update_add_htlc_opt: None },
4 => {
let reason = match <u8 as Readable>::read(reader)? {
0 => InboundHTLCRemovalReason::FailRelay(msgs::OnionErrorPacket {
Expand Down Expand Up @@ -15229,6 +15239,7 @@ where

let mut pending_outbound_held_htlc_flags_opt: Option<Vec<Option<()>>> = None;
let mut holding_cell_held_htlc_flags_opt: Option<Vec<Option<()>>> = None;
let mut inbound_committed_update_adds_opt: Option<Vec<Option<msgs::UpdateAddHTLC>>> = None;

read_tlv_fields!(reader, {
(0, announcement_sigs, option),
Expand Down Expand Up @@ -15278,6 +15289,7 @@ where
(69, holding_cell_held_htlc_flags_opt, optional_vec), // Added in 0.2
(71, holder_commitment_point_previous_revoked_opt, option), // Added in 0.3
(73, holder_commitment_point_last_revoked_opt, option), // Added in 0.3
(75, inbound_committed_update_adds_opt, optional_vec),
});

let holder_signer = signer_provider.derive_channel_signer(channel_keys_id);
Expand Down Expand Up @@ -15401,6 +15413,17 @@ where
return Err(DecodeError::InvalidValue);
}
}
if let Some(update_adds) = inbound_committed_update_adds_opt {
let mut iter = update_adds.into_iter();
for htlc in pending_inbound_htlcs.iter_mut() {
if let InboundHTLCState::Committed { ref mut update_add_htlc_opt } = htlc.state {
*update_add_htlc_opt = iter.next().ok_or(DecodeError::InvalidValue)?;
}
}
if iter.next().is_some() {
return Err(DecodeError::InvalidValue);
}
}

if let Some(attribution_data_list) = removed_htlc_attribution_data {
let mut removed_htlcs = pending_inbound_htlcs.iter_mut().filter_map(|status| {
Expand Down Expand Up @@ -15985,7 +16008,7 @@ mod tests {
amount_msat: htlc_amount_msat,
payment_hash: PaymentHash(Sha256::hash(&[42; 32]).to_byte_array()),
cltv_expiry: 300000000,
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
Copy link
Contributor

@joostjager joostjager Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok to never populate update_add_htlc_opt in the tests in this commit, if that isn't supposed to happen in prod?

});

node_a_chan.context.pending_outbound_htlcs.push(OutboundHTLCOutput {
Expand Down Expand Up @@ -16831,7 +16854,7 @@ mod tests {
amount_msat: 1000000,
cltv_expiry: 500,
payment_hash: PaymentHash::from(payment_preimage_0),
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
});

let payment_preimage_1 =
Expand All @@ -16841,7 +16864,7 @@ mod tests {
amount_msat: 2000000,
cltv_expiry: 501,
payment_hash: PaymentHash::from(payment_preimage_1),
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
});

let payment_preimage_2 =
Expand Down Expand Up @@ -16881,7 +16904,7 @@ mod tests {
amount_msat: 4000000,
cltv_expiry: 504,
payment_hash: PaymentHash::from(payment_preimage_4),
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
});

// commitment tx with all five HTLCs untrimmed (minimum feerate)
Expand Down Expand Up @@ -17270,7 +17293,7 @@ mod tests {
amount_msat: 2000000,
cltv_expiry: 501,
payment_hash: PaymentHash::from(payment_preimage_1),
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
});

chan.context.pending_outbound_htlcs.clear();
Expand Down Expand Up @@ -17521,7 +17544,7 @@ mod tests {
amount_msat: 5000000,
cltv_expiry: 920150,
payment_hash: PaymentHash::from(htlc_in_preimage),
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
}));

chan.context.pending_outbound_htlcs.extend(
Expand Down Expand Up @@ -17595,7 +17618,7 @@ mod tests {
amount_msat: 100000,
cltv_expiry: 920125,
payment_hash: htlc_0_in_hash,
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
});

let htlc_1_in_preimage =
Expand All @@ -17613,7 +17636,7 @@ mod tests {
amount_msat: 49900000,
cltv_expiry: 920125,
payment_hash: htlc_1_in_hash,
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
});

chan.context.pending_outbound_htlcs.extend(
Expand Down Expand Up @@ -17721,7 +17744,7 @@ mod tests {
amount_msat,
cltv_expiry,
payment_hash,
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
}),
);

Expand Down