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

htlcswitch/[feature]: dynamically regulate the max anchor fee based on subjective min relay fee and peer's feefilter #8302

Closed
Roasbeef opened this issue Dec 21, 2023 · 3 comments · Fixed by #8418
Assignees
Labels
bitcoind Bitcoin Core backend enhancement Improvements to existing features / behaviour fees Related to the fees paid for transactions (both LN and funding/commitment transactions) mempool P1 MUST be fixed or reviewed
Milestone

Comments

@Roasbeef
Copy link
Member

Is your feature request related to a problem? Please describe.

Today for anchor channels, we have a value max-commit-fee-rate-anchors that regulates the highest fee rate in sat/vbyte that we'll use for our commitment transaction. Under the hood, there's more logic here, but for the most part, users can set this value in order to ensure that their commitment transaction will properly propagate in the case of a force close.

This worked ok in prior times of a calm mempool, but these days, rapid spikes in the mempool can easily render the default of 10 sat/byte much too low to effectively propagate.

One other relevant interaction is that of the feefilter message, specified in BIP 133. Peers send this message to tell other peers that they shouldn't send transactions below a certain fee rate. This fee rate is a function of their current mempool size, and also their configured max mempool size.

Recently, I've seen an affect wherein the min mempool fee of my node is much lower than the fee rate advertised by many of my peers. As a result, even with the default of a 300 MB mempool, my node's mempool is well below that value as due to transitive propagation obstructions caused by the feefilter message, there's a class of transactions my node never actually sees.

This is where a key propagation issue can arise:

  • Today we look at the min mempool fee of our node to determine what the absolute min we need to ensure relay is:
  • However, for a node with default, or even high mempool settings, that doesn't have tens of hundreds of connections, that reported value may actually be insufficient to effectively propagate. To see why, check out the feefilter value of a sample of peers:
/ # bitcoin-cli getpeerinfo | jq '.[].minfeefilter | ((. * 100000000) / 1024)'
0
0
22.0986328125
22.0986328125
22.0986328125
43.064453125
18.263671875
0.9765625000000001
22.0986328125
22.0986328125

Compare that to my min mempool fee: "mempoolminfee": 0.00001000.

Describe the solution you'd like

We should factor the minfeefilter value available in getpeerinfo into the algorithm we choose to set the current anchor commitment fee rate. In the case above, even with a default value of 10 sat/byte as the max, due to the current logic, we're able to get into our mempool, but not our peer's mempool, and therefore not their peer's peer's mempool and so on.

We shouldn't just take these values as is though, to protect against a degree of manipulation we may want to use some smoothing like a moving average sampled periodically.

We should also factor this into the sweeper as well, since any time sensitive sweeps need to be above this value in order to actually propagate.

With this in place, we should be able to get rid of the max-commit-fee-rate-anchors and take the guess work out of running a secure node.

@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour fees Related to the fees paid for transactions (both LN and funding/commitment transactions) bitcoind Bitcoin Core backend P1 MUST be fixed or reviewed mempool labels Dec 21, 2023
@saubyk
Copy link
Collaborator

saubyk commented Jan 3, 2024

Concept Ack and always in favor of removing available configuration options (max-commit-fee-rate-anchors in this case) reducing the cognitive load on the users

@Roasbeef Roasbeef pinned this issue Jan 5, 2024
@djkazic
Copy link
Contributor

djkazic commented Jan 7, 2024

Bit of a mobile user edge case but probably worth mentioning: some users don't have a lot of onchain coins so it might make sense to have a configurable parameter that does a proportional padding of fees (working on the assumption that they don't fee bump the commit tx later).

Else the tx might still be too low in fees (not for propagation necessarily, more about time to conf).

@Crypt-iQ
Copy link
Collaborator

Crypt-iQ commented Jan 8, 2024

We shouldn't just take these values as is though, to protect against a degree of manipulation we may want to use some smoothing like a moving average sampled periodically.

This is a good point. I think that we could weight outbound connections' filters higher than inbound connections' as they are harder to manipulate. We should also ignore filter values of 0 (the peer hasn't sent fee_filter or are on an earlier protocol version) and MAX_MONEY (bitcoin core sends this during IBD).

@saubyk saubyk added this to the v0.18.0 milestone Jan 10, 2024
@saubyk saubyk added this to lnd v0.18 Jan 10, 2024
@saubyk saubyk moved this to 📋 Backlog in lnd v0.18 Jan 10, 2024
@hieblmi hieblmi unpinned this issue Jan 17, 2024
@saubyk saubyk moved this from 📋 Backlog to 🏗 In progress in lnd v0.18 Jan 18, 2024
@saubyk saubyk linked a pull request Jan 25, 2024 that will close this issue
@Roasbeef Roasbeef moved this from 🏗 In progress to 👀 In review in lnd v0.18 Feb 14, 2024
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in lnd v0.18 Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bitcoind Bitcoin Core backend enhancement Improvements to existing features / behaviour fees Related to the fees paid for transactions (both LN and funding/commitment transactions) mempool P1 MUST be fixed or reviewed
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants