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

Improve NodeRelayer errors #1261

Merged
merged 4 commits into from
Jan 21, 2020
Merged

Improve NodeRelayer errors #1261

merged 4 commits into from
Jan 21, 2020

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Dec 19, 2019

We add new errors that let senders know they need to raise the trampoline fee/ctlv.
When the error is downstream, we select the best error to forward.

@t-bast t-bast requested review from dpad85 and pm47 December 19, 2019 13:49
@codecov-io
Copy link

codecov-io commented Dec 19, 2019

Codecov Report

Merging #1261 into master will increase coverage by 0.06%.
The diff coverage is 91.22%.

@@            Coverage Diff             @@
##           master    #1261      +/-   ##
==========================================
+ Coverage   77.08%   77.14%   +0.06%     
==========================================
  Files         142      142              
  Lines        9892     9925      +33     
  Branches      394      396       +2     
==========================================
+ Hits         7625     7657      +32     
- Misses       2267     2268       +1
Impacted Files Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 54.02% <0%> (ø) ⬆️
...r/acinq/eclair/payment/send/PaymentLifecycle.scala 92.8% <100%> (ø) ⬆️
...clair/payment/send/MultiPartPaymentLifecycle.scala 98.22% <100%> (ø) ⬆️
...in/scala/fr/acinq/eclair/wire/FailureMessage.scala 76.71% <80%> (-0.1%) ⬇️
...la/fr/acinq/eclair/payment/relay/NodeRelayer.scala 96.77% <92.3%> (-0.82%) ⬇️
...r/acinq/eclair/payment/send/PaymentInitiator.scala 96.29% <94.11%> (-0.85%) ⬇️
...clair/blockchain/electrum/ElectrumClientPool.scala 75.26% <0%> (-3.23%) ⬇️
...src/main/scala/fr/acinq/eclair/router/Router.scala 92% <0%> (-0.72%) ⬇️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 75.59% <0%> (-0.3%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 83.47% <0%> (+0.45%) ⬆️
... and 2 more

Copy link
Member

@pm47 pm47 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 we should immediately merge 2b4b916 and take more time on the NodeRelayer errors.

@t-bast
Copy link
Member Author

t-bast commented Jan 8, 2020

I think we should immediately merge 2b4b916 and take more time on the NodeRelayer errors.

Agreed, I just created #1271

t-bast added 2 commits January 9, 2020 14:25
We add new errors that let senders know they need to raise the trampoline fee/ctlv.
When the error is downstream, we select the best error to forward.
When receiving a fee/cltv error, we should retry with a different fee/cltv.
This process is currently quite manual: the sender decides upfront on
each attempt's fee/cltv.
@t-bast t-bast force-pushed the trampoline-errors branch from 2b4b916 to dc8459e Compare January 9, 2020 13:40
@t-bast
Copy link
Member Author

t-bast commented Jan 9, 2020

I added the sender-side in PaymentInitiator. I hesitated to create a dedicated TrampolinePaymentLifecycle actor, but it felt a bit overkill at the moment.

@@ -72,7 +72,7 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
router ! FinalizeRoute(c.hops)
if (cfg.storeInDb) {
val targetNodeId = cfg.trampolineData.map(_.paymentRequest.nodeId).getOrElse(cfg.targetNodeId)
val finalAmount = c.finalPayload.amount - cfg.trampolineData.map(_.trampolineFees).getOrElse(0 msat)
val finalAmount = c.finalPayload.amount - cfg.trampolineData.map(_.trampolineAttempts.headOption.map(_._1).getOrElse(0 msat)).getOrElse(0 msat)
Copy link
Member Author

Choose a reason for hiding this comment

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

Please ignore this hack. This is because we haven't update the DB schema yet and we needed to take trampoline fees into account in a hackish way. This will be fixed in the next trampoline PR which update the DB schema to clean things up.

@t-bast t-bast requested review from pm47 and dpad85 January 9, 2020 13:43
pm47
pm47 previously approved these changes Jan 21, 2020
@t-bast t-bast merged commit d34d342 into master Jan 21, 2020
@t-bast t-bast deleted the trampoline-errors branch January 21, 2020 16:27
@ACINQ ACINQ deleted a comment from perrystan1489 Oct 21, 2023
@ACINQ ACINQ deleted a comment from 13202916251441262485 Oct 21, 2023
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.

4 participants