Skip to content

resource manager implementation#4409

Open
elnosh wants to merge 8 commits intolightningdevkit:mainfrom
elnosh:resource-mgr
Open

resource manager implementation#4409
elnosh wants to merge 8 commits intolightningdevkit:mainfrom
elnosh:resource-mgr

Conversation

@elnosh
Copy link
Copy Markdown
Contributor

@elnosh elnosh commented Feb 10, 2026

Part of #4384

This PR introduces a ResourceManager trait and DefaultResourceManager implementation of that trait which is based on the proposed mitigation in lightning/bolts#1280.

It only covers the standalone implementation of the mitigation. I have done some testing with integrating it into the ChannelManager but that can be done separately. As mentioned in the issue, the resource manager trait defines these 4 methods to be called from the channel manager:

  • add_channel
  • remove_channel
  • add_htlc
  • resolve_htlc

Integrating into the ChannelManager

  • The ResourceManager is intended to be internal to the ChannelManager rather than users instantiating their own and passing it to a ChannelManager constructor.

  • add/remove_channel should be called when channels are opened/closed.

  • add_htlc: When processing HTLCs, the channel manager would call add_htlc which returns a ForwardingOutcome telling it whether to forward or fail the HTLC along with the accountable signal to use in case that it should be forwarded. For the initial "read-only" mode, the channel manager would log the results but not actually fail the HTLC if it was told to do so. A bit more specific on where it would be called: I think it will be when processing the forward_htlcs before we queue the add_htlc to the outgoing channel

    if let Err((reason, msg)) = optimal_channel.queue_add_htlc(

  • resolve_htlc: Used to tell back the ResourceManager the resolution of an HTLC. It will be used to release bucket resources and update reputation/revenue values internally.

This could have more tests but opening early to get thoughts on design if possible

cc @carlaKC

@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented Feb 10, 2026

👋 Thanks for assigning @carlaKC as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 11, 2026

Codecov Report

❌ Patch coverage is 93.44660% with 108 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.21%. Comparing base (94d1e5e) to head (4e10168).
⚠️ Report is 374 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/resource_manager.rs 93.44% 85 Missing and 23 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4409      +/-   ##
==========================================
+ Coverage   86.03%   87.21%   +1.18%     
==========================================
  Files         156      164       +8     
  Lines      103091   110413    +7322     
  Branches   103091   110413    +7322     
==========================================
+ Hits        88690    96295    +7605     
+ Misses      11891    11609     -282     
+ Partials     2510     2509       -1     
Flag Coverage Δ
fuzzing 39.86% <0.00%> (?)
tests 86.32% <93.44%> (+0.29%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@carlaKC carlaKC self-requested a review February 11, 2026 07:04
Copy link
Copy Markdown
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Really great job on this! Done an overly-specific first review round for something that's in draft because I've taken a look at previous versions of this code before when we wrote simulations. Also haven't looked at the tests in detail yet, but coverage is looking ✨ great ✨ .

I think that taking a look at tracking slot usage in GeneralBucket with a single source of truth is worth taking a look at, seems like it could clean up a few places where we need to two hashmap lookups one after the other.

In the interest of one day fuzzing this, I think it could also use some validation that enforces our protocol assumptions (eg, number of slots <= 483).

@ldk-reviews-bot
Copy link
Copy Markdown

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@elnosh
Copy link
Copy Markdown
Contributor Author

elnosh commented Feb 16, 2026

think I have addressed most of the comments code-wise. Still need to add some requested comments/docs changes.

@elnosh
Copy link
Copy Markdown
Contributor Author

elnosh commented Feb 17, 2026

pushed more fixups addressing requests for adding docs/comments, lmk if those look good

Comment on lines +20 to +28
/// Tracks the occupancy of HTLC slots in the bucket.
slots_occupied: Vec<bool>,

/// SCID -> (slots assigned, salt)
/// Maps short channel IDs to an array of tuples with the slots that the channel is allowed
/// to use and the current usage state for each slot. It also stores the salt used to
/// generate the slots for the channel. This is used to deterministically generate the
/// slots for each channel on restarts.
channels_slots: HashMap<u64, (Vec<(u16, bool)>, [u8; 32])>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this shouldn't accidentally double-assign them.

Yeah it shouldn't (provided we don't have bugs), but tracking the same information (whether a slot is occupied) in multiple places is a design that allows for inconsistency / the possibility of bugs. If we have a single source of truth, we move from "shouldn't double assign" to "can't double assign".

Gave it a shot here, lmk what you think!

@TheBlueMatt
Copy link
Copy Markdown
Collaborator

First of all not sure why all your commit messages are line-wrapped at 40 chars, but you can use like 60 or 70 lol.

Copy link
Copy Markdown
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

A few comments, I think the design is fine, but startup resync may be annoying.

}
}

