Skip to content

ChannelManager read refactor follow ups #4374

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

Merged
valentinewallace merged 5 commits into from
Feb 5, 2026
Merged

ChannelManager read refactor follow ups #4374

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
77be67b
Group legacy fields in ChannelManagerData to deduplicate comment
joostjager Feb 3, 2026
4a640d1
Use consistent unwrap_or_else pattern for optional TLV fields
joostjager Feb 3, 2026
4790153
Simplify peer_storage_dir from Option<Vec> to Vec in ChannelManagerData
joostjager Feb 4, 2026
07bcba9
Combine claimable HTLCs with purposes in ChannelManagerData
joostjager Feb 5, 2026
b509c4a
Simplify in_flight_monitor_updates from Option<HashMap> to HashMap
joostjager Feb 4, 2026
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
214 changes: 105 additions & 109 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17224,50 +17224,49 @@ pub(super) struct ChannelManagerData<SP: SignerProvider> {
best_block_height: u32,
best_block_hash: BlockHash,
channels: Vec<FundedChannel<SP>>,
// Marked `_legacy` because in versions > 0.2 we are taking steps to remove the requirement of
// regularly persisting the `ChannelManager` and instead rebuild the set of HTLC forwards from
// `Channel{Monitor}` data. See [`ChannelManager::read`].
forward_htlcs_legacy: HashMap<u64, Vec<HTLCForwardInfo>>,
claimable_htlcs_list: Vec<(PaymentHash, Vec<ClaimableHTLC>)>,
claimable_payments: HashMap<PaymentHash, ClaimablePayment>,
peer_init_features: Vec<(PublicKey, InitFeatures)>,
pending_events_read: VecDeque<(events::Event, Option<EventCompletionAction>)>,
highest_seen_timestamp: u32,
// Marked `_legacy` because in versions > 0.2 we are taking steps to remove the requirement of
// regularly persisting the `ChannelManager` and instead rebuild the set of HTLC forwards from
// `Channel{Monitor}` data. See [`ChannelManager::read`].
pending_intercepted_htlcs_legacy: HashMap<InterceptId, PendingAddHTLCInfo>,
pending_outbound_payments: HashMap<PaymentId, PendingOutboundPayment>,
pending_claiming_payments: HashMap<PaymentHash, ClaimingPayment>,
received_network_pubkey: Option<PublicKey>,
monitor_update_blocked_actions_per_peer:
Vec<(PublicKey, BTreeMap<ChannelId, Vec<MonitorUpdateCompletionAction>>)>,
fake_scid_rand_bytes: Option<[u8; 32]>,
claimable_htlc_purposes: Option<Vec<events::PaymentPurpose>>,
probing_cookie_secret: Option<[u8; 32]>,
claimable_htlc_onion_fields: Option<Vec<Option<RecipientOnionFields>>>,
// Marked `_legacy` because in versions > 0.2 we are taking steps to remove the requirement of
// regularly persisting the `ChannelManager` and instead rebuild the set of HTLC forwards from
// `Channel{Monitor}` data. See [`ChannelManager::read`].
decode_update_add_htlcs_legacy: HashMap<u64, Vec<msgs::UpdateAddHTLC>>,
inbound_payment_id_secret: Option<[u8; 32]>,
in_flight_monitor_updates: Option<HashMap<(PublicKey, ChannelId), Vec<ChannelMonitorUpdate>>>,
peer_storage_dir: Option<Vec<(PublicKey, Vec<u8>)>>,
peer_storage_dir: Vec<(PublicKey, Vec<u8>)>,
async_receive_offer_cache: AsyncReceiveOfferCache,
// Marked `_legacy` because in versions > 0.2 we are taking steps to remove the requirement of
// regularly persisting the `ChannelManager` and instead rebuild the set of HTLC forwards from
// `Channel{Monitor}` data.
forward_htlcs_legacy: HashMap<u64, Vec<HTLCForwardInfo>>,
pending_intercepted_htlcs_legacy: HashMap<InterceptId, PendingAddHTLCInfo>,
decode_update_add_htlcs_legacy: HashMap<u64, Vec<msgs::UpdateAddHTLC>>,
}

