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

More flexible fee rate estimates #2660

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

benthecarman
Copy link
Contributor

@benthecarman benthecarman commented Oct 12, 2023

Closes #2659

My attempt at making fee rate estimation more flexible. Names and docs need some work (happy for suggestions).

@TheBlueMatt
Copy link
Collaborator

My one real ask if about keeping Background, Normal, and HighPriority, the way they are used in compute_fee_from_spent_amounts doesn't really seem replaceable, maybe it is fine to keep them with this api.

compute_fee_from_spent_amounts is honestly pretty dumb. It just ladders down ConfirmationTarget, but it oculd, like, just use a lower feerate? Like subtract 1? Divide by two? I don't see why it does what it does, it should just use max(HtlcSweep, input_amounts / 2) or something like that.

@TheBlueMatt TheBlueMatt added this to the 0.0.118 milestone Oct 13, 2023
@TheBlueMatt
Copy link
Collaborator

Tentatively setting 118, but if it doesnt make it it slips.

@benthecarman benthecarman force-pushed the flexible-fee-rate branch 2 times, most recently from 73686c6 to 3e39bcc Compare October 14, 2023 00:39
@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (2c51080) 89.04% compared to head (9baac2f) 88.96%.
Report is 66 commits behind head on main.

❗ Current head 9baac2f differs from pull request most recent head dd15ab0. Consider uploading reports for the commit dd15ab0 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2660      +/-   ##
==========================================
- Coverage   89.04%   88.96%   -0.09%     
==========================================
  Files         112      112              
  Lines       87229    87625     +396     
  Branches    87229    87625     +396     
==========================================
+ Hits        77674    77954     +280     
- Misses       7319     7430     +111     
- Partials     2236     2241       +5     
Files Coverage Δ
lightning/src/chain/chaininterface.rs 97.14% <100.00%> (ø)
lightning/src/chain/onchaintx.rs 80.41% <100.00%> (-1.44%) ⬇️
lightning/src/ln/channelmanager.rs 81.32% <100.00%> (-0.31%) ⬇️
lightning/src/ln/functional_tests.rs 97.17% <100.00%> (-0.14%) ⬇️
lightning/src/util/config.rs 67.87% <ø> (ø)
lightning/src/util/test_utils.rs 77.19% <100.00%> (+0.10%) ⬆️
lightning/src/ln/channel.rs 88.38% <91.66%> (+0.01%) ⬆️
lightning/src/chain/package.rs 87.65% <77.77%> (+1.00%) ⬆️

... and 24 files with indirect coverage changes

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

@benthecarman benthecarman marked this pull request as ready for review October 14, 2023 05:46
@benthecarman benthecarman force-pushed the flexible-fee-rate branch 2 times, most recently from 8257e6f to cbe6adf Compare October 14, 2023 23:13
@benthecarman
Copy link
Contributor Author

Responded to @TheBlueMatt's review

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.

I think all these docs need a good bit more verbiage to make them more friendly to folks newer to lightning, I suggested some alternative text for a few of them.

@benthecarman benthecarman force-pushed the flexible-fee-rate branch 3 times, most recently from 0f11c17 to 8e45e78 Compare October 16, 2023 17:25
@benthecarman
Copy link
Contributor Author

Added some more color around why the min channel fees are important for preventing force closures too

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.

Okay, I think we're getting there. Next time you squash can you line-wrap the lines to 100 chars? Other than that, need another reviewer.

let mut fee = updated_feerate * (predicted_weight as u64) / 1000;
let sweep_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::OnChainSweep);
let fee_rate = cmp::min(sweep_feerate, compute_feerate_sat_per_1000_weight(input_amounts / 2, predicted_weight as u64)) as u64;
let fee = fee_rate * (predicted_weight as u64) / 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use fee_for_weight here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using that breaks a test, it has an extra -1 in it

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably because fee_for_weight will round up. That test isn't very accurate to begin with because it's estimating the witness weight, which can be off by a few units.

@benthecarman
Copy link
Contributor Author

Responded to @wpaulino's review, good catches

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.

Okay, finally there from my end. Some remaining doc nits but I'm happy to land after this.

@TheBlueMatt
Copy link
Collaborator

CI is mad cause there's a test that was checking an error message:

thread 'ln::functional_tests::accept_busted_but_better_fee' panicked at lightning/src/ln/functional_test_utils.rs:1491:17:
assertion `left == right` failed
  left: ProcessingError { err: "Peer's feerate much too low. Actual: 1000. Our expected lower limit: 5000" }
 right: ProcessingError { err: "Peer's feerate much too low. Actual: 1000. Our expected lower limit: 5000 (- 250)" }
stack backtrace:
   0: rust_begin_unwind
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/panicking.rs:595:5
   1: core::panicking::panic_fmt
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/panicking.rs:67:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/panicking.rs:229:5
   4: lightning::ln::functional_test_utils::check_closed_event
             at ./src/ln/functional_test_utils.rs:1491:5
   5: lightning::ln::functional_tests::accept_busted_but_better_fee
             at ./src/ln/functional_tests.rs:10016:4
   6: lightning::ln::functional_tests::accept_busted_but_better_fee::{{closure}}
             at ./src/ln/functional_tests.rs:9948:35
   7: core::ops::function::FnOnce::call_once
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/ops/function.rs:250:5
   8: core::ops::function::FnOnce::call_once
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    ln::functional_tests::accept_busted_but_better_fee

