Add min mempool estimate for feerate updates on anchor channels #2415

wpaulino · 2023-07-13T23:21:27Z

Channels supporting anchors outputs no longer require their feerate updates to target a prompt confirmation since commitment transactions can be bumped when broadcasting. Commitment transactions must now at least meet the minimum mempool feerate, until package relay is deployed, such that they can propagate across node mempools in the network by themselves.

TheBlueMatt · 2023-07-14T00:09:58Z

lightning/src/chain/chaininterface.rs

 /// estimation.
 #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
 pub enum ConfirmationTarget {
-	/// We are happy with this transaction confirming slowly when feerate drops some.
+	/// We are happy with a transaction eventually confirming, regardless of how long it may take,


I don't think we should phrase this in terms of "transaction confirming", but rather state explicitly that we will use CPFP to bump the feerate of this transaction, and that the feerate here should be, roughly, what we expect to be the maximum mempool-min-fee over the course of the coming year or so, noting that our peers are often dumb and will send us 1sat/vb and we have to accept that (or force-close).

I think to clarify the discussion, it should be precised which mempool min fee MempoolMinimum aims to represent. As a recall you have two mempool min fees in Bitcoin Core:

mempool min fee

min relay fee

The first one is dynamic and based on the size of your mempool and the current received transactions (see GetMinFee in txmempool.cpp).

The second one is static and it can be configured by the node operator (see m_min_relay_feerate in txmempool.cpp)

Our pre-signed transactions should respect both at all time, at least until bip331 + nversion=3 are widely deployed on the network, which might take one or two Core release cycle.

What is unclear in my understanding is how to evaluate the “maximum mempool-min-fee over the course of the coming year or so” as we’re trying to predict future blockspace demand by all the Bitcoin users. If we hardcode some value and we deploy LDK software, and we got wrong our prediction, in my understanding it would mean all the deployed nodes are vulnerable to loss of funds (as the pre-signed commitment with MempoolMinimum won’t satisfy Core’s GetMinFee check in CheckFeeRate.

What is unclear in my understanding is how to evaluate the “maximum mempool-min-fee over the course of the coming year or so” as we’re trying to predict future blockspace demand by all the Bitcoin users.

Like everything fee-related in lightning, until we have package relay its simply not possible to be "secure". We should communicate that to our users here, and maybe also note that once the user is using package relay, they should always return 253 for this value.

and that the feerate here should be, roughly, what we expect to be the maximum mempool-min-fee over the course of the coming year or so

Assuming they're not returning some fixed value (which they likely shouldn't yet) is this really necessary? The frequency of update_fee is unchanged, so we should be able to detect any feerate fluctuations.

Like everything fee-related in lightning, until we have package relay its simply not possible to be "secure". We should communicate that to our users here, and maybe also note that once the user is using package relay,

Yes this is correct it cannot be “secure” until we have package relay. The more intermediate solution would be to scale our update_fee frequency on the changes of mempool min fee announced to us by the local mempool, it’s still a “security” advance. Though clearly conversation for another issue / PR.

Assuming they're not returning some fixed value (which they likely shouldn't yet) is this really necessary? The frequency of update_fee is unchanged, so we should be able to detect any feerate fluctuations.

Sure if they’re using fixed value it’s okay though at commit 7751cb90 MempoolMinimum documentation could be clear on that concern, what do you think of the following, I think this is the most accurate we can say until wide deployment of package relay (might take another couple of years)

diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs index 4204d714..b5c05226 100644 --- a/lightning/src/chain/chaininterface.rs +++ b/lightning/src/chain/chaininterface.rs @@ -44,8 +44,19 @@ pub enum ConfirmationTarget { /// bump of the transaction. /// /// The feerate returned should be the absolute minimum feerate required to enter most node - /// mempools across the network. Note that if you are not able to obtain this feerate estimate, - /// you should likely use the furthest-out estimate allowed by your fee estimator. + /// mempools across the network. A good implementation can be to poll frequently (e.g every minute) + /// Bitcoin Core's `getmempoolinfo` and consume the `mempoolminfee` min fee, as such guaranteeing + /// this value reflect roughly real-time network mempools congestion. + /// + /// In the lack of access to a local mempool, this feerate estimate can be fetched from (ideally) + /// a set of third-party hosting local mempools, however this means those third-parties can + /// salvage the propagation of your pre-signed transactions at anytime and therefore endangers + /// the safety of channels funds. It SHOULD be considered carefully as a deployment. + /// + /// A third alternative is to use a furthest-out estimate based on historical worst-case seen + /// mempools feerate, and multiply this historical worst in function of the safety margin you + /// wish. Failure in the further-out prediction renders the propagation of your pre-signed + /// transaction ineffective and therefore endangers the safety of channels funds too. MempoolMinimum, /// We are happy with a transaction confirming slowly, at least within a day or so worth of /// blocks.

I don't think we should advise users to poll once a minute, lightning feerates can't be updated regularly, and the problem is all the old states also need to be able to get into the mempool too, not just the latest one, so polling more often doesn't help anything.

The advice about using a remote fee estimator putting you at risk is good, but not specific to the new variant, if we want to include that it should go on the overall enum docs, not just the mempool-min-fee variant.

I don't think we should really be giving advice to use getmempoolinfo's mempoolminfee, honestly, or even telling the user to always use the current min-fee, rather if we're trying to be pedantic here we should consider writing something about future min-fees.

If I remember correctly the spec says nothing about the frequency of the update_fee, or at least nothing in a mandatory way, otherwise it would be bad as you have to synchronize clocks, including being based on blocks. Polling does help for the negotiation of yet-to-come state, where yet-to-come state might be defined simply by your counterparty sending update_add_htlc / update_fulfill_htlc at anytime, without synchronicity on network mempools congestion.

The advice about remote fee estimator starts to be spread multiple times across the feedback, e.g about dust exposure, so yes it could be unified for FeeEstimator, and the other hand it might be better to have the critical documentation nearer from where the user are interacting with a feature / variables.

If I had a reliable methodology that one can implement in code about predicting future min-fees and that one can resume in 4 lines of documentation, well I would write them :) To be honest that type of discussions belong more to #2404, it’s a nice thing we can have on top of it.

For this PR, I think an effective documentation might be just to give a hardcoded min-fee based on the last two years of data, with a x2 or x3 (or more ?) safety margin with a suggestion to update it as each release.

Sadly we use this canoe both for our own channels and to validate peers, so we generally can't add a few x multiplier. We might, however, want to mention that this value has a dual use in the documentation. As for refresh time, note that users can update their fee as often as they want - it doesn't result in an update_fee message :)

Sadly we use this canoe both for our own channels and to validate peers, so we generally can't add a few x multiplier.

Changes nothing as here we wanna to validate the sanity of the mempool min fee, whoever is responsible of the update_fee. Of course if your counterparty announces something worst than the multiplier we should reject it, as it means our future min fees prediction are broken. And here we have to consider what we accept as a worst-case frequency from counterparty’s update_fee (4 source of logic that can influence on update validity, holder’s mempool/feeestimator, holder’s update_fee frequency, counterparty’s mempool/feestimator, counterparty’s update_fee frequency).

That's great in theory, in practice people want their channels to stay open, and we should explain what the implications of various API implementation decisions would be.

codecov-commenter · 2023-07-14T00:47:26Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02 ⚠️

Comparison is base (07606c1) 90.26% compared to head (c14e09b) 90.24%.

❗ 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    #2415      +/-   ##
==========================================
- Coverage   90.26%   90.24%   -0.02%     
==========================================
  Files         106      106              
  Lines       55188    55449     +261     
  Branches    55188    55449     +261     
==========================================
+ Hits        49815    50040     +225     
- Misses       5373     5409      +36

