Additional parameters for findroute* API calls #1969

rorp · 2021-09-26T22:41:58Z

This PR adds new parameters for findroute* API calls useful automated route calculations for third-party applications:

ignoreNodeIds - specifies ignored nodes
ignoreChannelIds - specifies ignored channels
maxFeeMsat - sets max fee for route calculations

Also it adds full format that returns all information that Router can provide about the found routes.

codecov-commenter · 2021-09-26T22:57:01Z

Codecov Report

Merging #1969 (abeac70) into master (6b202c3) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1969      +/-   ##
==========================================
- Coverage   87.77%   87.77%   -0.01%     
==========================================
  Files         161      161              
  Lines       12526    12534       +8     
  Branches      539      535       -4     
==========================================
+ Hits        10995    11002       +7     
- Misses       1531     1532       +1

Impacted Files	Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala	`40.25% <0.00%> (-2.21%)`	⬇️
...n/scala/fr/acinq/eclair/json/JsonSerializers.scala	`87.09% <ø> (ø)`
...main/scala/fr/acinq/eclair/io/PeerConnection.scala	`84.16% <0.00%> (-0.42%)`	⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala	`88.91% <0.00%> (ø)`
...clair/channel/publish/ReplaceableTxPublisher.scala	`87.35% <0.00%> (+1.14%)`	⬆️
...main/scala/fr/acinq/eclair/router/Validation.scala	`92.30% <0.00%> (+1.53%)`	⬆️
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala	`97.56% <0.00%> (+1.62%)`	⬆️

t-bast

Can you update the release notes as well since this PR updates some APIs?

And please post detailed results of how you manage to use this for rebalancing.

eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala

eclair-core/src/main/scala/fr/acinq/eclair/json/JsonSerializers.scala

eclair-node/src/main/scala/fr/acinq/eclair/api/directives/RouteFormat.scala

eclair-node/src/test/scala/fr/acinq/eclair/api/ApiServiceSpec.scala

rorp · 2021-10-01T02:37:00Z

Can you update the release notes as well since this PR updates some APIs?

I don't know how to it

rorp · 2021-10-01T03:27:24Z

And please post detailed results of how you manage to use this for rebalancing.

        ignore_node_ids = [self.get_own_pubkey()]

        if isinstance(ignored_nodes, list):
            ignore_node_ids = ignore_node_ids + ignored_nodes

        params = {
            'sourceNodeId': first_hop_pubkey,
            'targetNodeId': last_hop_pubkey,
            'amountMsat': int(amount * 1000),
            'format': 'full',
            'ignoreNodeIds': ignore_node_ids,
            'ignoreChannelIds': ignore_channel_ids,
            'maxFeeMsat': fee_limit,
        }
        found_routes = self.call_eclair("findroutebetweennodes", params)
        if isinstance(found_routes, list):
            routes = []
            for found_route in found_routes:
                amount_msat = found_route['amount']
                hops = [self.route_to_hop(hop, amount_msat) for hop in found_route['hops']]
                if first_hop_channel:
                    hops.insert(0, first_hop_channel.to_hop(amount_msat, 0, first=True))
                if last_hop_channel:
                    hops.append(
                        last_hop_channel.to_hop(amount_msat, self.calc_fees_msat(amount_msat, last_hop_channel.chan_id),
                                                first=False))
                routes.append(Route(amount_msat, hops))
            return routes

t-bast · 2021-10-01T06:28:45Z

I don't know how to it

Simply update this section: https://github.com/ACINQ/eclair/blob/master/docs/release-notes/eclair-vnext.md#api-changes

t-bast

Concept ACK, we should integrate this soon (we're planning a release and it would be nice to have this PR be part of it).

Two things to consider:

Json serializers refactoring #1979 will be merged soon and provides more flexibility for JSON serialization: I think you can leverage this to more cleanly apply the right serialization based on the requested route format
Ignore redundant channels #1974 will change the internal logic of ignoring channels. When you ask to ignore a channel between Bob and Carol, we will ignore ALL channels between them (because non-strict forwarding means that if one channel cannot be used, none of them can unless the node is very buggy). The only exception is obviously for local channels. IIUC for rebalancing you only need to use this with local channels, right? If that's the case it should work properly.

Regarding release notes, it would be great if you could add a new "Circular rebalancing" section to https://github.com/ACINQ/eclair/blob/master/docs/release-notes/eclair-vnext.md where you explain how the eclair API can be used to rebalance your channels. You should describe the example of a triangular topology and show the concrete eclair-cli commands that should be used.

eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala

eclair-node/src/test/scala/fr/acinq/eclair/api/ApiServiceSpec.scala

rorp · 2021-10-04T03:47:41Z

Regarding release notes, it would be great if you could add a new "Circular rebalancing" section to https://github.com/ACINQ/eclair/blob/master/docs/release-notes/eclair-vnext.md where you explain how the eclair API can be used to rebalance your channels. You should describe the example of a triangular topology and show the concrete eclair-cli commands that should be used.

I think it should be in a separate doc

t-bast · 2021-10-04T07:18:52Z

I think it should be in a separate doc

Good idea, a separate doc would be very useful.

viaj3ro · 2021-10-14T08:24:13Z

Is this ready to merge after review? It's desperately needed! My nodes routing activity and fee earnings have dropped more than 90% since LND started routing around nodes with unbalanced channels.

t-bast · 2021-10-18T09:57:06Z

Is this ready to merge after review? It's desperately needed! My nodes routing activity and fee earnings have dropped more than 90% since LND started routing around nodes with unbalanced channels.

FWIW, @Roasbeef confirmed that this behavior is a bug on their side, which is being fixed in their next release, so it is not a deliberate attempt at overly penalizing your node when you have unbalanced channels with one specific node.

We are making progress on this PR and the other rebalancing-related PRs though, don't worry this will land soon.

t-bast

Thanks @rorp!

I'd just like to see a parameter renamed and serialization re-worked, and we should be good to go.

docs/release-notes/eclair-vnext.md

eclair-node/src/main/scala/fr/acinq/eclair/api/directives/ExtraDirectives.scala

t-bast · 2021-10-18T10:14:14Z

eclair-core/src/main/scala/fr/acinq/eclair/json/JsonSerializers.scala

+object RouteSerializer extends MinimalSerializer ({
+  case route: Route =>
+    Extraction.decompose(route)(DefaultFormats +
+      ByteVector32Serializer +
+      ByteVectorSerializer +
+      PublicKeySerializer +
+      ShortChannelIdSerializer +
+      MilliSatoshiSerializer +
+      CltvExpiryDeltaSerializer)


I think we should remove this entirely, and rely on the new mechanism with private case classes which gives you more flexibility.
You should define three private case classes:

private case class RouteNodeIdsJson(nodeIds: Seq[PublicKey]) private case class RouteShortChannelIdsJson(shortChannelIds: Seq[ShortChannelId]) private case class RouteFullJson(<whatever you need in the "full" format>)

And the serializers to convert from a Route to these new private case classes. You should add only the RouteNodeIdsJson serializer to the default formats (to preserve as a default behavior listing the node ids only).

In RouteFormat.scala, you should inject the route serializer you want to use based on what format was requested.

Does that make sense?

Haha fair enough, I'll prototype it and send you the corresponding commit

See #1969 (review)

docs/release-notes/eclair-vnext.md

eclair-node/src/test/scala/fr/acinq/eclair/api/ApiServiceSpec.scala

…w_findroute_params

t-bast

I reworked the route serializers in 6d40b0d, please apply that commit, test it with your rebalancing flow and rebase and we should be good to go.

…' into new_findroute_params

t-bast

LGTM, just needs a last merge for the release notes conflict and we should be good to go.

viaj3ro · 2021-10-23T05:20:13Z

FWIW, @Roasbeef confirmed that this behavior is a bug on their side, which is being fixed in their next release, so it is not a deliberate attempt at overly penalizing your node when you have unbalanced channels with one specific node.

That's good to know! And also thanks a lot to the both of you for finishing this so quickly! Very much appreciated!

Additional parameters for findroute* API calls

4b6fc32

rorp mentioned this pull request Sep 26, 2021

Add firstHopChannel/lastHopChannel parameters to findroute* API calls #1950

Closed

t-bast reviewed Sep 29, 2021

View reviewed changes

rorp added 3 commits September 30, 2021 19:57

fix tests

ac52380

Rename the method parameters

019e864

Merge branch 'master' into new_findroute_params

ae61615

fix build

6a0b177

t-bast reviewed Oct 1, 2021

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala Show resolved Hide resolved

eclair-node/src/test/scala/fr/acinq/eclair/api/ApiServiceSpec.scala Outdated Show resolved Hide resolved

rorp added 2 commits October 3, 2021 11:15

update release notes

35ce3f0

cleanup

d31e6d2

rorp added 4 commits October 6, 2021 09:55

Merge branch 'master' into new_findroute_params

68b0e60

add docs

d65db5d

typo

c2e9181

Merge branch 'master' into new_findroute_params

0e6f10e

Merge branch 'master' into new_findroute_params

c926af5

t-bast reviewed Oct 18, 2021

View reviewed changes

rorp and others added 3 commits October 19, 2021 09:46

address the comments

3030ce9

Merge branch 'new_findroute_params' of github.com:rorp/eclair into ne…

4cdc0a3

…w_findroute_params

Rework route serializers

6d40b0d

t-bast reviewed Oct 20, 2021

View reviewed changes

rorp added 2 commits October 20, 2021 11:24

Merge branch 'master' into new_findroute_params

b02c0b7

Merge remote-tracking branch 'upstream/route-format-json-improvements…

ae6ac55

…' into new_findroute_params

Merge branch 'master' into new_findroute_params

abeac70

t-bast reviewed Oct 21, 2021

View reviewed changes

Merge branch 'master' into new_findroute_params

efeacc8

t-bast approved these changes Oct 22, 2021

View reviewed changes

t-bast merged commit bdef833 into ACINQ:master Oct 22, 2021

t-bast mentioned this pull request Oct 22, 2021

Circular rebalancing #1591

Closed

ShahanaFarooqui mentioned this pull request Oct 22, 2021

Eclair Channel Rebalance Ride-The-Lightning/RTL#842

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional parameters for findroute* API calls #1969

Additional parameters for findroute* API calls #1969

rorp commented Sep 26, 2021

codecov-commenter commented Sep 26, 2021 •

edited

Loading

t-bast left a comment

rorp commented Oct 1, 2021

rorp commented Oct 1, 2021

t-bast commented Oct 1, 2021

t-bast left a comment

rorp commented Oct 4, 2021

t-bast commented Oct 4, 2021

viaj3ro commented Oct 14, 2021

t-bast commented Oct 18, 2021

t-bast left a comment

t-bast Oct 18, 2021

rorp Oct 19, 2021

t-bast Oct 19, 2021

t-bast Oct 20, 2021

t-bast left a comment

t-bast left a comment

viaj3ro commented Oct 23, 2021

Additional parameters for findroute* API calls #1969

Additional parameters for findroute* API calls #1969

Conversation

rorp commented Sep 26, 2021

codecov-commenter commented Sep 26, 2021 • edited Loading

Codecov Report

t-bast left a comment

Choose a reason for hiding this comment

rorp commented Oct 1, 2021

rorp commented Oct 1, 2021

t-bast commented Oct 1, 2021

t-bast left a comment

Choose a reason for hiding this comment

rorp commented Oct 4, 2021

t-bast commented Oct 4, 2021

viaj3ro commented Oct 14, 2021

t-bast commented Oct 18, 2021

t-bast left a comment

Choose a reason for hiding this comment

t-bast Oct 18, 2021

Choose a reason for hiding this comment

rorp Oct 19, 2021

Choose a reason for hiding this comment

t-bast Oct 19, 2021

Choose a reason for hiding this comment

t-bast Oct 20, 2021

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

viaj3ro commented Oct 23, 2021

codecov-commenter commented Sep 26, 2021 •

edited

Loading