-
Notifications
You must be signed in to change notification settings - Fork 26
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
non-node clients grpc api proposal #560
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plan is basically to reuse some already existing implementations, right? At least, I can imagine that with BlockSubscription
and other stuff must be trivial.
features = ["prost"] | ||
|
||
[features] | ||
default = ["tonic/transport", "tonic-build/transport"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A newline please :)
// fetch all blocks from the given initial chainlength to the tip, from all | ||
// possible branches and in increasing order | ||
rpc SyncMultiverse(SyncMultiverseRequest) returns (stream Block); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
|
||
// fetch all blocks from the given initial chainlength to the tip, from all | ||
// possible branches and in increasing order | ||
rpc SyncMultiverse(SyncMultiverseRequest) returns (stream Block); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be consistency issues, e.g. branches starting from different roots. But I guess we just let the client to deal with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about when that can happen, you mean branches with different genesis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I meant something like that:
o - o - o - o - o - o - o
\ - o - o - o
^
|
We make request from this height and
get two incomplete branches instead
of a "proper" tree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be more documentation on how the blocks will be ordered in presence of multiple branches. Perhaps the service needs only to guarantee the order of parent-child relationships, i.e. never send a child before its parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's clarify how would a client use this API.
If I run an explorer and have several branches with different chain lengths in storage, should I request sync with the shortest length I have? Or should I start from the stability depth, since the branches I have stored might have been discarded by the consensus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until the explorer has a database I guess you need to start from 0. But after that, I was thinking on starting from min(stability_depth, last_block_I_have)
. It may lead to sending a lot of redundant blocks, though... And you would need a way of knowing the current length (or the current stable block)
Also, I was thinking on sending all blocks of a given height before the next one, separate branches can be applied potentially in parallel, so I think it's better than sending one branch at a time.
I did write this because it was the simplest thing I could think of, though... Originally I thought about the explorer sending the list of tips it has, but that's not that simple I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been also been wondering if this endpoint shouldn't be merged with the live block subscription, because otherwise there is a chance of missing a block in the middle if you switch from one stream to the other.
In that case the block subscription could have an optional parameter to sync before, and then start sending new blocks.
But clients can work around that by starting both I guess, I'm not really sure what's idiomatic here.
// get tip updates | ||
rpc TipSubscription(TipSubscriptionRequest) returns (stream BlockId); | ||
|
||
rpc MempoolSubscription(MempoolSubscriptionRequest) returns (stream MempoolEvent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now I am not sure that this should belong to chain-libs. We usually did not introduce the concept of mempool at the chain-libs level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, current plan is to move it to chain-network, actually. If we put this in jormungandr... And we want to re-use from a different repository, then we would have to include jormungandr there somehow?
Although, @mzabaluev motivation to put it in chain-network
was the dependencies/build process, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
motivation to put it in
chain-network
was the dependencies/build process, I think
Yeah, mostly that I think.
then we would have to include jormungandr there somehow
Not a huge problem, we are doing that for jormungandr-lib
already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather disentangle protocol things from jormungandr.
And yes, I would prefer to move this into chain-network as a separate module.
51c061b
to
adaac63
Compare
adaac63
to
c51fba7
Compare
Context
It seems there is a need for some API's to support heterogeneous clients (not nodes, basically), besides the one's that we have in rest.
See input-output-hk/jormungandr#3227 and maybe input-output-hk/jormungandr#2354 for some context.
The kinds of clients we could support are:
Disclaimers
Just a PoC to get a conversation started. I think this belongs in
chain-libs
and I wanted to put it inchain-network
, but I think it doesn't fit as it is, and I don't want to spend time reorganizing things without having some discussion about it first.Also, the name
chain-watch
is only half-serious... I just couldn't think of something better yet.Extra comments
The
SyncMultiverse
procedure is somewhat a placeholder to bring out discussion about it... I think it would be a valid way to bootstrap something like the explorer, but there are more than one way we could go about that.For example, we could re-expose some of the procedures in the
node
service. (None does exactly the same, but there are some similar things). Or have a third common service for those things... Or just let clients use thenode
service.Also, I think this may be also missing a way to get the current fragments in the mempool (which we already have in rest, so that brings out another question).
Another thing I think could be nice for these API's, is that we could try to send