search

lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.16k stars 366 forks source link

Use substitude channel for forwarding if we have another with the same peer #1278

Closed TheBlueMatt closed 4 months ago

TheBlueMatt commented 2 years ago

I believe we're now the only major lightning implementation that insists on forwarding HTLCs out over the same channel as described in the SCID in the onion. Instead, all other implementations will happily forward over a different channel if another one is available to the same counterparty node and it doesn't have enough liquidity in the first channel. We should do the same.

jurvis commented 2 years ago

hi @ViktorTigerstrom, does your PR https://github.com/lightningdevkit/rust-lightning/pull/1325 address this issue?

ViktorTigerstrom commented 2 years ago

Hi @jurvis, no not really. My PR does not deal with forwarding HTCLs at all.

The only way my PR could be seen as slightly related to this is that when our implementation is used to generate the invoice, the hints generally won't include channels with too low inbound capacity, so that this scenario hopefully shouldn't occur at end of the path (meaning the sender hopefully shouldn't construct a route for the last hop in which the 2nd last node will face this problem). But that's stretching it a bit :)

jurvis commented 2 years ago

@ViktorTigerstrom ah okay -- I saw that this issue was tagged in #1279 so I wanted to clarify. thank you!

ViktorTigerstrom commented 2 years ago

I'd be interested in trying to this one up, unless you think this one is too big to tackle at the moment. It would be fun to work on something a bit more substantial, and to get into the HTLC handling!

Before I dig too deep into this, I just have a few questions which would be really helpful to get an answer to:

Do I understand the issue correctly, that we should forward the HTLC over another channel with the same counterparty node (if a channel with enough outbound balance exists) if send_htlc fails here due too low outbound balance in the channel? https://github.com/lightningdevkit/rust-lightning/blob/29727a37fc0534270e899ad4ede0ea4c649d2aab/lightning/src/ln/channelmanager.rs#L2987

Do I also understand it correctly, that in order to do that in reasonably effective way, we would need to update the ChannelManager::per_peer_state to hold the Channels per counterparty node, so that the other channels for the same counterparty node could be found easily?

We'd also need to refactor the code in the if clause here, so that it uses the correct channel for the corresponding add_htlc_msgs if another channel was used to forward the payment: https://github.com/lightningdevkit/rust-lightning/blob/29727a37fc0534270e899ad4ede0ea4c649d2aab/lightning/src/ln/channelmanager.rs#L3049

Are there other aspects to this issue that complicates it further?

TheBlueMatt commented 2 years ago

Instead of doing this all at once, probably best to break it into small chunks and make progress over time. eg if you first redo the storage of channels, try tackling #105 as a side-effect of doing so then coming back to this one.

ViktorTigerstrom commented 2 years ago

Ok thanks, sounds like a good idea! I'll start by looking into that :)