Renepay simpler routetracking #7357
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lagrang3
commented
Jun 1, 2024
| struct route_map *pending_routes; | ||
|
|
||
| /* Routes that have concluded (either SENDPAY_FAILED or | ||
| * SENDPAY_COMPLETE). */ | ||
| struct route **finalized_routes; |
There was a problem hiding this comment.
Note to myself: make sure all this extra data gets cleaned after the payment session concludes.
|
The current payment workflow can be described as this: There is an internal state of the payment called |
Lagrang3
force-pushed
the
renepay-simpler-routetracker
branch
from
0f0c188
to
c5e1a05
Compare
|
Rebased onto |
Lagrang3
force-pushed
the
renepay-simpler-routetracker
branch
from
c5e1a05
to
c02b675
Compare
|
Rebased on top of PR #7337 |
Lagrang3
force-pushed
the
renepay-simpler-routetracker
branch
from
c02b675
to
479ac90
Compare
|
Rebased on top of master, hope to solve conflicts 479ac90. |
|
Rebasing on master again to fix bookeeper... |
- remove payment pointer from routetracker, fetch payment if necessary from payment_hash; - "have results" condition as a function call to routetracker. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Use a single global map of computed routes instead of one for each payment. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
It feels unsafe to rely on the internal state of the plugin's database to tell how many pending sendpays there are for the current payment. The safest way is to assume lightningd knows and thus use listsendpay before computing routes. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
There were some dummy rpc calls to waitblockheight in the payment workflow to allow the function stack to clear. But it is better if some steps in the payment are executed "atomically" to avoid strange race conditions. For example: all steps between getting pending sendpays, computing new routes and sending those routes. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Simply move the "computed routes" array from the payment to the routetracker. It makes sense to put all temporary stages of routing into a single data structure: the routetracker. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Use success_data_from_listsendpays to check if there are "complete" sendpays instead of imposing the presence of "complete" sendpays as a precondition for success_data_from_listsendpays. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Add missing uncertainty update after payment success. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Add remove_htlc in the route destructor. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
We rather have a payment hash table to look up payments based on the payment hash than having a list. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Changelog-Fixed: renepay: finalized routes have to be processed and determine the payment status even after the payment goes into the background (no current command active). Not doing so leads to finalized routes getting stuck in the payment internal data structure and their associated HTLCs in the uncertainty network don't get released. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Remove __PRETTY_FUNCTION__ in favor of __func__ Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
routefail object allocation was linked to route, we had a crash of the plugin with the following error: 0x561f424fc07a send_backtrace common/daemon.c:33 0x561f424fc102 crashdump common/daemon.c:75 0x7f5b0e7dc04f ??? ???:0 0x7f5b0e82ae2c ??? ???:0 0x7f5b0e7dbfb1 ??? ???:0 0x7f5b0e7c6471 ??? ???:0 0x561f4252581f call_error ccan/ccan/tal/tal.c:95 0x561f425258c8 check_bounds ccan/ccan/tal/tal.c:169 0x561f425258f9 to_tal_hdr ccan/ccan/tal/tal.c:179 0x561f42526283 tal_free ccan/ccan/tal/tal.c:525 0x561f424e5379 routefail_end plugins/renepay/routefail.c:52 0x561f424e557b handle_failure plugins/renepay/routefail.c:431 apparently there was a race condition for which the route was first freed before we arrived to routefail_end where we manually free routefail. I don't see how this could have happened, but anyways this subtle bug can be avoided by linking the routefail to the payment. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Often I find this in the logs: DEBUG plugin-cln-renepay: gossmap ignored 94692259263600 channel updates Apparently gossmap_refresh does not initialize the input variable num_channel_updates_rejected. Fix this by explicitely initializing it before calling gossmap_refresh. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
rustyrussell
force-pushed
the
renepay-simpler-routetracker
branch
from
479ac90
to
3319f80
Compare
|
ACK 3319f80 |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While fixing the bug #7337 I have realized that the mechanism for tracking pending routes internally in renepay is way too complicated, prone to bugs.
With this PR I wan to reduce that complexity by relying in the internal state as little as possible and using
lightningd as oracle. The tradeoff is that now there is an extra rpc call
listsendpaysfor every route computation cycle;this IO communication is way more expensive that checking the internal database. However the bottleneck of the payment
is the time spent waiting for onion messages from peers, not RPC communication with ligthningd and neither the computation
of the routes.