Skip to content
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.

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

Reject new inbound channels / HTLC forward if fee-bumping reserves are low #2320

Open
Tracked by #2297
ariard opened this issue May 25, 2023 · 5 comments
Open
Tracked by #2297

Comments

@ariard
Copy link

ariard commented May 25, 2023

If the number of opened channels is too high and our fee-bumping reserves are too low, we should reject new inbound channels to avoid a situation where we cannot cover high feerates at current level of network mempools congestion. This type of mechanism is already implemented by LND.

#2089’s CoinSelectionSource could return the level of fee-bumping reserves currently available (e.g get_available_utxos). This method could be called by ChannelManager::timer_tick_occurred and in function refuse new channels in ``do_accept_inbound_channels`.

Sounds a follow-up of #2089.

(merged from #2327)

If the numbers of pending HTLCs is too high and our fee-bumping reserves are too low, we should reject new HTLC forwarding request to avoid a situation where we cannot cover high feerates at current level of network mempools congestion. This type of mechanism is not implemented by any Lightning implementation, afaik.

This type of checks could be introduced in the same code path where we're checking other BOLT4 checks in decode_update_add_htlc_onion() or offloaded to the LDK user if we can have a generic HTLC interception interface.

@wpaulino
Copy link
Contributor

This is something I discussed with @TheBlueMatt recently. We may not want to impose this restriction on all LDK users, e.g., it seems that mobile wallets may rely on their LSP always playing their commitment onchain such that users don't have to be concerned about maintaining an onchain funds reserve.

At the very least, we'll require inbound anchors channels to be accepted manually, such that LDK users can decide whether they want to uphold a reserve or not.

@TheBlueMatt
Copy link
Collaborator

Yea this is gonna need careful and explicit documentation and maybe some variables that say how much LDK thinks you need in the channel open event and then I feel like manual acceptance suffices. I'm not sure what else we can really do - an implementation using LDK can always simply lie to LDK to accept channels in excess of a reserve. Anything else would require querying the user's UTXO set during channel acceptance, which also kinda sucks.

@ariard
Copy link
Author

ariard commented May 31, 2023

This is something I discussed with @TheBlueMatt recently. We may not want to impose this restriction on all LDK users, e.g., it seems that mobile wallets may rely on their LSP always playing their commitment onchain such that users don't have to be concerned about maintaining an onchain funds reserve.

At the very least, we'll require inbound anchors channels to be accepted manually, such that LDK users can decide whether they want to uphold a reserve or not.

Yea this is gonna need careful and explicit documentation and maybe some variables that say how much LDK thinks you need in the channel open event and then I feel like manual acceptance suffices. I'm not sure what else we can really> do - an implementation using LDK can always simply lie to LDK to accept channels in excess of a reserve. Anything else would require querying the user's UTXO set during channel acceptance, which also kinda sucks.

Sure, for mobile wallets it is expected they do not accept by default inbound channels, therefore maintaining onchain funds reserve is not a concern for them. In the other direction, if you're a LSP or a routing hops who have to accept inbound channels from mobile users or other routing hops in a blinded fashion (i.e no information beyond the node pubkey), every new acceptance is a risk exposure in case of network mempools congestion spikes.

I think automatic logic where the level of fee-bumping reserves (CoinSelectionSource::get_available_utxos()) is probed in ChannelManager::timer_tick_occured is more protective as you can connect with you fee-estimator interface to have real-time monitoring in function of network mempools congestions (rather than suggestions in hardcoded documentation breaking as soon as sat/vb fluctuates). Of course, an implementation using LDK can always lie to LDK to accept channels excess of a reserve, though this would be the same than a Listen::block_connected() implementation, "faking" blocks connections, we can assume our LDK users are in good faith.

Beyond there is a last new consideration in having automatic logic, beyond feerates fluctuations, everytime we introduce a new channel types (e.g Taproot), the witness weights are changing and therefore changing any documentation rational or the checks than a LDK user could implement.

For the separation between mobile and servers, we can have a config knob disable_fee_bumping_reserve_monitoring or disable_inbound_channels_safety_checks.

@TheBlueMatt
Copy link
Collaborator

Currently we don't have any references to the coin selector in the "main" ldk objects, and it really sucks to change that :( - it both adds reentrancy in yet another API, but also adds a fairly huge layer violation in some node designs.

While we should have some kind of logic to allow users with UTXOs to limit their need-for-anchor-able-balance exposure, I'm not really convinced we have the magic solution and can really implement it on the LDK end. In general, if the HTLC timeouts are long enough, its not even about UTXO count but rather total balance * feerate (now or in the future).

One of the main goals of anchors is to get away from all these feerate-dependent limits that require predicting the feerate in the future - adding some HTLC/channel gating on the current feerate (plus some delta?) kinda defeats that goal.

All that said, it is obviously an important security limit, and if we don't apply it, users may not, which would be pretty bad. I'm just not convinced "poll the users' UTXO set and make decisions for them" is really the best way to apply that limit. A simpler option would be to allow the user to set some global config knob with the number outputs they think they can claim (or amount they have available to claim outputs, which we multiply by the feerate as we see new channels/HTLCs).

@ariard ariard changed the title Reject new inbound channels if fee-bumping reserves are low Reject new inbound channels / HTLC forward if fee-bumping reserves are low Jul 9, 2023
@ariard
Copy link
Author

ariard commented Jul 9, 2023

Yeah I think such api with references to the coin selector passed in the “main” ldk objects isn’t the best direction. Maybe we could have another approach where BumpTransactionEventHandler yield events to a new module and new Event::ChannelLoad, though we would have to care about tracking events in a way which is a) real-time and b) not impact routing latency perf, I guess.

I think such solution still have to encompass number of UTXOs and their feerate values, as in the ideal world you have as much UTXOs than you need to cover all your second-stage HTLCs commitments fee-bumping, without shooting yourself in the foot due to transaction-relay policy.

I know the goal of anchor outputs was to move away from all those feerate prediction heuristic, though here I think this is more short-coming of the design of anchor outputs. At least we have removed the dependency on counterparty signature cooperation in term of time-sensitive transaction fee-setting with option_anchors_zero_fee_htlc_tx, which is already a notable security advance!

I think we could have a lightning-fee-bumping-management module (with sane default security settings) in its own crate consuming events and telling back “reactions” to the node, though the big wonder on my side is if we wouldn’t be better off having fee-estimation based on historical buckets, see past conversations on #1056 as a first step to build such new module.

I think we can add a new label “Brainstorming” like we have on the Core side, signaling it’s good to have more conceptual and interface feedback before to do something here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants