lnd: add devrpc sub server and devrpc.ImportGraph to import graph dumps
#6149
Conversation
bhandras
force-pushed
the
graph_import_rpc
branch
from
ebd9f68
to
2871c7d
Compare
bhandras
changed the title
devrpc sub server and devrpc.ImportGraph to import graph dumps
bhandras
force-pushed
the
graph_import_rpc
branch
from
2871c7d
to
9386ab5
Compare
bhandras
force-pushed
the
graph_import_rpc
branch
2 times, most recently
from
97d4cd0
to
83ec7b3
Compare
lnrpc/devrpc/dev.proto
Outdated
| ImportGraph imports a ChannelGraph into the graph database. Shall not be | ||
| used for normal operation. |
There was a problem hiding this comment.
What does "shall not be used for normal operation" mean? That we don't use the graph data if we import it? Or that its only intended for testing?
There was a problem hiding this comment.
Ok, so only for development purposes? I think "should only be used for development" is a bit clearer ?
There was a problem hiding this comment.
Ok, so only for development purposes?
Yes.
I think "should only be used for development" is a bit clearer ?
Thanks, much better the way you phrased.
| node := &channeldb.LightningNode{ | ||
| HaveNodeAnnouncement: true, | ||
| LastUpdate: time.Unix( | ||
| int64(rpcNode.LastUpdate), 0, |
There was a problem hiding this comment.
Will we always have a non-zero rpcNode.LastUpdate? If not, this will set to 1 sept 1970. Perhaps we should validate since its coming from user input?
There was a problem hiding this comment.
How would validation look like for these timestamps? I suppose we could use a threshold of say 2019. Jan 1., but that's also not perfect. Also validating the timestamp may restrict testing.
There was a problem hiding this comment.
Can we just check that the values are non-zero? Or do nodes sometimes not have a last update time?
If they can have zero times, then I think we should only set LastUpdate = time.Unix(rpcNode.LastUpdate,0) if its non-zero. That way we have an empty time.Time{} and .IsZero will work. As is, we set all zero values to unix epoch zero, which doesn't really make sense.
| policy := &channeldb.ChannelEdgePolicy{ | ||
| ChannelID: rpcEdge.ChannelId, | ||
| LastUpdate: time.Unix( | ||
| int64(rpcPolicy.LastUpdate), 0, |
There was a problem hiding this comment.
Same time question here. Also we we need to validate any other fields, or are they checked later on when we do the import in UpdateEdgePolicy?
There was a problem hiding this comment.
Similarly what's a valid timestamp for a routing policy update?
There was a problem hiding this comment.
Surely this can't be zero?
| @@ -0,0 +1,67 @@ | |||
| //go:build dev | |||
There was a problem hiding this comment.
Do we need to update makefile's linter to include files behind build tags, or are they already covered?
There was a problem hiding this comment.
Good catch, we never covered files with the dev tag before. Added in separate commit.
This commits adds the devrpc package which implements a subserver that adds clean separation for RPC calls useful for development and debugging. This subserver is only compiled in if the dev tag is set. Furthermore the commit adds the devrpc.ImportGraph call which can import a graph dump obtained from another node by calling DescribeGraph. Since the graph dump does not include the auth proofs, the imported channels will be considered private.
bhandras
force-pushed
the
graph_import_rpc
branch
from
83ec7b3
to
39ea802
Compare
| client, cleanUp := getDevClient(ctx) | ||
| defer cleanUp() | ||
|
|
||
| jsonFile := lncfg.CleanAndExpandPath(ctx.Args().First()) |
There was a problem hiding this comment.
nit: could check Nargs=1 here
The API doc generator failed after merging lightningnetwork#6149 because of the `lncli importgraph` tag in the proto file. We could also fix the API doc generator by adding the "dev" build tag, but not including any dev only features in the API docs is probably the preferred path.
The PR adds the
devrpcpackage which implements a subserver that is activated with thedevbuild tag. Additionally the PR adds thedevrpc.ImportGraphcall andlncli importgraphcommand to allow importing (large) graphs from dumps cleanly for performance and other testing purposes.