Skip to content

Add fee-bumping reserves recommendations for anchor channels #2450

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

Closed

Conversation

ariard
Copy link

@ariard ariard commented Jul 25, 2023

This PR introduces recommendations for the fee-bumping reserves that a LDK user using anchor outputs channels is required to maintain in a future where update_fee as a fee-updating mechanism is deprecated, or its impact reduced.

The recommendations are given on an indicative title only, and in no way are compulsive. Recommendations are conservative for the worst-case of the routing hop, one might have to envision lower level of thresholds if the channels are with “semi-trusted” peers, HTLC forwarding is blocked or fee-bumping is relied on a blessed third-party (e.g a LSP).

While the soundest recommendations might be translated in code default in the future, I think (and hope) this documentation will be always hopeful for LDK users integrating their own wallets with our fee-bumping traits like CoinSelectionSource or WalletSource.

Copy link
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.

Thanks for writing this up! Handful of comments and I think we should try hard to lighten up the wording to make it as understandable as possible (mostly by trying to make it less wordy - pretend every paragraph has to fit in a tweet and cut it down accordingly 😀).


Under the Lightning security model, commitment and second-stage HTLC transactions (BOLT3's `HTLC-timeout` and `HTLC-preimage` and the corresponding spends on counterparty commitment transaction) must be realized
in a time-sensitive manner if there are pending HTLCs. Lack of chain confirmation before HTLC expiration timelocks can constitute a loss of funds, as the counterparty can claim the HTLC output with a [competing
transaction](https://bitcoinops.org/en/blog/waiting-for-confirmation/#policy-as-an-interface). Failure of confirmation might be provoked by blockspace demand spikes provoked by unrelated Bitcoin on-chain transaction
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why this link is here. I don't think it adds to the understandability of the overall sentence. Also I would drop the second sentence.

Copy link
Author

Choose a reason for hiding this comment

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

I don’t know if you have read the link, I think the following excerpt is a nice reminder about Lightning security model about time-sensitive transaction.

"Contracting protocols are even more intimately dependent on policies related to prioritization because enforceability on-chain depends on being able to get transactions confirmed quickly. In adversarial environments, cheating counterparties may have an interest in delaying a transaction’s confirmation, so we must also think about how quirks in the transaction relay policy interface can be used against a user.”

Do you think I should rather take a digest in fee-bumping-reserves.md rather than quoting a link ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because this sentence isn't about policy and edge-cases, its purely a high-level introduction stating that there's outputs which are competing.

Copy link
Author

Choose a reason for hiding this comment

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

Which is nice to refresh users memory on anchor outputs channels and the competing balance interest ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, but the sentences here themselves seem more than sufficient to remind folks of the ln security model, the fact that LDK itself also has to consider transaction standardness policies seems entirely unrelated?

Copy link
Author

Choose a reason for hiding this comment

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

the fact that LDK itself also has to consider transaction standardness policies seems entirely unrelated?

I wish it was that simple, though e.g with nversin=3 there is a limit on the number of package descendant, and therefore how many commitment a CPFP can cover as one example coming out of mind.

Though I think you’re right in the let’s start simple approach, removed the whole section. I still added a warning at the end “Those recommendations should be also revised in function of the interactions with transaction standardness policy and its evolutions (e.g number of transactions allowed to be relayed as a package and as such how many LN commitment one CPFP can cover)”.

issuers. Anchor output enables to remedy to this issue by allowing a Lightning node to dynamically adjust the feerate of its commitment and second-stage HTLC transactions in an unilateral way.

However, anchor output channels are requiring from the user the provision and maintenance of fee-bumping reserves to attach feerate increase to the commitment (via a Child-Pay-For-Parent) or the
second-stage HTLC transactions (via `SIGHASH_SINGLE | SIGHASH_ANYONECANPAY`). Those fee-bumping reserves should be a collection of UTXOs, of which the state and signing capabilites should be maintained
Copy link
Collaborator

Choose a reason for hiding this comment

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

To someone with only a cursory knowledge of Bitcoin, mentioning HTLC transactions and SIGHASH modes is confusing. Let's just say CPFP and adding additional inputs in RBF.

Copy link
Author

Choose a reason for hiding this comment

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

That’s a good question, what level of knowledge we expect from the user reading the doc ? In my mind it’s more an application developer building on top of the raw LDK API or someone maintaining an enterprise Lightning infrastructure or advanced hobbyist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just because they're an application developer who knows a good bit about integrating Bitcoin APIs and building a good UX doesn't mean they understand (or, maybe, remember) what specific sighash flags mean.

Copy link
Author

Choose a reason for hiding this comment

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

I’m taking the optimistic point than an application developer will search for the sighash flags and get smarter from our documentation, I can add more docs about sighash flags ? e.g https://bitcoin.stackexchange.com/questions/37093/what-goes-in-to-the-message-of-a-transaction-signature/37095#37095

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see why RBF is less useful for users. We could dump information at them, or we could refer to it in the common terminology and leave it, and its not like the additional information adds context which allows users to make better decisions here.

Copy link
Author

Choose a reason for hiding this comment

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

For now I took the simplest approach of just mentioning replace-by-fee. I think in the future it can be still interesting to develop the point on sighash flags, as this malleability allow external fee-bumping e.g by a LSP or watchtower.

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2023

Codecov Report

Patch coverage has no change and project coverage change: +1.71% 🎉

Comparison is base (c383f06) 90.24% compared to head (031c8e5) 91.96%.
Report is 181 commits behind head on main.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2450      +/-   ##
==========================================
+ Coverage   90.24%   91.96%   +1.71%     
==========================================
  Files         106      111       +5     
  Lines       55811    78059   +22248     
  Branches    55811    78059   +22248     
==========================================
+ Hits        50368    71786   +21418     
- Misses       5443     6273     +830     
Files Changed Coverage Δ
lightning/src/events/bump_transaction.rs 77.55% <ø> (ø)
lightning/src/util/config.rs 56.56% <ø> (ø)

... and 71 files with indirect coverage changes

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

@ariard ariard force-pushed the 2023-07-fee-bumping-reserves-doc branch from 8df6989 to b8fcd03 Compare August 1, 2023 01:30
@ariard
Copy link
Author

ariard commented Aug 1, 2023

Updated at b8fcd03e with some comments addressed.

See the high-level feedback on a) documentation format and b) user level knowledge expected as other points will derive from those ones.

Happy to land it early in the 0.0.117 and see if it’s useful for folks using anchor channels. If we think this documentation is more adapted at the spec-level, I can close this one and open a PR in the bolt repository as a recommendation.

@ariard
Copy link
Author

ariard commented Aug 7, 2023

Happy to land it and other ones on timelock before to open more PRs to do sec hardening on the codebase.

@ariard ariard force-pushed the 2023-07-fee-bumping-reserves-doc branch from b8fcd03 to d7fbd10 Compare August 17, 2023 01:40
@ariard
Copy link
Author

ariard commented Aug 17, 2023

Updated at d7fbd10

cc @wpaulino I think you will be interested by the doc, at least to give feedback on how much it’s currently human-readable.

//! Predictive worst-case must be understood as the worst-historical network mempools feerate with an additional
//! safety margin. A factor of 2 is a conservative estimation.
//!
//! The number of opened anchor outputs channels must be selected, i.e `current_opened_channels`. As of 0.0.116
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this sentence. What is the "number of opened anchor outputs channels" exactly referring to, i.e., what does the user need to 'select'?

Copy link
Author

Choose a reason for hiding this comment

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

Updated, the number of opened channel is the number of opened channels under this Lightning node (i.e the outcome of ChannelManager::list_channels()).

//! The number of opened anchor outputs channels must be selected, i.e `current_opened_channels`. As of 0.0.116
//! release, LDK does not have a max number of opened channels enforced by default.
//!
//! The maximum number of HTLC outputs per-commitment transaction on both directions must be selected, i.e
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly here, I'm not sure I understand how the user should act on this advise.

Copy link
Author

Choose a reason for hiding this comment

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

The aim of this documentation is to give the heuristics to the user to understand the amount in satoshis of fee-bumping reserves one should keep, assuming conservative risk exposure (e.g the “A factor of 2 is a conservative estimation”).

This section explains the details of the factors entering in the computation of the max number of pending HTLCs (inbound / outbound) that a channel can have, under default LDK settings.

Note, high-level helpers could be built from the level ones we have currently (get_inbound_pending_htlcs_stats() / get_outbound_pending_htlc_stats()).

//! release, LDK does not have a max number of opened channels enforced by default.
//!
//! The maximum number of HTLC outputs per-commitment transaction on both directions must be selected, i.e
//! `max_htlc_allsides`. As of 0.0.116 release, LDK has a BOLT3's `max_accepted_htlcs` of 50 (`DEFAULT_MAX_HTLCS`)
Copy link
Contributor

Choose a reason for hiding this comment

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

max_htlc_allsides does not exist in this repo, what is it referring to?

Copy link
Author

Choose a reason for hiding this comment

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

See comment just above. In this context, this is used a convenience to yield the level of fee-bumping reserve that a LDK user is recommended to maintain. Specific helpers to compute it could be introduced.

@ariard ariard force-pushed the 2023-07-fee-bumping-reserves-doc branch from d7fbd10 to 031c8e5 Compare September 4, 2023 02:40
@ariard
Copy link
Author

ariard commented Sep 4, 2023

Updated at 031c8e5. Thanks for the review, a lot of comments should have been included.

Current recommendation aims to be simple and waive at issues such as third-party delegated fee-bumping management or future interactions w.r.t changes in transaction-relay policy rules (and as such the dimension of many UTXOs one should keep for a number of opened channels) or the channel types. I think if the main equation and its components are judged sound, helpers to compute them can be introduced.

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

The docs mostly read well to me, though I'm wondering how useful they'll be once we introduce a helper to provide such max_channel_weight_surface to the user. This value is very dependent on the commitment format/channel type so I don't think it's worth providing a breakdown of it other than briefly describing what it covers (commitment transaction + max HTLC transactions).

Comment on lines +37 to +39
//! and the value can be adjusted with the config setting `our_max_accepted_htlcs`. There is no LDK mechanism to
//! limit the number of forward HTLCs (i.e offered HTLC outputs on a commitment transaction). As such `max_htlc_allsides`
//! is `our_max_accepted_htlcs` + `483` (BOLT3 maximum for outgoing HTLCs).
Copy link
Contributor

Choose a reason for hiding this comment

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

The counterparty can also limit us so the 483 is actually dynamic per channel as well.

@ariard
Copy link
Author

ariard commented Oct 17, 2023

For now, no interest in the security maintenance of this codebase.

@ariard ariard closed this Oct 17, 2023
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

Successfully merging this pull request may close these issues.

5 participants