Impacted Files	Coverage Δ
lightning/src/chain/chaininterface.rs	`95.23% <ø> (ø)`
lightning/src/ln/channel.rs	`89.47% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.74% <100.00%> (-0.21%)`	⬇️

... and 66 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

lightning/src/ln/channel.rs

ariard · 2023-07-14T01:51:41Z

lightning/src/chain/chaininterface.rs

 /// estimation.
 #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
 pub enum ConfirmationTarget {
-	/// We are happy with this transaction confirming slowly when feerate drops some.
+	/// We are happy with a transaction eventually confirming, regardless of how long it may take,


I think to clarify the discussion, it should be precised which mempool min fee MempoolMinimum aims to represent. As a recall you have two mempool min fees in Bitcoin Core:

mempool min fee

min relay fee

The first one is dynamic and based on the size of your mempool and the current received transactions (see GetMinFee in txmempool.cpp).

The second one is static and it can be configured by the node operator (see m_min_relay_feerate in txmempool.cpp)

Our pre-signed transactions should respect both at all time, at least until bip331 + nversion=3 are widely deployed on the network, which might take one or two Core release cycle.

What is unclear in my understanding is how to evaluate the “maximum mempool-min-fee over the course of the coming year or so” as we’re trying to predict future blockspace demand by all the Bitcoin users. If we hardcode some value and we deploy LDK software, and we got wrong our prediction, in my understanding it would mean all the deployed nodes are vulnerable to loss of funds (as the pre-signed commitment with MempoolMinimum won’t satisfy Core’s GetMinFee check in CheckFeeRate.

ariard · 2023-07-14T02:10:28Z

Note Core’s estimaterawfee always ensure that feerate returns for a confirmation target in conf_target block is above both mempool dynamic min fee and mempool tx relay min fee: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/fees.cpp#L88

So I’m not sure how an implementation of MempoolMininum should proceed on our side (assuming the fee estimator used is Core’s one). Best sounds to me to modify Core’s estimaterawfee to yield back mempool min fee (independently of a confirmation target which might never for pre-signed transaction). I’m fine to do it, it’s a 2 lines of cpp code on the Core side.

Then in the future, we can have update_fee based on network mempools evolution. This will be a saving in term of fee-bumping reserve as value can be withdraw from the channel liquidity and a probabilistic saving in case of effective broadcast if you don’t have to pay the witness cost of fee-bumping inputs.

TheBlueMatt · 2023-07-14T02:39:44Z

Note Core’s estimaterawfee always ensure that feerate returns for a confirmation target in conf_target block is above both mempool dynamic min fee and mempool tx relay min fee: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/fees.cpp#L88

Mmm, good point, we should probably mention this in our docs.

So I’m not sure how an implementation of MempoolMininum should proceed on our side (assuming the fee estimator used is Core’s one).

At least in LDK-sample/ldk-node, I assume we'll basically just use the furthest-out fee estimate a estimator gives us.

Then in the future, we can have update_fee based on network mempools evolution.

I hope we never get there :) If we get package relay we can just return 1sat/vB and be happy.

lightning/src/chain/chaininterface.rs

wpaulino · 2023-07-14T17:01:42Z

Note Core’s estimaterawfee always ensure that feerate returns for a confirmation target in conf_target block is above both mempool dynamic min fee and mempool tx relay min fee: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/fees.cpp#L88

So I’m not sure how an implementation of MempoolMininum should proceed on our side (assuming the fee estimator used is Core’s one). Best sounds to me to modify Core’s estimaterawfee to yield back mempool min fee (independently of a confirmation target which might never for pre-signed transaction). I’m fine to do it, it’s a 2 lines of cpp code on the Core side.

If they're running Core, they can just use mempoolminfee from the getmempoolinfo RPC.

TheBlueMatt · 2023-07-14T21:06:36Z

LGTM, feel free to squash.

Now that we support channels with anchor outputs, we add a new ConfirmationTarget variant that, for now, will only apply to such channels. This new variant should target estimating the minimum feerate required to be accepted into most node mempools across the network.

Channels supporting anchors outputs no longer require their feerate updates to target a prompt confirmation since commitment transactions can be bumped when broadcasting. Commitment transactions must now at least meet the minimum mempool feerate, until package relay is deployed, such that they can propagate across node mempools in the network by themselves. The existing higher bound no longer applies to channels supporting anchor outputs since their HTLC transactions don't have any fees committed, which directly impact the available balance users can send.

As done with inbound feerate updates, we can afford to commit less in fees, as long as we still may the minimum mempool feerate. This enables users to spend a bit more of their balance, as less funds are being committed to transaction fees.

ariard · 2023-07-15T00:42:05Z

At least in LDK-sample/ldk-node, I assume we'll basically just use the furthest-out fee estimate a estimator gives us.

Yeah see comment suggestion above on how to implement MempoolMininum.

I hope we never get there :) If we get package relay we can just return 1sat/vB and be happy.

