gRPC API Redux

[martyall]

This PR builds on the work of @alexanderbez and @charlescrain to implement the gRPC server interface via protobuf service descriptions. Currently the only real difference between this branch and #184 is the RWMutex used to control reads and writes. I am not a golang developer, but this seems pretty straight forward.

In addition to the tests in server_test.go, I have tested hs-iavl-client against a docker image build from this branch and (after updating the library with the correct proto files) it passes. If you would like more tests, please specify here.

[martyall]

note that I was forced to copy files over from the other branch rather than try to rebase because the repositories had diverged so much that it not worth the effort

[erikgrinaker]

Thanks for picking this up! This still needs a bit of work, let me know if you're up for it or if you'd like a hand from someone more familiar with Go.

[martyall]

Ok so I think I have done everything we've discussed up to now. In the end the methods added were:

• List
• AvailableVersions
• Load, LoadVersion, and LoadVersionForOverwriting
• Version
• Size

I haven't written tests yet for any of the new methods, I was thinking I would write them for each one. However if there any additional tests requested, now would be the time to make sure.

[erikgrinaker]

Thanks @blinky3713, that's awesome! Would you like me to do another pass over this now, or wait until all unresolved comments have been addressed?

As for tests, it's always desirable with more tests, but since this is just a thin wrapper it should be sufficient as a start to just check that all gRPC and HTTP methods work and that all parameters are properly passed through.

[martyall]

@erikgrinaker I still have some of the smaller initial comments to address, I will ping you when it's ready for final review.

[martyall]

@erikgrinaker I think it's ready for a final review, I don't understand this conflicting go.sum error but it's causing problems with CI.

I didn't include tests for Load, LoadVersion and LoadVersionForOverwrite because they were just direct wrappers.

My test for List could probably be improved but I'm open to advice.

I built a docker image and tested against hs-iavl-client (this branch), everything seems to work still. However, when I try to test a simple set/get using curl for the README, i'm failing to save values. For example:

> curl --location --request GET 'http://localhost:8091/v1/version' --header 'Content-Type: application/json' \
--data-raw '{
    "key": "aGVsbG8=",
    "value": "Ynll"
}'

{
  "result": false
}

> curl --location --request GET 'http://localhost:8091/v1/get' \
--header 'Content-Type: application/json' \
--data-raw '{
    "key": "aGVsbG8=",
}'

{
  "code": 5,
  "message": "the key requested does not exist",
  "details": []
}

Is there something I'm missing about how keys have to be formatted? "aGVsbG8=" is the base64 encoding of the string "hello"

[martyall]

also as a side note, the linters are all broken:

• for the proto-lint and proto-breakage jobs it's unclear to me how to include the third_party directory.
• for the golangci-lint job, I don't see what file it's complaining about. if I install version 1.30.0 on my machine and run the command I get a completely different output:

(base)  ✘ ✝  code/golang/iavl   martin/grpc  golangci-lint run --out-format=github-actions --timeout 10m
::error file=benchmarks/bench_test.go,line=70,col=13::G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
::error file=benchmarks/bench_test.go,line=89,col=15::G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
::error file=benchmarks/bench_test.go,line=128,col=16::G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)

[erikgrinaker]

Sorry for not getting back to you here @blinky3713 - this weekend is pretty busy, but I'll have a look on Monday for sure.

[erikgrinaker]

This looks great, thanks for seeing this through! And props for doing it in a language you're not familiar with! Left some minor comments.

[erikgrinaker]

However, when I try to test a simple set/get using curl for the README, i'm failing to save values.

GET requests can't contain a request body, parameters must be specified as URL parameters - this works:

$ curl -XPOST http://localhost:8091/v1/set -d '{"key": "'$(echo -n foo | base64)'", "value": "'$(echo -n bar | base64)'"}'
{
  "result": false
}

$ curl "http://localhost:8091/v1/get?key=$(echo -n foo | base64)"
{
  "index": "0",
  "value": "YmFy"
}

[erikgrinaker]

for the proto-lint and proto-breakage jobs it's unclear to me how to include the third_party directory.

Add them to build.roots in buf.yaml.

for the golangci-lint job, I don't see what file it's complaining about

You have to expand the command output in the Github lint job output (not very intuitive, I agree). It's complaining about this:

server/server_test.go:39:2: directive `//nolint` is unused (nolintlint)
	//nolint

Just remove the nolint comment.

[erikgrinaker]

I don't understand this conflicting go.sum error but it's causing problems with CI

Remove some of the go.mod packages that shouldn't be there (see review comments), then run go mod tidy to clean up the go.mod and go.sum files. That should do it.

[martyall]

I have made all of the requested changes and updated the example in the README. I'm still getting a conflicting error in go.sum after running go mod tidy. I've never worked with a language where checking in these kinds of files is advised, so I'm not really sure what to do about it.

This should be the last thing so just tell me what else I can do and hopefully that will be it.

[martyall]

@erikgrinaker any advice on this last little step ?

[erikgrinaker]

Sorry again for the late reply, I was on vacation last week.

I'm still getting a conflicting error in go.sum after running go mod tidy.

Ah. Yes, this is a Git conflict. You'll need to merge master into your branch, or rebase your branch. In order to fix the go.sum conflicts, just run go mod tidy and regenerate the correct file instead of trying to resolve manually.

[martyall]

🎉

[erikgrinaker]

🎉! Unfortunately, the panic handler isn't working. You're installing it for the HTTP server, which is running in a separate goroutine, but not for the gRPC server in the main thread - so when the main gRPC panics it takes down the whole process. You can test this by e.g. running panic("boom") in the Version() method.

Here's a patch which fixes it, be sure to run go mod tidy to generate go.sum:

diff --git a/cmd/iavlserver/main.go b/cmd/iavlserver/main.go
index 9220427..346276b 100644
--- a/cmd/iavlserver/main.go
+++ b/cmd/iavlserver/main.go
@@ -14,6 +14,7 @@ import (
        "syscall"
 
        "github.com/gogo/gateway"
+       grpc_recovery "github.com/grpc-ecosystem/go-grpc-middleware/recovery"
        "github.com/grpc-ecosystem/grpc-gateway/runtime"
        "github.com/pkg/errors"
        dbm "github.com/tendermint/tm-db"
@@ -64,8 +65,10 @@ func main() {
                log.Fatalf("failed to listen on %s: %s", *gRPCEndpoint, err)
        }   
 
-       var svrOpts []grpc.ServerOption
-       grpcServer := grpc.NewServer(svrOpts...)
+       grpcServer := grpc.NewServer(
+               grpc.UnaryInterceptor(grpc_recovery.UnaryServerInterceptor()),
+               grpc.StreamInterceptor(grpc_recovery.StreamServerInterceptor()),
+       )
 
        db, err := openDB()
        if err != nil {
diff --git a/go.mod b/go.mod
index f863986..eb3a033 100644
--- a/go.mod
+++ b/go.mod
@@ -7,6 +7,7 @@ require (
        github.com/gogo/gateway v1.1.0
        github.com/gogo/protobuf v1.3.1
        github.com/golang/protobuf v1.4.2
+       github.com/grpc-ecosystem/go-grpc-middleware v1.2.1
        github.com/grpc-ecosystem/grpc-gateway v1.14.6
        github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect
        github.com/pkg/errors v0.9.1

[erikgrinaker]

Cool cool, let's merge this thing! Thanks for the PR, you really went above and beyond here! 🚀