Skip to content

routing: add mission control import rpc#5061

Merged
Roasbeef merged 3 commits into
lightningnetwork:masterfrom
carlaKC:4483-importmissioncontrol
Mar 23, 2021
Merged

routing: add mission control import rpc#5061
Roasbeef merged 3 commits into
lightningnetwork:masterfrom
carlaKC:4483-importmissioncontrol

Conversation

@carlaKC
Copy link
Copy Markdown
Collaborator

@carlaKC carlaKC commented Mar 2, 2021

This PR adds in-memory mission control import. We take a very simple approach, as outlined in the original issue, where we only import results that are fresher than our current records.

Fixes #4833

Copy link
Copy Markdown
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

Nice feature!! Just a few nits on logging

Fixes #4483

Looks like this might be the wrong issue? Think we want #4833

Comment thread routing/missioncontrol_state.go Outdated
Comment thread routing/missioncontrol.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: maybe. bump this to Infof? would also make a nice addition to log how many were added/ignored! that way info users only see:

[CRTR] Importing history snapshot to mission control
[CRTR] Imported XX/YY fresh results from mission control history snapshot.

@bhandras bhandras self-requested a review March 2, 2021 20:08
Copy link
Copy Markdown
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Nice feature, mostly nits. 🚀

Comment thread lnrpc/routerrpc/router_server.go Outdated
Comment thread lnrpc/routerrpc/router_server.go Outdated
Comment thread lnrpc/routerrpc/router_server.go Outdated
Comment thread routing/missioncontrol_state.go Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why wouldn't we want to simply overwrite? I see it perfectly valid to just overwrite with an old dump. WDYT?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why would we want to overwrite our existing fresh results with older results? Since the goal in doing this is to get a hotter cache

Comment thread cmd/lncli/cmd_import_mission_control.go Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is very nice, maybe could also add csv import? (Otherwise MC may change if node is busy).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

What do you by MC might change?

I was picturing the main use case for bulk-import to be the rpc? Since you can pass the request from QueryMissionControl directly in, adding a csv is an extra step. Could add an option to feed the json output in like we do for SendToRoute if we want to do bulk import via cli?

@Roasbeef Roasbeef added this to the 0.13.0 milestone Mar 3, 2021
@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour mission control P1 MUST be fixed or reviewed labels Mar 3, 2021
@carlaKC carlaKC force-pushed the 4483-importmissioncontrol branch from 79552ac to 68e9eae Compare March 3, 2021 11:35
@carlaKC carlaKC requested review from bhandras and cfromknecht March 4, 2021 07:23
Comment thread lnrpc/routerrpc/router.proto Outdated
@Roasbeef
Copy link
Copy Markdown
Member

Re making the "experimental'-ness of RPCs more explicit, offline Conner suggested an X prefix similar to /x/go, which isn't a bad idea. If we want to go that route in the end, I think we can merge this as is since it's very close and blocking experiments on our end, then ensure that we circle back to change things if we want to before we tag 0.13.

@cfromknecht
Copy link
Copy Markdown
Contributor

@Roasbeef sounds good to me 👍

@carlaKC carlaKC force-pushed the 4483-importmissioncontrol branch from 68e9eae to 01201ef Compare March 15, 2021 09:30
@carlaKC carlaKC requested review from bhandras and cfromknecht and removed request for bhandras and cfromknecht March 15, 2021 13:42
Copy link
Copy Markdown
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

looks just about ready! last comment is the addition of the X prefix to the rpc method and messages

Comment thread lnrpc/routerrpc/router.proto Outdated
@carlaKC carlaKC force-pushed the 4483-importmissioncontrol branch from 01201ef to cb97762 Compare March 18, 2021 08:52
@joostjager
Copy link
Copy Markdown
Contributor

Getting back to the privacy and space aspects: one option would be to only share successes. I think that priming with just the success amounts for each channel is already very effective. The failures will decay anyway. Users could choose to prune their exports before sharing.

Copy link
Copy Markdown
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🍫

@Roasbeef Roasbeef merged commit 6f3e14a into lightningnetwork:master Mar 23, 2021
@Roasbeef
Copy link
Copy Markdown
Member

Getting back to the privacy and space aspects: one option would be to only share successes.

Yeah that may make sense for certain classes of user for this RPC, what's implemented right now doesn't preclude that.

yaslama added a commit to breez/lnd that referenced this pull request Jul 19, 2021
roeierez added a commit to breez/lnd that referenced this pull request Sep 5, 2021
…rical-sync

* commit 'b444ae37125d32eaab1d68e73c4b1b12bf6451bc':
  backport lightningnetwork#5061 - routing: add mission control import rpc
  backport lightningnetwork#4909 - routing: allow runtime updates to mission control config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvements to existing features / behaviour mission control P1 MUST be fixed or reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

routerrpc+routing: add ability to import mission control weights/data

5 participants