Open ziggie1984 opened 3 months ago
[!IMPORTANT]
Review skipped
Auto reviews are limited to specific labels.
Labels to auto review (1)
* llm-reviewPlease check the settings in the CodeRabbit UI or the
.coderabbit.yaml
file in this repository. To trigger a single review, invoke the@coderabbitai review
command.You can disable this status message by setting the
reviews.review_status
tofalse
in the CodeRabbit configuration file.
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
So this is still a draft and no tests added yet, tho I want your spotlight on the following topics before continuing:
See also https://github.com/lightningnetwork/lnd/issues/8803#issuecomment-2308899898 for the overall design.
hodlHTLCDisp
in parcticular the following question in regards of the subscriber:I had to change the EventReceiver because I wanted to be flexible in using a particular ID for the subsriber, so I went with the short channel ID, so that when an HTLC gets resolved I can quickly look up the relevant subscriber just by using the shortchanID which is part of the HTLC (resolution) info.
Another approach would be that we always register the correct subscriber to an Hodl HTLC meaning that instead of:
hodlMap fn.Set[models.CircuitKey]
we would need to change the set to a map with the subscribers as a value
hodlMap map[models.CircuitKey]fn.EventReceiver
Then we would need to add the subscriber in the invoices.NotifyExitHop
call. Then when resolving we would based on the circuit key get the receiver/subscriber we want to send this resolution to
If we do it the latter way, I think there would be no need for the introduction of an interface of the EventReceiver.
type hodlHTLCNotifier interface {
fn.EventPublisher[HtlcResolution, bool]
publishSubscriberEvent(res HtlcResolution) error
recordHTLC(htlc models.CircuitKey) error
Start() error
Stop() error
}
I had to change the EventReceiver because I wanted to be flexible in using a particular ID for the subsriber, so I went with the short channel ID, so that when an HTLC gets resolved I can quickly look up the relevant subscriber just by using the shortchanID which is part of the HTLC (resolution) info.
Unless I'm missing something, I don't see how the introduction of the interface actually helps things here. I see no mention of the ShortChannelID in the updated Impl or in a new implementation.
Another approach would be that we always register the correct subscriber to an Hodl HTLC meaning that instead of:
This seems like the way I would try to handle it.
Overall I don't know that I can endorse the HTLCNotifier interface, mostly because I don't see what it is solving. The main issue at hand here is that the invoice registry has a lot of its data structures being accessed concurrently from different threads. In other parts of LND we've started to take the approach of moving everything into the main event loop of the component. So rather than having methods on the invoice registry that directly modify data structures, locked or otherwise. Instead, we describe the actions we want to occur and then send those over some channel into the main event loop. This guarantees sequential processing of all data owned by the invoice registry which will then allow us to remove locking infrastructure which should drastically reduce the deadlock risk.
Fixes https://github.com/lightningnetwork/lnd/issues/8803
Depends on https://github.com/lightningnetwork/lnd/pull/8826