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

R4R: Bianjie/lcd implementation #2147

Closed
wants to merge 29 commits into from
Closed
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
34548db
lcd implemeation
Aug 16, 2018
eccb889
Add golang dependency for swagger
Aug 16, 2018
2047673
Add key rest api
Aug 16, 2018
85f47d2
Add stake query api to lcd swagger
Aug 16, 2018
187dda4
Merge branch 'bianjie/lcd' of https://github.com/cosmos/cosmos-sdk in…
Aug 18, 2018
26af936
Refactor code according to lint result
Aug 21, 2018
4607ca9
Refactor comment and fix test failure
Aug 21, 2018
7528534
Add lcd test for swagger lcd
Aug 21, 2018
473fe21
Fix for test lint warnning
Aug 21, 2018
95e367a
Refactor comment
Aug 21, 2018
9d944b0
Add new test for lcd with swagger
Aug 22, 2018
f266472
Refactor lcd according to code review
Aug 23, 2018
6cdf21d
Refactor code according to code review
Aug 24, 2018
9409e73
Fix errors in test_cover and test_lint
Aug 24, 2018
8e843bc
Merge branch 'haoyang/lcd-implementation-develop' into haoyang/lcd-im…
Aug 25, 2018
70aa433
Add new stake api and remove advanced command
Aug 25, 2018
7eda13a
Merge pull request #5 from HaoyangLiu/haoyang/lcd-implementation-new
Aug 25, 2018
7a4b019
Add spec to client.md and add this subcommand to basecli
Aug 27, 2018
00c43fa
change key apis to accommodate to doc/light/api.md
Aug 27, 2018
2455604
Refactor multi store proof verification, reuse commitInfo hash calcul…
Aug 27, 2018
7760296
Fix key management apis test error
Aug 27, 2018
f6ebccf
Refactor lcd related spec
Aug 27, 2018
986504f
Add missed api in ICS0 and ICS1, remove the response wrapper in api.md
Aug 28, 2018
9427eb5
Fix failures in test_cover and test_lint
Aug 28, 2018
81f294b
Add key sign command and fix test_lint failure in key sign
Aug 28, 2018
b6bb5ff
Merge with cosmos develop
Aug 28, 2018
ff15759
Fix failure in test_cover and test_lint
Aug 28, 2018
5b250cc
Refactor code according to code review feedback
Aug 29, 2018
135473c
Fix a mistake in previous commit
Aug 29, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions Gopkg.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@
name = "github.com/stretchr/testify"
version = "=1.2.1"

[[constraint]]
name = "github.com/swaggo/gin-swagger"
version = "=v1.0.0"

[[constraint]]
name = "github.com/swaggo/swag"
version = "=v1.3.2"

[[override]]
name = "github.com/tendermint/go-amino"
version = "=v0.12.0-rc0"
Expand Down
15 changes: 15 additions & 0 deletions client/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/spf13/viper"

rpcclient "github.com/tendermint/tendermint/rpc/client"
tendermintLite"github.com/tendermint/tendermint/lite"
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting error

)

const ctxAccStoreName = "acc"
Expand All @@ -30,6 +31,8 @@ type CLIContext struct {
Async bool
JSON bool
PrintResponse bool
Certifier tendermintLite.Certifier
ClientManager *ClientManager
}

// NewCLIContext returns a new initialized CLIContext with parameters from the
Expand Down Expand Up @@ -113,3 +116,15 @@ func (ctx CLIContext) WithUseLedger(useLedger bool) CLIContext {
ctx.UseLedger = useLedger
return ctx
}

// WithCertifier - return a copy of the context with an updated Certifier
func (ctx CLIContext) WithCertifier(certifier tendermintLite.Certifier) CLIContext {
ctx.Certifier = certifier
return ctx
}

// WithClientManager - return a copy of the context with an updated ClientManager
func (ctx CLIContext) WithClientManager(clientManager *ClientManager) CLIContext {
ctx.ClientManager = clientManager
return ctx
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline.

46 changes: 46 additions & 0 deletions client/context/loadbalancing.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package context
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this entire needs to be formatted correctly. Also, it should probably be renamed to client_manager.go


import (
rpcclient "github.com/tendermint/tendermint/rpc/client"
"strings"
"sync"
"github.com/pkg/errors"
)

// ClientManager is a manager of a set of rpc clients to full nodes.
// This manager can do load balancing upon these rpc clients.
type ClientManager struct {
clients []rpcclient.Client
currentIndex int
mutex sync.RWMutex
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'll suffice to just have a write only mutex here (I don't see any logic that obtains a read-lock).

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean use sync.Mutex instead?

}

// NewClientManager create a new ClientManager
func NewClientManager(nodeURIs string) (*ClientManager,error) {
if nodeURIs != "" {
nodeURLArray := strings.Split(nodeURIs, ",")
var clients []rpcclient.Client
for _, url := range nodeURLArray {
client := rpcclient.NewHTTP(url, "/websocket")
clients = append(clients, client)
}
mgr := &ClientManager{
currentIndex: 0,
clients: clients,
}
return mgr, nil
}
return nil, errors.New("missing node URIs")
}

func (mgr *ClientManager) getClient() rpcclient.Client {
mgr.mutex.Lock()
defer mgr.mutex.Unlock()

client := mgr.clients[mgr.currentIndex]
mgr.currentIndex++
if mgr.currentIndex >= len(mgr.clients){
mgr.currentIndex = 0
}
return client
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing new line.

16 changes: 16 additions & 0 deletions client/context/loadbalancing_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package context

import (
"testing"
"github.com/stretchr/testify/assert"
)

func TestLoadBalancing(t *testing.T) {
nodeURIs := "10.10.10.10:26657,20.20.20.20:26657,30.30.30.30:26657"
clientMgr,err := NewClientManager(nodeURIs)
assert.Empty(t,err)
endpoint := clientMgr.getClient()
assert.NotEqual(t,endpoint,clientMgr.getClient())
clientMgr.getClient()
assert.Equal(t,endpoint,clientMgr.getClient())
}
77 changes: 76 additions & 1 deletion client/context/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,19 @@ import (
cmn "github.com/tendermint/tendermint/libs/common"
rpcclient "github.com/tendermint/tendermint/rpc/client"
ctypes "github.com/tendermint/tendermint/rpc/core/types"
"github.com/cosmos/cosmos-sdk/store"
"github.com/cosmos/cosmos-sdk/wire"
"strings"
tendermintLiteProxy "github.com/tendermint/tendermint/lite/proxy"
abci "github.com/tendermint/tendermint/abci/types"
)

// GetNode returns an RPC client. If the context's client is not defined, an
// error is returned.
func (ctx CLIContext) GetNode() (rpcclient.Client, error) {
if ctx.ClientManager != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see the reason to have a both a client manager and a client. Seems like we should only have a client manager which, in the original case, has only a single client. Thoughts?

return ctx.ClientManager.getClient(), nil
}
if ctx.Client == nil {
return nil, errors.New("no RPC client defined")
}
Expand Down Expand Up @@ -282,6 +290,50 @@ func (ctx CLIContext) ensureBroadcastTx(txBytes []byte) error {
return nil
}

// proofVerify perform response proof verification
func (ctx CLIContext) proofVerify(path string, resp abci.ResponseQuery) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

verifyProof sounds better I think.

// Data from trusted node or subspace query doesn't need verification
if ctx.TrustNode || !isQueryStoreWithProof(path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the !isQueryStoreWithProof(path) should be moved outside to the caller. This provides a better abstraction.

return nil
}

// TODO: Later we consider to return error for missing valid certifier to verify data from untrusted node
if ctx.Certifier == nil {
if ctx.Logger != nil {
io.WriteString(ctx.Logger, fmt.Sprintf("Missing valid certifier to verify data from untrusted node\n"))
}
return nil
}

node, err := ctx.GetNode()
if err != nil {
return err
}
// AppHash for height H is in header H+1
commit, err := tendermintLiteProxy.GetCertifiedCommit(resp.Height+1, node, ctx.Certifier)
if err != nil {
return err
}

var multiStoreProof store.MultiStoreProof
cdc := wire.NewCodec()
err = cdc.UnmarshalBinary(resp.Proof, &multiStoreProof)
if err != nil {
return errors.Wrap(err, "failed to unmarshalBinary rangeProof")
}

// Validate the substore commit hash against trusted appHash
substoreCommitHash, err := store.VerifyMultiStoreCommitInfo(multiStoreProof.StoreName, multiStoreProof.CommitIDList, commit.Header.AppHash)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can break this call into multiple lines for legibility.

if err != nil {
return errors.Wrap(err, "failed in verifying the proof against appHash")
}
err = store.VerifyRangeProof(resp.Key, resp.Value, substoreCommitHash, &multiStoreProof.RangeProof)
if err != nil {
return errors.Wrap(err, "failed in the range proof verification")
}
return nil
}