/// Tracks an average value over multiple rolling windows to smooth out volatility.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm kinda confused by this struct. First of all, the docs here are wrong - we aren't tracking "multiple windows" we're tracking a rolling average over one window of window * window_count. The only difference between this and DecayingAverage is it tries to compensate for if we don't have enough data to actually go back window_count * window. Why shouldn't we just have DecayingAverage do that instead of having a separate struct here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to keep separate because the use of DecayingAverage for reputation differs from AggregatedWindowAverage when tracking revenue. For reputation, we want the DecayingAverage over the full window (24 weeks). For revenue, using AggregatedWindowAverage, we track the decaying average over the same window (24 weeks) but divide by window_count because we want the revenue for 2 weeks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree that we want to track two different things here:

  • Reputation (as DecayingAverage): we want shocks to reflect, so that we can quickly react to a change in attacker behavior
  • Revenue (as AggregatedWindowAverage): we want to smooth shocks to track our peer's average revenue in two weeks over a window_count periods.

But ran some numbers and it does look like we're penalizing old data a bit too much with this approach, as mentioned below.

struct DecayingAverage {
value: i64,
last_updated_unix_secs: u64,
window: Duration,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You don't actually use window (only decay_rate) so we can drop it here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems okay to me to just write the decay_rate directly. We'd only need the window if we wanted to change the way that we calculate it, and that seems unlikely?

// We are not concerned with the rounding precision loss for this value because it is
// negligible when dealing with a long rolling average.
Ok((self.aggregated_revenue_decaying.value_at_timestamp(timestamp_unix_secs)? as f64
/ window_divisor)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't buy this? Let's say our windows_tracked is 4 and we have some data for the last 3 windows. On average, those 3 windows worth of data data will have been multiplied by 0.62175 (https://www.wolframalpha.com/input?i=%28integral+from+0+to+3+%280.5+%5E+0.5%29+%5E+x%29+%2F+3) but then we divide it by three. Whereas if we only have data for a single-window, that data will multiplied by, on average, 0.845111 (https://www.wolframalpha.com/input?i=%28integral+from+0+to+1+%280.5+%5E+0.5%29+%5E+x%29+%2F+1), and then we'll divide by one. We have to factor in the decrease in the data from the decay as well as just the increased amount of data here.

Comment on lines +20 to +28
/// Tracks the occupancy of HTLC slots in the bucket.
slots_occupied: Vec<bool>,

/// SCID -> (slots assigned, salt)
/// Maps short channel IDs to an array of tuples with the slots that the channel is allowed
/// to use and the current usage state for each slot. It also stores the salt used to
/// generate the slots for the channel. This is used to deterministically generate the
/// slots for each channel on restarts.
channels_slots: HashMap<u64, (Vec<(u16, bool)>, [u8; 32])>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does the protection algorithm break if slots are allocated probabilistically? We could reduce implementation complexity a good bit if we just drop channel_slots entirely and generate the list of slots the channel can occupy any time we need it and allow two channels to occupy the same slot (presumably leading to some extra HTLC failures in that case?). This feels very much like a bloom filter problem where we should be able to reduce FPs somehow, though maybe it isn't quite the same because we actually do want conflicts to be "common".

}
}

impl Readable for DefaultResourceManager {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmmmmmmmmmmmmmmmmmmm. Reconciliation on startup is gonna be tricky here. What happens if we accept an HTLC then restart and actually it never made it to disk in the ChannelMonitor? Theoretically this can be persisted as a part of ChannelManager and it should be consistent-ish, but Val is hard at work making it so that we don't have to persist ChannelManager at all.

Instead, I wonder how easy we can make it to rebuild this from HTLC information. It would require some additional integration into "LDK core" but hopefully not much. If we have some HTLCSlotUsage struct that we return from add_htlc in the ForwardingOutcome::Forward case, we could presumably shove that into the HTLCSource (as the lots are "on" the inbound channel) and rebuild the resource manager very cheaply.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What happens if we accept an HTLC then restart and actually it never made it to disk in the ChannelMonitor? Theoretically this can be persisted as a part of ChannelManager and it should be consistent-ish, but Val is hard at work making it so that we don't have to persist ChannelManager at all.

hmmmm yeah I thought about that but was operating under the assumption that by persisting along with the ChannelManager it should stay consistent.

In a world where we don't persist the ChannelManager I was exploring your suggestion to rebuild the resource manager from HTLC data we have on startup and came up with the approach here: elnosh@cdd0bf8 With some caveats, I think we can replay HTLCs by calling add_htlc on the ResourceManager so we would only need general HTLC information and no need to shove bucket/resourcemanager specific information into HTLCSource. We would basically need this HTLC info on startup. I added 2 helper methods in channel.rs and the replay on the ChannelManager could look like this https://github.com/elnosh/rust-lightning/blob/cdd0bf80cb200d370995c4f859645c0a54b3a798/lightning/src/ln/channelmanager.rs#L19303-L19366

With this, I was able to restart a node with pending HTLCs and replayed them fine in the resource manager using Channel data. The only field I would need to add to HTLCSource is incoming_accountable

The caveat is that reputation and in-flight-risk when replaying the HTLCs might be somewhat (slightly) different if the shutdown time was long because the current timestamp is different.

Another approach would be to store the specific bucket usage in the HTLCSource so we replay HTLCs and add them directly to the bucket they were before shutdown. I went with previous approach mentioned since I think that will be less intrusive in the channel manager and would require less resourcemanager-specific information to leak into the channel manager. Let me know what you think

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

My only question there is what the performance cost is. If we have 500 channels and have to replay a hundred HTLCs per channel how bad does it get?

Copy link
Copy Markdown
Contributor Author

@elnosh elnosh Feb 25, 2026

Choose a reason for hiding this comment

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

I'd have to run it but, indeed, it is not optimal because for each outbound HTLC in each channel it needs to lookup the inbound htlc on the incoming channel. It could store the missing fields in the HTLCSource as well to avoid the inbound htlc lookup.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

did alternative approach in 9094319

@elnosh
Copy link
Copy Markdown
Contributor Author

elnosh commented Mar 3, 2026

I have pushed changes for majority of comments from last round - diff here.

The most notable things are:

  • added a PendingHTLCReplay to be passed from upstream by the ChannelManager to replay pending HTLCs on startup instead of writing them in the ResourceManager
  • Do not double-track HTLC slot occupancy in general bucket and only track them in slots_occupied.
  • Use ChaCha instead of sha256 for slot generation in general bucket
  • Added more test cases

@elnosh elnosh marked this pull request as ready for review March 3, 2026 14:38
@valentinewallace valentinewallace requested review from carlaKC and removed request for valentinewallace March 3, 2026 14:40
@ldk-reviews-bot
Copy link
Copy Markdown

🔔 1st Reminder

Hey @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link
Copy Markdown

🔔 2nd Reminder

Hey @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link
Copy Markdown

🔔 3rd Reminder

Hey @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Copy link
Copy Markdown
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Didn't review tests yet, main comment is about how we handle replays on restart (+ saving needing to persist a few things).

struct DecayingAverage {
value: i64,
last_updated_unix_secs: u64,
window: Duration,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems okay to me to just write the decay_rate directly. We'd only need the window if we wanted to change the way that we calculate it, and that seems unlikely?

}
}

/// Tracks an average value over multiple rolling windows to smooth out volatility.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree that we want to track two different things here:

  • Reputation (as DecayingAverage): we want shocks to reflect, so that we can quickly react to a change in attacker behavior
  • Revenue (as AggregatedWindowAverage): we want to smooth shocks to track our peer's average revenue in two weeks over a window_count periods.

But ran some numbers and it does look like we're penalizing old data a bit too much with this approach, as mentioned below.

Comment on lines +146 to +147
// TODO: could return the slots already assigned instead of erroring.
Entry::Occupied(_) => Err(()),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Meant that assign_slots_for_channel doesn't need &self at all - we can just pass in our_scid + per_channel_slots, return the slots/salt we're adding and then have the caller be responsible for adding these values to self.channel_slots.

Saves us a double lookup because we're looking up the entry in the caller (to see if we need to assign_slots_for_channel and looking up again here).

@elnosh elnosh mentioned this pull request Mar 31, 2026
Copy link
Copy Markdown
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Discussed error handling offline in a bit more detail:

