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

Trampoline payments use per-channel fee and cltv #1853

Merged
merged 2 commits into from
Jul 1, 2021

Conversation

thomash-acinq
Copy link
Member

Make sure trampoline users pay at least as much as the other users.

@thomash-acinq thomash-acinq requested review from t-bast and pm47 and removed request for t-bast June 30, 2021 14:23
@codecov-commenter
Copy link

Codecov Report

Merging #1853 (42dd0db) into master (85ed433) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1853      +/-   ##
==========================================
+ Coverage   88.94%   88.99%   +0.04%     
==========================================
  Files         150      150              
  Lines       11239    11241       +2     
  Branches      447      460      +13     
==========================================
+ Hits         9997    10004       +7     
+ Misses       1242     1237       -5     
Impacted Files Coverage Δ
...src/main/scala/fr/acinq/eclair/router/Router.scala 93.54% <ø> (ø)
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 96.06% <100.00%> (+0.03%) ⬆️
.../src/main/scala/fr/acinq/eclair/router/Graph.scala 98.14% <100.00%> (ø)
...cala/fr/acinq/eclair/router/RouteCalculation.scala 99.33% <100.00%> (+<0.01%) ⬆️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 86.27% <0.00%> (+0.08%) ⬆️
...main/scala/fr/acinq/eclair/router/Validation.scala 92.30% <0.00%> (+1.53%) ⬆️

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Thanks, this is indeed simple enough to include asap ;)

@@ -120,12 +120,13 @@ object NodeRelay {

/** Compute route params that honor our fee and cltv requirements. */
def computeRouteParams(nodeParams: NodeParams, amountIn: MilliSatoshi, expiryIn: CltvExpiry, amountOut: MilliSatoshi, expiryOut: CltvExpiry): RouteParams = {
Copy link
Member

Choose a reason for hiding this comment

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

NB: we still verify in validateRelay above that the fee is at least our current default fee, so we ignore the fact that we may have channels that require lower fees and could relay this payment.

But I think it's the right behavior to have, trampoline is more computationally expensive than standard channel-relay so it makes sense to have a higher threshold than our cheapest channels.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM!

Make sure to choose the "Squash and merge" option when merging, and provide a good commit title and message.

@thomash-acinq thomash-acinq merged commit f857368 into master Jul 1, 2021
@thomash-acinq thomash-acinq deleted the no-more-trampoline-discount branch July 1, 2021 11:32
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.

3 participants