-
Notifications
You must be signed in to change notification settings - Fork 268
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
feat: support gRPC endpoints in core #1513
base: v0.34.x-celestia
Are you sure you want to change the base?
Conversation
Just found out that once we enable the app grpc, these endpoints get overridden, I'll be looking into it |
This branch is now ready to be merged:
|
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.
Thanks for E2E testing this change @rach-id. Have a few questions before we merge
message BlockByHashResponse { | ||
tendermint.types.Part block_part = 1; | ||
tendermint.types.Commit commit = 2; | ||
tendermint.types.ValidatorSet validator_set = 3; | ||
tendermint.types.BlockMeta block_meta = 4; | ||
bool is_last = 5; | ||
} | ||
|
||
message BlockByHeightResponse { | ||
tendermint.types.Part block_part = 1; | ||
tendermint.types.Commit commit = 2; | ||
tendermint.types.ValidatorSet validator_set = 3; | ||
tendermint.types.BlockMeta block_meta = 4; | ||
bool is_last = 5; | ||
} |
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.
Is it possible to prefix these types as StreamedBlockByHashResponse
for example so it's more obvious that this type is streamed. Also could we add a comment that the commit, validator set and block meta is only populated for the first response in that stream? (I guess this is how it works right).
Also why BlockMeta and not just Header?
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.
Is it possible to prefix these types as StreamedBlockByHashResponse for example so it's more obvious that this type is streamed. Also could we add a comment that the commit, validator set and block meta is only populated for the first response in that stream? (I guess this is how it works right).
Also why BlockMeta and not just Header?
that's what node team asked for
return ps.AddPartWithoutProof(part) | ||
} | ||
|
||
func (ps *PartSet) AddPartWithoutProof(part *Part) (bool, error) { |
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 don't see where else we call this. What was the reason for splitting?
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.
it's called in Celestia-node to have the parts and build the block without verifying the proof
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.
why do we want to build the block without verifying the proof
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.
Since they make the assumption that every bridge node will be connected to a local full node. So the data is trusted and no need to verify inclusion. Also to reduce the size of the parts.
The initial implementation provided the proofs by default, but they said its better to disable it by default
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.
LGTM. Left a few non-blocking questions / suggestions
env.Logger.Debug("couldn't cast event data to new block") | ||
continue |
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.
[question] should this log an error and terminate instead of continuing?
env.Logger.Debug("couldn't cast event data to new block") | |
continue | |
env.Logger.Error("couldn't cast event data to new block") | |
return |
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 didn't want to have the gRPC logs polluting the output but it's a good point. I will handle this in a separate PR where I will be doing more testing and adding more logic for failures etc. I didn't want to include everything in the same PR to reduce complexity
defer ticker.Stop() | ||
blockAPI.Lock() | ||
defer blockAPI.Unlock() | ||
for i := 1; i < 6; i++ { |
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.
[question] Should we extract a constant for RETRY_ATTEMPTS = 5
for i := 1; i < 6; i++ { | |
for i := 0; i < RETRY_ATTEMPTS; i++ { |
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.
yes, we could. I can extract it if you want
Co-authored-by: Rootul P <[email protected]>
This is an implementation of a streaming API for blocks in core.
Helps close celestiaorg/celestia-app#3421 but not sure it entirely closes it.
It can easily be used:
Ps: I didn't add the tests because I didn't find a direct way of mocking the environment without polluting the rest of the repo (exporting some methods, adding new helpers, etc). And I think since the implementation is simple, just querying the block/state stores for results, it's fine to leave it untested.