Skip to content
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

rpc: Add gRPC support #81

Open
2 of 3 tasks
Tracked by #40
thanethomson opened this issue Jan 5, 2023 · 1 comment
Open
2 of 3 tasks
Tracked by #40

rpc: Add gRPC support #81

thanethomson opened this issue Jan 5, 2023 · 1 comment
Assignees
Labels
grpc Anything relating to the gRPC API P:integrator-experience Priority: Improve experience for integrators rpc

Comments

@thanethomson
Copy link
Contributor

thanethomson commented Jan 5, 2023

At present, we support a flavor of JSON-RPC as one of the primary ways of integrating with a running node.

From discussions with users (e.g. @hdevalence), it's clear there's a need for a full gRPC interface to a node. This would dramatically simplify client generation for interaction with a node, and would help get around our very odd (inherited) JSON serialization quirks.

There's some context provided by ADR-057, but no clear decision was documented in that ADR. Previously, the team had decided to deprecate and remove gRPC support (see tendermint/tendermint#7121 and tendermint/tendermint#9683), but that decision has now been reversed.

Definition of Done

There are several deliverables associated with this issue, after which it could be considered done:

Original issue: tendermint/tendermint#9751

@thanethomson
Copy link
Contributor Author

Some of the endpoints we should aim to provide in the first iteration of our API should at least provide the same functionality as those exposed by Penumbra's TendermintProxyService here: https://buf.build/penumbra-zone/penumbra/docs/main:penumbra.client.v1alpha1#penumbra.client.v1alpha1.TendermintProxyService

@adizere adizere added this to the 2024-Q1 milestone Jan 11, 2024
github-merge-queue bot pushed a commit that referenced this issue Mar 27, 2024
Partially addresses #81.

[:book:
Rendered](https://github.com/cometbft/cometbft/blob/thane/81-grpc-adr/docs/references/architecture/adr-106-grpc-api.md)

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](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Co-authored-by: Andy Nogueira <me@andynogueira.dev>
@andynog andynog self-assigned this Mar 29, 2024
@andynog andynog added the grpc Anything relating to the gRPC API label Mar 29, 2024
@adizere adizere removed this from the 2024-Q1 milestone Apr 1, 2024
cometcrafter pushed a commit to graphprotocol/cometbft that referenced this issue Jun 1, 2024
…… (backport cometbft#76) (cometbft#81)

* perf(blockstore): Add LRU caches to blockstore operations used in consensus (backport cometbft#3003) (cometbft#3083)

Closes cometbft#2844

We are seeing that the blockstore loading operations get used in hot
loops within gossip routines, and queryMaj23 routines. This PR reduces
that overhead using an LRU cache.

The LRU cache does have a mutex on every get, but the time with the LRU
cache is 9x faster than without (before even adding in DB overheads),
due to the proto unmarshalling saved. We could imagine a setup where we
avoided a lock there entirely. I don't think this is worth right now,
since the new code is 9x faster, and these mostly appear in catchup code
which should not be highly contended for across peers at the same time.

With the new benchmark I added:
OLD:
```
BenchmarkRepeatedLoadSeenCommit-12         24447             54691 ns/op           46495 B/op        319 allocs/op
```
NEW:
```
BenchmarkRepeatedLoadSeenCommit-12        224131              6401 ns/op            8320 B/op          2 allocs/op
```

It turns out these gossip routines don't need mutative copies, so we
could optimize out the large allocation in the future if we want.

1 hour cpu profile that shows this appearing in prod:

![image](https://github.com/cometbft/cometbft/assets/6440154/5a7e0f02-8385-4c01-aa6a-dba2a2bf376d)

The state machine execution time here for context is 92 seconds. So this
is adding up in system load (and GC! The GC load is 52GB, the entire
trace is 200GB, with other parts being optimized down from recent PRs)

---

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request cometbft#3003 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
(cherry picked from commit 0c10bd5)

* Add Changelog

(cherry picked from commit 4594f29)

# Conflicts:
#	CHANGELOG.md

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <dojha@berkeley.edu>
Co-authored-by: PaddyMc <paddymchale@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grpc Anything relating to the gRPC API P:integrator-experience Priority: Improve experience for integrators rpc
Projects
Status: In Progress
Status: In Progress
Development

No branches or pull requests

3 participants