Add min mempool estimate for feerate updates on anchor channels #2415
Conversation
| /// 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, |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 ReportPatch coverage:
❗ 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
☔ View full report in Codecov by Sentry. |
| /// 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, |
There was a problem hiding this comment.
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.
|
Note Core’s So I’m not sure how an implementation of Then in the future, we can have |
Mmm, good point, we should probably mention this in our docs.
At least in LDK-sample/ldk-node, I assume we'll basically just use the furthest-out fee estimate a estimator gives us.
I hope we never get there :) If we get package relay we can just return 1sat/vB and be happy. |
If they're running Core, they can just use |
wpaulino
force-pushed
the
update-fee-anchors
branch
from
e136f9c
to
c14e09b
Compare
|
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.
wpaulino
force-pushed
the
update-fee-anchors
branch
from
c14e09b
to
7751cb9
Compare
Yeah see comment suggestion above on how to implement
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 :)
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. |
There was a problem hiding this comment.
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).
|
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 |
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. |
| 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>, |
There was a problem hiding this comment.
Could we make this a member function instead of passing in ChannelTypeFeatures?
There was a problem hiding this comment.
No because we don't have a ChannelContext until much later in InboundV1Channel::new.
| /// 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, |
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. |
|
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 |
|
Lets add cleanups in #2423. |
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.