Don’t be sure of that in a world where fee-bumping reserves are limited and blockspace demand unbounded: lightning/bolts#845, though yeah more spec-level conversation :)

If they're running Core, they can just use mempoolminfee from the getmempoolinfo RPC.

Yeah it’s the best of mempool min relay tx fee and mempool min fee, which can both be falsified by config settings like mempool max size, or even by the user for the former. Ideally it would be more mempool min fee based on historical feerate buckets, though this is clearly conversation for another day.

ariard

Code Review ACK 7751cb9 modulo the two suggestions to improve docs.

LDK is a library so always good to make it harder to misuse for our users (at the very least I can see ldk-node to have options to use a set of third-party fee-estimators like my doc suggestion is saying you can).

ariard · 2023-07-15T04:08:06Z

Anyway, I’ll let Wilmer take what can make the documentation better, I think I’ve pointed enough source of confusion for LDK users in terms of what fee estimation and mempool min fee, my code ack is already there. Still appreciated if comments can be integrated as it help future review work by other people less familiar with those part of the codebase, I think.

Generally, I think all those discussions from the fact than LDK is a library and therefore there is no authorative fee estimation or mempool backend, no even full-node like CLN or LND have. Therefore the number of ways a user can screw up it’s integration is quite wide. And I must say we are not help by the clusterfuck than Core’s policy rules is currently.

Overall, discussion is beyond the scope of this PR, glad we have ldk-node starting to be more mature to simplify API :)

jkczyz · 2023-07-17T18:30:17Z

I hope we never get there :) If we get package relay we can just return 1sat/vB and be happy.

Instead of adding another variant that will ultimately get removed in a package relay world, should we simply add another method to the trait that can be later deprecated? At least that will make implementors think about what should be returned in case they are either returning a fixed or catch-all value.

jkczyz · 2023-07-17T18:16:39Z

lightning/src/ln/channel.rs

