8000 docs: ADR-106: gRPC API by thanethomson · Pull Request #850 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

docs: ADR-106: gRPC API #850

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Mar 27, 2024
Merged

docs: ADR-106: gRPC API #850

merged 10 commits into from
Mar 27, 2024

Conversation

thanethomson
Copy link
Contributor
@thanethomson thanethomson commented May 17, 2023 •
edited by andynog
Loading

Partially addresses #81.

📖 Rendered

There's already a partial implementation of this on the feature/adr101-pull-companion branch. See #818.

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

@thanethomson thanethomson added the adr Issue relating to the implementation of a specific ADR label May 17, 2023
@thanethomson thanethomson added this to the 2023-Q2 milestone May 17, 2023
@thanethomson thanethomson requested a review from a team as a code owner May 17, 2023 10:29
@thanethomson thanethomson mentioned this pull request May 17, 2023
Open
3 tasks
lasarojc
lasarojc reviewed May 17, 2023
View reviewed changes
@romac
Copy link
Contributor
romac commented May 17, 2023

Looks great!

@thanethomson
Copy link
Contributor Author

Looks great!

Glad to hear you think so! Are there any other endpoints that you'd like to see exposed here @romac?

@hdevalence
Copy link

This looks great for us, I think it would cover our use-case and replace our current proxy service.

It could also be helpful to have access to the /genesis and /net_info endpoints, which we use to implement an auto-configurator (pd testnet join, which pulls config data from a running node and uses it to build a skeleton set of configs), but this is less important to us than the RPCs required for processing transactions.

@romac
Copy link
Contributor
romac commented May 18, 2023 •
edited
Loading

Glad to hear you think so! Are there any other endpoints that you'd like to see exposed here @romac?

Here's the full list of RPC endpoints we use from Hermes: https://github.com/informalsystems/hermes/blob/master/docs/spec/relayer/Data-Requirements.md

Aside from the first two endpoints which are strictly required for relaying (/health & /consensus_params), the gRPC list is missing these three endpoints:

  • /header
  • /tx_search
  • /block_search

While it would be nice to get rid of JSON-RPC altogether, since we'll have to maintain multi-version support for some time we could initially do without and keep hitting the JSON-RPC endpoints for those, and wait until they are added at a later time.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale For use by stalebot label May 29, 2023
@jmalicevic jmalicevic removed the stale For use by stalebot label May 29, 2023
@thanethomson thanethomson added the wip Work in progress label Jun 6, 2023
@thanethomson
Add first draft
b6dd6b8 
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson
Add section on service versioning
eff47dc 
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson
Fix typo
ca2bedc 
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson requested a review from a team as a code owner July 4, 2023 22:05
@thanethomson
Add GetLatestHeight method for BlockService
e1e30e3 
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson
Add endpoints based on user feedback
e5f5bdf 
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson
Remove redundancy in method name
7301d6c 
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson
Copy link
Contributor Author

@hdevalence I've added a NetworkService from which you can query the genesis data and currently connected peers - does that suffice? Do you need the listener info that the /net_info provides?

@romac I've updated the endpoints - will this suffice for the relayer?

@lasarojc lasarojc self-requested a review July 5, 2023 10:36
@thanethomson thanethomson modified the milestones: 2023-Q2, 2023-Q3 Jul 14, 2023
@romac
Copy link
Contributor
romac commented Jul 14, 2023

@romac I've updated the endpoints - will this suffice for the relayer?

I might be missing something but I believe the /latest_commit, /commit, and /validators endpoints are missing. Those are required by the light client to fetch and verify light blocks.

Aside from that, looks great! 🙏

@tony-iqlusion
Copy link

@thanethomson what about privval support? (previously tracked as tendermint/tendermint#9256)

If that's in the cards, I have some feedback on the design of that API based on some recent experiences (namely it would be nice if there were an error which KMS systems can use to communicate the last signed h/r/s back to CometBFT in the event of double signing, so CometBFT could potentially update its round counter in response)

@thanethomson
Copy link
Contributor Author

what about privval support?

That could definitely be on the cards @tony-iqlusion. We do have a separate tracking issue for that in #476, but we'd be very interested to hear what your ideal API/interaction model would look like.

@tony-iqlusion
Copy link
tony-iqlusion commented Aug 3, 2023 •
edited
Loading

Really all I'd like is a documented standard way of communicating a double signing error from sign_vote and sign_proposal and including metadata that CometBFT can parse and use to inform future signing requests. Something like the following:

  • On double signing error, return 409 Conflict to indicate a signature at the requested height already exists
  • The error should include metadata for last_height in h/r/s format, which reflects the last height a signature was computed for

If last_height reflects the current block height, but CometBFT's round counter is lower than the KMS's, then CometBFT can advance the round counter to one more than the one included in last_height.

Right now CometBFT does not handle privval errors very well. This was pretty painful with the last Neutron halt, where the chain halted for several hours and validators needed to resync. When the halt height was reached syncing, it took an additional hour for validators using TMKMS to reach the correct round to participate in consensus. It seems settings like timeout_prevote(_delta) and timeout_precommit(_delta) are not properly honored in this case (which perhaps I should report as a separate bug).

Edit: see also RemoteSignerError

@thanethomson thanethomson modified the milestones: 2023-Q3, 2023-Q4 Aug 14, 2023
@tony-iqlusion tony-iqlusion mentioned this pull request Oct 25, 2023
Open
@thanethomson thanethomson marked this pull request as draft October 31, 2023 19:05
@adizere adizere modified the milestones: 2023-Q4, 2024-Q1 Jan 10, 2024
@melekes
Copy link
Contributor
melekes commented Jan 31, 2024

@thanethomson what's the state of this work? Should we take over here?

@thanethomson
Copy link
Contributor Author

@thanethomson what's the state of this work? Should we take over here?

Yes please @melekes!

@andynog andynog changed the title ADR 105: gRPC API docs: ADR 105: gRPC API Mar 27, 2024
@andynog andynog self-assigned this Mar 27, 2024
@adizere adizere added this to the 2024-Q1 milestone Mar 27, 2024
adizere
adizere approved these changes Mar 27, 2024
View reviewed changes
Copy link
Member
@adizere adizere left a comment

Choose a reason for hiding this comment

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

Pre-approving based on a sync review with Andy today.

What is left:

  • double-check comments from Henry & Tony & Romain
  • validate the Go API server instantiation
  • double-check potential name conflict "ADR 105"

@andynog andynog changed the title docs: ADR 105: gRPC API docs: ADR-106: gRPC API Mar 27, 2024
@andynog andynog marked this pull request as ready for review March 27, 2024 21:56
@andynog andynog added this pull request to the merge queue Mar 27, 2024
Merged via the queue into main with commit 0147e63 Mar 27, 2024
@andynog andynog deleted the thane/81-grpc-adr branch March 27, 2024 22:03
@tony-iqlusion tony-iqlusion mentioned this pull request Aug 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adizere adizere adizere approved these changes

@lasarojc lasarojc Awaiting requested review from lasarojc

Assignees

@andynog andynog

Labels
adr Issue relating to the implementation of a specific ADR wip Work in progress
Projects
No open projects
CometBFT
Status: Done
CometBFT 2023
Status: Ready for Review
Milestone
2024-Q1
Development

Successfully merging this pull request may close these issues.
9 participants
@thanethomson @romac @hdevalence @tony-iqlusion @melekes @adizere @lasarojc @jmalicevic @andynog
0