TheBlueMatt
TheBlueMatt previously approved these changes Oct 19, 2023
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.

Happy to clean up the docs in a followup.

@TheBlueMatt TheBlueMatt merged commit be8797e into lightningdevkit:main Oct 20, 2023
@benthecarman benthecarman deleted the flexible-fee-rate branch October 20, 2023 17:48
k0k0ne pushed a commit to bitlightlabs/rust-lightning that referenced this pull request Sep 30, 2024
0.0.118 - Oct 23, 2023 - "Just the Twelve Sinks"

API Updates
===========

 * BOLT12 sending and receiving is now supported as an alpha feature. You may
   run into unexpected issues and will need to have a direct connection with
   the offer's blinded path introduction points as messages are not yet routed.
   We are seeking feedback from early testers (lightningdevkit#2578, lightningdevkit#2039).
 * `ConfirmationTarget` has been rewritten to provide information about the
   specific use LDK needs the feerate estimate for, rather than the generic
   low-, medium-, and high-priority estimates. This allows LDK users to more
   accurately target their feerate estimates (lightningdevkit#2660). For those wishing to
   retain their existing behavior, see the table below for conversion.
 * `ChainHash` is now used in place of `BlockHash` where it represents the
   genesis block (lightningdevkit#2662).
 * `lightning-invoice` payment utilities now take a `Deref` to
   `AChannelManager` (lightningdevkit#2652).
 * `peel_onion` is provided to statelessly decode an `OnionMessage` (lightningdevkit#2599).
 * `ToSocketAddrs` + `Display` are now impl'd for `SocketAddress` (lightningdevkit#2636, lightningdevkit#2670)
 * `Display` is now implemented for `OutPoint` (lightningdevkit#2649).
 * `Features::from_be_bytes` is now provided (lightningdevkit#2640).

For those moving to the new `ConfirmationTarget`, the new variants in terms of
the old mempool/low/medium/high priorities are as follows:
 * `OnChainSweep` = `HighPriority`
 * `MaxAllowedNonAnchorChannelRemoteFee` = `max(25 * 250, HighPriority * 10)`
 * `MinAllowedAnchorChannelRemoteFee` = `MempoolMinimum`
 * `MinAllowedNonAnchorChannelRemoteFee` = `Background - 250`
 * `AnchorChannelFee` = `Background`
 * `NonAnchorChannelFee` = `Normal`
 * `ChannelCloseMinimum` = `Background`

Bug Fixes
=========

 * Calling `ChannelManager::close_channel[_with_feerate_and_script]` on a
   channel which did not exist would immediately hang holding several key
   `ChannelManager`-internal locks (lightningdevkit#2657).
 * Channel information updates received from a failing HTLC are no longer
   applied to our `NetworkGraph`. This prevents a node which we attempted to
   route a payment through from being able to learn the sender of the payment.
   In some rare cases, this may result in marginally reduced payment success
   rates (lightningdevkit#2666).
 * Anchor outputs are now properly considered when calculating the amount
   available to send in HTLCs. This can prevent force-closes in anchor channels
   when sending payments which overflow the available balance (lightningdevkit#2674).
 * A peer that sends an `update_fulfill_htlc` message for a forwarded HTLC,
   then reconnects prior to sending a `commitment_signed` (thus retransmitting
   their `update_fulfill_htlc`) may result in the channel stalling and being
   unable to make progress (lightningdevkit#2661).
 * In exceedingly rare circumstances, messages intended to be sent to a peer
   prior to reconnection can be sent after reconnection. This could result in
   undefined channel state and force-closes (lightningdevkit#2663).

Backwards Compatibility
=======================

 * Creating a blinded path to receive a payment then downgrading to LDK prior to
   0.0.117 may result in failure to receive the payment (lightningdevkit#2413).
 * Calling `ChannelManager::pay_for_offer` or
   `ChannelManager::create_refund_builder` may prevent downgrading to LDK prior
   to 0.0.118 until the payment times out and has been removed (lightningdevkit#2039).

Node Compatibility
==================

 * LDK now sends a bogus `channel_reestablish` message to peers when they ask to
   resume an unknown channel. This should cause LND nodes to force-close and
   broadcast the latest channel state to the chain. In order to trigger this
   when we wish to force-close a channel, LDK now disconnects immediately after
   sending a channel-closing `error` message. This should result in cooperative
   peers also working to confirm the latest commitment transaction when we wish
   to force-close (lightningdevkit#2658).

Security
========

0.0.118 expands mitigations against transaction cycling attacks to non-anchor
channels, though note that no mitigations which exist today are considered robust
to prevent the class of attacks.
 * In order to mitigate against transaction cycling attacks, non-anchor HTLC
   transactions are now properly re-signed before broadcasting (lightningdevkit#2667).

In total, this release features 61 files changed, 3470 insertions, 1503
deletions in 85 commits from 12 authors, in alphabetical order:
 * Antonio Yang
 * Elias Rohrer
 * Evan Feenstra
 * Fedeparma74
 * Gursharan Singh
 * Jeffrey Czyz
 * Matt Corallo
 * Sergi Delgado Segura
 * Vladimir Fomene
 * Wilmer Paulino
 * benthecarman
 * slanesuke
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.

Make Fee Estimator more flexible
4 participants