-		feerate_per_kw: u32, cur_feerate_per_kw: Option<u32>, logger: &L)
-		-> Result<(), ChannelError> where F::Target: FeeEstimator, L::Target: Logger,
+	fn check_remote_fee<F: Deref, L: Deref>(
+		channel_type: &ChannelTypeFeatures, fee_estimator: &LowerBoundedFeeEstimator<F>,


Could we make this a member function instead of passing in ChannelTypeFeatures?

No because we don't have a ChannelContext until much later in InboundV1Channel::new.

jkczyz · 2023-07-17T18:30:55Z

lightning/src/chain/chaininterface.rs

+	/// bump of the transaction.
+	///
+	/// The feerate returned should be the absolute minimum feerate required to enter most node
+	/// mempools across the network. Note that if you are not able to obtain this feerate estimate,


nit: s/not able/unable

wpaulino · 2023-07-17T18:45:49Z

Instead of adding another variant that will ultimately get removed in a package relay world, should we simply add another method to the trait that can be later deprecated? At least that will make implementors think about what should be returned in case they are either returning a fixed or catch-all value.

We generally just mark things deprecated with a comment since the attribute results in build warnings, so not much would change there. I agree though that the new method would force implementors to think about this a bit more over the enum variant, which could even be ignored if they're using a catch-all match.

TheBlueMatt · 2023-07-17T19:10:56Z

I really hope we don't have users doing a general match on the feerates and returning some default value always, but if we do they're gonna suffer channel closures and that's on them :). Regarding using a new method, that seems like overkill - once package relay is broadly supported I figure we'll just drop this variant and update_fee entirely, and before then (whenever then is...we've been waiting a few years already) why add the API complexity?

TheBlueMatt · 2023-07-17T19:45:47Z

Lets add cleanups in #2423.

wpaulino added the Anchor Support label Jul 13, 2023

wpaulino added this to the 0.0.116 milestone Jul 13, 2023

TheBlueMatt reviewed Jul 14, 2023

View reviewed changes

ariard reviewed Jul 14, 2023

View reviewed changes

alecchendev reviewed Jul 14, 2023

View reviewed changes

lightning/src/chain/chaininterface.rs Show resolved Hide resolved

wpaulino force-pushed the update-fee-anchors branch from e136f9c to c14e09b Compare July 14, 2023 17:16

wpaulino added 3 commits July 14, 2023 14:49

wpaulino force-pushed the update-fee-anchors branch from c14e09b to 7751cb9 Compare July 14, 2023 21:50

TheBlueMatt approved these changes Jul 14, 2023

View reviewed changes

ariard reviewed Jul 15, 2023

View reviewed changes

jkczyz reviewed Jul 17, 2023

View reviewed changes

jkczyz approved these changes Jul 17, 2023

View reviewed changes

TheBlueMatt merged commit baf9731 into lightningdevkit:main Jul 17, 2023

wpaulino deleted the update-fee-anchors branch July 17, 2023 20:19

wpaulino mentioned this pull request Jul 17, 2023

Anchor Outputs Tracking Issue #2297

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add min mempool estimate for feerate updates on anchor channels #2415

Add min mempool estimate for feerate updates on anchor channels #2415

wpaulino commented Jul 13, 2023

TheBlueMatt Jul 14, 2023 •

edited

Loading

ariard Jul 14, 2023

TheBlueMatt Jul 14, 2023

wpaulino Jul 14, 2023

ariard Jul 15, 2023

TheBlueMatt Jul 15, 2023

ariard Jul 15, 2023

TheBlueMatt Jul 15, 2023

ariard Jul 15, 2023

TheBlueMatt Jul 15, 2023

codecov-commenter commented Jul 14, 2023 •

edited

Loading

ariard Jul 14, 2023

ariard commented Jul 14, 2023

TheBlueMatt commented Jul 14, 2023

wpaulino commented Jul 14, 2023

TheBlueMatt commented Jul 14, 2023

ariard commented Jul 15, 2023

ariard left a comment

ariard commented Jul 15, 2023

jkczyz commented Jul 17, 2023

jkczyz Jul 17, 2023

wpaulino Jul 17, 2023

jkczyz Jul 17, 2023

wpaulino commented Jul 17, 2023

TheBlueMatt commented Jul 17, 2023

TheBlueMatt commented Jul 17, 2023

Add min mempool estimate for feerate updates on anchor channels #2415

Add min mempool estimate for feerate updates on anchor channels #2415

Conversation

wpaulino commented Jul 13, 2023

TheBlueMatt Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 14, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ariard commented Jul 14, 2023

TheBlueMatt commented Jul 14, 2023

wpaulino commented Jul 14, 2023

TheBlueMatt commented Jul 14, 2023

ariard commented Jul 15, 2023

ariard left a comment

Choose a reason for hiding this comment

ariard commented Jul 15, 2023

jkczyz commented Jul 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino commented Jul 17, 2023

TheBlueMatt commented Jul 17, 2023

TheBlueMatt commented Jul 17, 2023

TheBlueMatt Jul 14, 2023 •

edited

Loading

codecov-commenter commented Jul 14, 2023 •

edited

Loading