/// Arguments for deserializing [`ChannelManagerData`].
struct ChannelManagerDataReadArgs<'a, ES: EntropySource, SP: SignerProvider, L: Logger> {
struct ChannelManagerDataReadArgs<
'a,
ES: EntropySource,
NS: NodeSigner,
SP: SignerProvider,
L: Logger,
> {
entropy_source: &'a ES,
node_signer: &'a NS,
signer_provider: &'a SP,
config: UserConfig,
logger: &'a L,
}

impl<'a, ES: EntropySource, SP: SignerProvider, L: Logger>
ReadableArgs<ChannelManagerDataReadArgs<'a, ES, SP, L>> for ChannelManagerData<SP>
impl<'a, ES: EntropySource, NS: NodeSigner, SP: SignerProvider, L: Logger>
ReadableArgs<ChannelManagerDataReadArgs<'a, ES, NS, SP, L>> for ChannelManagerData<SP>
{
fn read<R: io::Read>(
reader: &mut R, args: ChannelManagerDataReadArgs<'a, ES, SP, L>,
reader: &mut R, args: ChannelManagerDataReadArgs<'a, ES, NS, SP, L>,
) -> Result<Self, DecodeError> {
let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);

Expand Down Expand Up @@ -17404,9 +17403,9 @@ impl<'a, ES: EntropySource, SP: SignerProvider, L: Logger>
let mut probing_cookie_secret: Option<[u8; 32]> = None;
let mut claimable_htlc_purposes = None;
let mut claimable_htlc_onion_fields = None;
let mut pending_claiming_payments = Some(new_hash_map());
let mut pending_claiming_payments = None;
let mut monitor_update_blocked_actions_per_peer: Option<Vec<(_, BTreeMap<_, Vec<_>>)>> =
Some(Vec::new());
None;
let mut events_override = None;
let mut legacy_in_flight_monitor_updates: Option<
HashMap<(PublicKey, OutPoint), Vec<ChannelMonitorUpdate>>,
Expand Down Expand Up @@ -17487,34 +17486,103 @@ impl<'a, ES: EntropySource, SP: SignerProvider, L: Logger>
// Resolve events_override: if present, it replaces pending_events.
let pending_events_read = events_override.unwrap_or(pending_events_read);

// Combine claimable_htlcs_list with their purposes and onion fields. For very old data
// (pre-0.0.107) that lacks purposes, reconstruct them from legacy hop data.
let expanded_inbound_key = args.node_signer.get_expanded_key();

let mut claimable_payments = hash_map_with_capacity(claimable_htlcs_list.len());
if let Some(purposes) = claimable_htlc_purposes {
if purposes.len() != claimable_htlcs_list.len() {
return Err(DecodeError::InvalidValue);
}
if let Some(onion_fields) = claimable_htlc_onion_fields {
if onion_fields.len() != claimable_htlcs_list.len() {
return Err(DecodeError::InvalidValue);
}
for (purpose, (onion, (payment_hash, htlcs))) in purposes
.into_iter()
.zip(onion_fields.into_iter().zip(claimable_htlcs_list.into_iter()))
{
let claimable = ClaimablePayment { purpose, htlcs, onion_fields: onion };
let existing_payment = claimable_payments.insert(payment_hash, claimable);
if existing_payment.is_some() {
return Err(DecodeError::InvalidValue);
}
}
} else {
for (purpose, (payment_hash, htlcs)) in
purposes.into_iter().zip(claimable_htlcs_list.into_iter())
{
let claimable = ClaimablePayment { purpose, htlcs, onion_fields: None };
let existing_payment = claimable_payments.insert(payment_hash, claimable);
if existing_payment.is_some() {
return Err(DecodeError::InvalidValue);
}
}
}
} else {
// LDK versions prior to 0.0.107 did not write a `pending_htlc_purposes`, but do
// include a `_legacy_hop_data` in the `OnionPayload`.
for (payment_hash, htlcs) in claimable_htlcs_list.into_iter() {
Copy link
Copy Markdown
Contributor

@valentinewallace valentinewallace Feb 5, 2026

Choose a reason for hiding this comment

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

There's a few minor differences in the code move, like this goes from drain to into_iter here. Maybe it's fine for this PR but in general it's best to keep a pure code move in its own commit.

Copy link
Copy Markdown
Contributor Author

@joostjager joostjager Feb 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I indeed verified before pushing using git diff -M, and from what I saw that was the only change. True that I could have kept that as is.

if htlcs.is_empty() {
return Err(DecodeError::InvalidValue);
}
let purpose = match &htlcs[0].onion_payload {
OnionPayload::Invoice { _legacy_hop_data } => {
if let Some(hop_data) = _legacy_hop_data {
events::PaymentPurpose::Bolt11InvoicePayment {
payment_preimage: match inbound_payment::verify(
payment_hash,
&hop_data,
0,
&expanded_inbound_key,
&args.logger,
) {
Ok((payment_preimage, _)) => payment_preimage,
Err(()) => {
log_error!(args.logger, "Failed to read claimable payment data for HTLC with payment hash {} - was not a pending inbound payment and didn't match our payment key", &payment_hash);
return Err(DecodeError::InvalidValue);
},
},
payment_secret: hop_data.payment_secret,
}
} else {
return Err(DecodeError::InvalidValue);
}
},
OnionPayload::Spontaneous(payment_preimage) => {
events::PaymentPurpose::SpontaneousPayment(*payment_preimage)
},
};
claimable_payments
.insert(payment_hash, ClaimablePayment { purpose, htlcs, onion_fields: None });
}
}

Ok(ChannelManagerData {
chain_hash,
best_block_height,
best_block_hash,
channels,
forward_htlcs_legacy,
claimable_htlcs_list,
claimable_payments,
peer_init_features,
pending_events_read,
highest_seen_timestamp,
pending_intercepted_htlcs_legacy: pending_intercepted_htlcs_legacy
.unwrap_or_else(new_hash_map),
pending_outbound_payments,
// unwrap safety: pending_claiming_payments is guaranteed to be `Some` after read_tlv_fields
pending_claiming_payments: pending_claiming_payments.unwrap(),
pending_claiming_payments: pending_claiming_payments.unwrap_or_else(new_hash_map),
received_network_pubkey,
// unwrap safety: monitor_update_blocked_actions_per_peer is guaranteed to be `Some` after read_tlv_fields
monitor_update_blocked_actions_per_peer: monitor_update_blocked_actions_per_peer
.unwrap(),
.unwrap_or_else(Vec::new),
fake_scid_rand_bytes,
claimable_htlc_purposes,
probing_cookie_secret,
claimable_htlc_onion_fields,
decode_update_add_htlcs_legacy: decode_update_add_htlcs_legacy
.unwrap_or_else(new_hash_map),
inbound_payment_id_secret,
in_flight_monitor_updates,
peer_storage_dir,
peer_storage_dir: peer_storage_dir.unwrap_or_default(),
async_receive_offer_cache,
})
}
Expand Down Expand Up @@ -17749,6 +17817,7 @@ impl<
reader,
ChannelManagerDataReadArgs {
entropy_source: &args.entropy_source,
node_signer: &args.node_signer,
signer_provider: &args.signer_provider,
config: args.config.clone(),
logger: &args.logger,
Expand Down Expand Up @@ -17790,7 +17859,7 @@ impl<
best_block_hash,
channels,
mut forward_htlcs_legacy,
mut claimable_htlcs_list,
claimable_payments,
peer_init_features,
mut pending_events_read,
highest_seen_timestamp,
Expand All @@ -17800,9 +17869,7 @@ impl<
received_network_pubkey,
monitor_update_blocked_actions_per_peer,
mut fake_scid_rand_bytes,
claimable_htlc_purposes,
mut probing_cookie_secret,
claimable_htlc_onion_fields,
mut decode_update_add_htlcs_legacy,
mut inbound_payment_id_secret,
mut in_flight_monitor_updates,
Expand Down Expand Up @@ -18110,11 +18177,9 @@ impl<

let pending_outbounds = OutboundPayments::new(pending_outbound_payments);

if let Some(peer_storage_dir) = peer_storage_dir {
for (peer_pubkey, peer_storage) in peer_storage_dir {
if let Some(peer_state) = per_peer_state.get_mut(&peer_pubkey) {
peer_state.get_mut().unwrap().peer_storage = peer_storage;
}
for (peer_pubkey, peer_storage) in peer_storage_dir {
if let Some(peer_state) = per_peer_state.get_mut(&peer_pubkey) {
peer_state.get_mut().unwrap().peer_storage = peer_storage;
}
}

Expand Down Expand Up @@ -18748,77 +18813,6 @@ impl<
}
}

let expanded_inbound_key = args.node_signer.get_expanded_key();

let mut claimable_payments = hash_map_with_capacity(claimable_htlcs_list.len());
if let Some(purposes) = claimable_htlc_purposes {
if purposes.len() != claimable_htlcs_list.len() {
return Err(DecodeError::InvalidValue);
}
if let Some(onion_fields) = claimable_htlc_onion_fields {
if onion_fields.len() != claimable_htlcs_list.len() {
return Err(DecodeError::InvalidValue);
}
for (purpose, (onion, (payment_hash, htlcs))) in purposes
.into_iter()
.zip(onion_fields.into_iter().zip(claimable_htlcs_list.into_iter()))
{
let claimable = ClaimablePayment { purpose, htlcs, onion_fields: onion };
let existing_payment = claimable_payments.insert(payment_hash, claimable);
if existing_payment.is_some() {
return Err(DecodeError::InvalidValue);
}
}
} else {
for (purpose, (payment_hash, htlcs)) in
purposes.into_iter().zip(claimable_htlcs_list.into_iter())
{
let claimable = ClaimablePayment { purpose, htlcs, onion_fields: None };
let existing_payment = claimable_payments.insert(payment_hash, claimable);
if existing_payment.is_some() {
return Err(DecodeError::InvalidValue);
}
}
}
} else {
// LDK versions prior to 0.0.107 did not write a `pending_htlc_purposes`, but do
// include a `_legacy_hop_data` in the `OnionPayload`.
for (payment_hash, htlcs) in claimable_htlcs_list.drain(..) {
if htlcs.is_empty() {
return Err(DecodeError::InvalidValue);
}
let purpose = match &htlcs[0].onion_payload {
OnionPayload::Invoice { _legacy_hop_data } => {
if let Some(hop_data) = _legacy_hop_data {
events::PaymentPurpose::Bolt11InvoicePayment {
payment_preimage: match inbound_payment::verify(
payment_hash,
&hop_data,
0,
&expanded_inbound_key,
&args.logger,
) {
Ok((payment_preimage, _)) => payment_preimage,
Err(()) => {
log_error!(args.logger, "Failed to read claimable payment data for HTLC with payment hash {} - was not a pending inbound payment and didn't match our payment key", &payment_hash);
return Err(DecodeError::InvalidValue);
},
},
payment_secret: hop_data.payment_secret,
}
} else {
return Err(DecodeError::InvalidValue);
}
},
OnionPayload::Spontaneous(payment_preimage) => {
events::PaymentPurpose::SpontaneousPayment(*payment_preimage)
},
};
claimable_payments
.insert(payment_hash, ClaimablePayment { purpose, htlcs, onion_fields: None });
}
}

// Similar to the above cases for forwarded payments, if we have any pending inbound HTLCs
// which haven't yet been claimed, we may be missing counterparty_node_id info and would
// panic if we attempted to claim them at this point.
Expand Down Expand Up @@ -18849,6 +18843,8 @@ impl<
let mut secp_ctx = Secp256k1::new();
secp_ctx.seeded_randomize(&args.entropy_source.get_secure_random_bytes());

let expanded_inbound_key = args.node_signer.get_expanded_key();

let our_network_pubkey = match args.node_signer.get_node_id(Recipient::Node) {
Ok(key) => key,
Err(()) => return Err(DecodeError::InvalidValue),
Expand Down