gRPC API

[alexanderbez]

gRPC API w/ JSON HTTP proxy (via grpc-gateway) implementation for the IAVL library.

TODO:

• Define gRPC service
• Define all message types
• Implement service implementation
• Implement JSON HTTP proxy server
• Tests
• Fix build/CI

closes: #183

/cc @blinky3713 @charlescrain

[martyall]

Just a comment after checking on on the status here, will there also be a Rollback endpoint? Is there a different API for the mutable tree vs immutable ?

[tnachen]

@blinky3713 I've updated the PR to return the grpc status code, do you mind verifying it on your end?

[tac0turtle]

proto-breakage will fail until this is merged into master

[tnachen]

@marbar3778 I think this is ready to merge, any more reviews?

[tac0turtle]

Havent read much into it since the updates but does it support the new pruning logic?

can you also make sure that all the check boxes in the body of the PR are checked if they are completed?

[tnachen]

@marbar3778 Just checked the boxes and updated the flags to include pruning options

[erikgrinaker]

I understand the use-case for accessing IAVL in other languages without writing a wrapper. However, going from an embedded database library to a networked database server is a pretty fundamental change - are we sure this is something we want to do?

In particular, I am thinking of concurrency control. IAVL is not concurrency safe, but gRPC is fundamentally concurrent. There is also an expectation that a network server can handle multiple clients, but IAVL is not built for this and will likely not perform well.

If we want do to this, we should wrap the database in a RWMutex as a first step, limit simultaneous connections to e.g. 2, and also document thoroughly that the gRPC service is meant for use by a single client. To achieve good multi-client performance we will need to use a database backend with proper concurrency control built-in (e.g. BadgerDB), and should also support something approximating ACID transactions to avoid conflicts between clients. This is related to tendermint/tm-db#62.

Thoughts?

[alexanderbez]

I have to agree with @erikgrinaker here. At the very least, we need to add smart mutex locking and think carefully around multi-client use and ensure we're ACID-compliant -- at least multi-client reads.

[martyall]

I'm not sure I understand why the full enterprise solution of a distributed database is required before the grpc layer is implemented. The semantics can remain undefined in the event that you have multiple concurrent writes -- just leave a big warning or mark the feature as experimental.

Unless I'm severely mistaking, the original intended use case of the grpc api is unaffected by concurrency, which was to be able to use the cosmos iavl storage from a non-golang abci application. When we have one tendermint client + abci application, the transactions are executed sequentially during block execution, and the transactions themselves are sequential. There are no race conditions for writes. As for reads, the abci application is responsible for keeping the appropriate indices -- e.g. the index used for query or checkTx commands points to committed storage, hence there are no race conditions there either.

Would it not be reasonable to just commit the work that the tendermint team and @charlescrain already did here and then open up separate issues to discuss the possible dirrections to allow concurrency?

To make the case concrete, the haskell abci framework that we wrote depends on this grpc layer. We are using an extremely old commit, and while it works fine for toy apps it's unnerving to keep this feature open ended.

[erikgrinaker]

I'm not sure I understand why the full enterprise solution of a distributed database is required before the grpc layer is implemented. The semantics can remain undefined in the event that you have multiple concurrent writes -- just leave a big warning or mark the feature as experimental.

At the very least, we should wrap the tree in an RWMutex.

Unless I'm severely mistaking, the original intended use case of the grpc api is unaffected by concurrency, which was to be able to use the cosmos iavl storage from a non-golang abci application.

Yes, this is your use-case (and I totally get that use-case). However, others are quite likely to start using this for other use-cases that it's not well-suited for, and then get sad when things don't work out very well.

Would it not be reasonable to just commit the work that the tendermint team and @charlescrain already did here and then open up separate issues to discuss the possible dirrections to allow concurrency?

Personally I'm a bit worried about the additional maintenance overhead, since IAVL is already a bit under-maintained. Maybe we could split this out as a separate package that could be maintained on a best-effort basis?

@aaronc Since your team will be taking over IAVL maintainership shortly, perhaps you'd like to weigh in here.

[aaronc]

I think the limitations that @erikgrinaker proposes are reasonable.

Regarding maintenance overhead, this is a relatively small layer on top of the go API so it would likely be easier to maintain within this source tree because bugs would ideally be caught by tests. I do think we should mark this as an alpha API so that we don’t make any strong compatibility guarantees.

https://github.com/google/trillian, a similar project, also has a gRPC interface so this is not unprecedented.

I would like to see some tests which I think should be possible in process by just spinning up a server at a random port in a separate thread and sending requests to it.

We don’t currently have the internal bandwidth to finish this work quite yet. But I know the Kepler team needs this and if they have the bandwidth to wrap up this work with adequate test coverage I would support merging it.

I do want to add the disclaimer that big changes may be coming to IAVL, hopefully big improvements. But hopefully this doesn't affect the user facing API too much.

[martyall]

@aaronc I have started the work here https://github.com/f-o-a-m/iavl/tree/martin/grpc . I was able to bring it up to speed with master and all tests are passing. I will implement the dumbest concurrency possible:

1. a global write mutex, no reading while writing, no writing while reading.
2. single sequential writes
3. unlimited concurrent reads

There are already tests (server/server_test.go). When I submit the PR you can explain what changes to make to them or what needs to be added.

[erikgrinaker]

Sounds good, thanks!

[martyall]

I would propose closing this in favor of #296

[erikgrinaker]

Closing this in favor of #296, discussion is continued there.