  • Error if we create a resource manager that has invalid configuration (we won't right now, because it's hardcoded but in general we shouldn't start if we're given a bad config).
  • debug_assert that we don't fail on add/remove_htlc in channelmanager
  • Failures on adding Channel::new means that the channel is too small to meaningfully protect, we return an error from add_channel and log in channelmanager

@elnosh
Copy link
Copy Markdown
Contributor Author

elnosh commented Apr 1, 2026

addressed comments from last round. Thanks for all the reviews and the patience @carlaKC !
diff here

@elnosh elnosh changed the title resource manager trait and impl resource manager implementation Apr 1, 2026
Implements a decaying average over a rolling window. It will be
used in upcoming commits by the resource manager to track
reputation and revenue of channels.
Copy link
Copy Markdown
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

half way through - submiting b/c I need to change laptops

Copy link
Copy Markdown
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Just one major comment about aligning congestion bucket with the spec, but otherwise this is shaping up!

After addressing the current round, I think that this is ready for a second reviewer. I'd also be happy to open up a new PR for remaining review (with a summary of some of the points discussed in the early review rounds) because gh is starting to unicorn pretty hard on this one now.

The AggregatedWindowAverage implemented here will be used in
upcoming commits to track the incoming revenue that channels have
generated through HTLC forwards.
@elnosh
Copy link
Copy Markdown
Contributor Author

elnosh commented Apr 8, 2026

addressed last comments and made some changes to handle HTLC resolution after force-closes. If a channel has been force-closed, we'll keep it around until all pending HTLCs have been resolved.

@elnosh elnosh requested a review from carlaKC April 8, 2026 12:38
elnosh and others added 5 commits April 8, 2026 12:00
Resources available in the channel will be divided into general,
congestion and protected resources. Here we implement the general
bucket with basic denial of service protections.

Co-authored-by: Carla Kirk-Cohen <kirkcohenc@gmail.com>
Resources available in the channel will be divided into general,
congestion and protected resources. Here we implement the bucket
resources that will be used for congestion and protected.
The Channel struct introduced here has the core information that
will be used by the resource manager to make forwarding decisions
on HTLCs:

- Reputation that this channel has accrued as an outgoing link
in HTLC forwards.

- Revenue (forwarding fees) that the channel has earned us as an
incoming link.

- Pending HTLCs this channel is currently holding as an outgoing link.

- Bucket resources that are currently in use in general, congestion
and protected.
Introduces the DefaultResourceManager struct. The core of methods
that will be used to inform the HTLC forward decisions are
add/resolve_htlc.

- add_htlc: Based on resource availability and reputation, it
evaluates whehther to forward or fail the HTLC.

- resolve_htlc: Releases the bucket resources used from a HTLC
previously added and updates the channel's reputation based on HTLC
fees and resolution times.
Copy link
Copy Markdown
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

This is ready for a second reviewer IMO!

Summary of major points we've discussed so far:

  • Closed channels: we keep closed channels around in the ResoruceManager's state so that we don't need special handling for HTLC resolutions that come after the channel has been closed. This means we have to scan for any usage of our closed channel as an incoming source, but it's not unreasonable given the protocol limits on max HTLC count.
  • Clock progression: the resource manager is tolerant to our clocktime moving backwards to account for the possibility of clock skew. If a time in the past is reported, we simply clamp it to the last (greater) timestamp we were given. An exception to this is the case where a HTLC is added/removed, because we expect several networking roundtrips in between so can reasonably expect the clock to more forward in this case - if the clock moves backwards here, we've likely got a bug in the calling code. By contrast, we could add/remove two distinct HTLCs in very close succession and can't distinguish between clock skew and a bug in the caller. Clamping has little impact on our reputation scheme, so we err on the side of being error tolerant.
  • Error handling: in the PR that follows, we'll debug_assert and log if there are any ResourceManager errors. We don't want to shut down, because we're not at risk of losing funds, but we do want people to report that something has gone wrong.
  • HTLC removal best effort: if we have a bug in our HTLC removal (say, we specify the wrong incoming channel and can't find the HTLC in the ResourceManager, and thus can't remove it), we're at risk of degraded routing performance if the ResourceManager's view of the HTLC state becomes out of sync with reality (when we turn this on for real). Even though this constitutes a pretty dire bug (which we should catch it in review/with debug_assert), we still attempt to handle this gracefully by cleaning up all the resources we're able to.

Some followup items we shouldn't forget:

  • Allow HTLCs to use resources across buckets. Eg: if a HTLC is just too big for protected, and the peer has sufficient reputation there's no reason not to allow them some of the space in general as well. Right now we restrict to fitting exactly in a single bucket, where we could probably be more permissive. This is okay for read-only, and we'll be able to get an idea whether it's actually a problem once we get some data out of the read-only impl. But worth remembering.
  • Right now if we add a channel with < 12 HTLCs, we won't add it to our state and we'll fail every time a HTLC using that channel is added/removed (because we didn't track it). This seems fine (to me) for a readonly impl, but we should add handle this more gracefully (as not adding a channel means it can be used to grief us).
  • We don't have support for the upgrade_accountability signal from the onion (which is relevant for bumping up to congestion or protected). When we add this to LDK, we should hook it into this system.
  • If a channel has zero fees, just use the default fee that "would have been" for reputation purposes.
  • Adding some informative logging in a follow up.

// If the channel has pending HTLCs, it is soft-deleted and will be fully removed once
// all its pending HTLCs have been resolved.
let channel = channels_lock.get_mut(&channel_id).ok_or(())?;
if channel.pending_htlcs.is_empty() && !incoming_htlcs_pending {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I belive that rust will short-circuit if channel.pending_htlcs.is_empty() so we might as well inline the incoming_htlcs_pending scan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

6 participants