// query performs a query from a Tendermint node with the provided store name
// and path.
func (ctx CLIContext) query(path string, key common.HexBytes) (res []byte, err error) {
Expand All @@ -301,10 +353,15 @@ func (ctx CLIContext) query(path string, key common.HexBytes) (res []byte, err e
}

resp := result.Response
if !resp.IsOK() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is resp.IsOK() invalid?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry. Maybe it is a mistake in code merging. I have changed it to resp.IsOK which seems more elegant

if resp.Code != uint32(0) {
return res, errors.Errorf("query failed: (%d) %s", resp.Code, resp.Log)
}

err = ctx.proofVerify(path, resp)
if err != nil {
return nil, err
}

return resp.Value, nil
}

Expand All @@ -314,3 +371,21 @@ func (ctx CLIContext) queryStore(key cmn.HexBytes, storeName, endPath string) ([
path := fmt.Sprintf("/store/%s/%s", storeName, endPath)
return ctx.query(path, key)
}

// isQueryStoreWithProof expects a format like /<queryType>/<storeName>/<subpath>
// queryType can be app or store
// if subpath equals to store or key, then return true
func isQueryStoreWithProof(path string) (bool) {
if !strings.HasPrefix(path, "/") {
return false
}
paths := strings.SplitN(path[1:], "/", 3)
if len(paths) != 3 {
return false
}
// WARNING This should be consistent with query method in iavlstore.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on this warning and why we need it? What needs to be done about it?

Copy link
Author

Choose a reason for hiding this comment

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

In iavlstore.go, proof will be included in response only when the query path is /store or /key. I concern that this may be changed in iavlstore.go. If there are some changes about proof building in iavlstore.go, then we must change code here to be consistent with the changes.

if paths[2] == "store" || paths[2] == "key" {
return true
}
return false
}
4 changes: 4 additions & 0 deletions client/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ const (
FlagAsync = "async"
FlagJson = "json"
FlagPrintResponse = "print-response"
FlagListenAddr = "laddr"
FlagSwaggerHostIP = "swagger-host-ip"
FlagModules = "modules"
FlagNodeList = "node-list"
)

// LineBreak can be included in a command list to provide a blank line
Expand Down
32 changes: 32 additions & 0 deletions client/httputils/httputils.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package httputils

import (
"github.com/gin-gonic/gin"
"net/http"
)

// NewError create error http response
func NewError(ctx *gin.Context, errCode int, err error) {
errorResponse := HTTPError{
API: "2.0",
Code: errCode,
}
if err != nil {
errorResponse.ErrMsg = err.Error()
}

ctx.JSON(errCode, errorResponse)
}

// NormalResponse create normal http response
func NormalResponse(ctx *gin.Context, data []byte) {
ctx.Status(http.StatusOK)
ctx.Writer.Write(data)
}

// HTTPError is http response with error
type HTTPError struct {
API string `json:"rest api" example:"2.0"`
Code int `json:"code" example:"500"`
ErrMsg string `json:"error message"`
}
Loading