light/rpc: fix ABCIQuery (#5375)

Closes #5106

light/rpc/client.go

@@ -5,7 +5,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 	"time"
 
 	"github.com/gogo/protobuf/proto"
@@ -15,7 +14,6 @@ import (
 	tmbytes "github.com/tendermint/tendermint/libs/bytes"
 	tmmath "github.com/tendermint/tendermint/libs/math"
 	service "github.com/tendermint/tendermint/libs/service"
-	light "github.com/tendermint/tendermint/light"
 	rpcclient "github.com/tendermint/tendermint/rpc/client"
 	ctypes "github.com/tendermint/tendermint/rpc/core/types"
 	rpctypes "github.com/tendermint/tendermint/rpc/jsonrpc/types"
@@ -24,26 +22,54 @@ import (
 
 var errNegOrZeroHeight = errors.New("negative or zero height")
 
-// Client is an RPC client, which uses light#Client to verify data (if it can be
-// proved!).
+// KeyPathFunc builds a merkle path out of the given path and key.
+type KeyPathFunc func(path string, key []byte) (merkle.KeyPath, error)
+
+// LightClient is an interface that contains functionality needed by Client from the light client.
+type LightClient interface {
+	ChainID() string
+	VerifyLightBlockAtHeight(ctx context.Context, height int64, now time.Time) (*types.LightBlock, error)
+	TrustedLightBlock(height int64) (*types.LightBlock, error)
+}
+
+// Client is an RPC client, which uses light#Client to verify data (if it can
+// be proved!). merkle.DefaultProofRuntime is used to verify values returned by
+// ABCIQuery.
 type Client struct {
 	service.BaseService
 
 	next rpcclient.Client
-	lc   *light.Client
-	prt  *merkle.ProofRuntime
+	lc   LightClient
+	// Proof runtime used to verify values returned by ABCIQuery
+	prt       *merkle.ProofRuntime
+	keyPathFn KeyPathFunc
 }
 
 var _ rpcclient.Client = (*Client)(nil)
 
+// Option allow you to tweak Client.
+type Option func(*Client)
+
+// KeyPathFn option can be used to set a function, which parses a given path
+// and builds the merkle path for the prover. It must be provided if you want
+// to call ABCIQuery or ABCIQueryWithOptions.
+func KeyPathFn(fn KeyPathFunc) Option {
+	return func(c *Client) {
+		c.keyPathFn = fn
+	}
+}
+
 // NewClient returns a new client.
-func NewClient(next rpcclient.Client, lc *light.Client) *Client {
+func NewClient(next rpcclient.Client, lc LightClient, opts ...Option) *Client {
 	c := &Client{
 		next: next,
 		lc:   lc,
-		prt:  defaultProofRuntime(),
+		prt:  merkle.DefaultProofRuntime(),
 	}
 	c.BaseService = *service.NewBaseService(nil, "Client", c)
+	for _, o := range opts {
+		o(c)
+	}
 	return c
 }
 
@@ -70,15 +96,18 @@ func (c *Client) ABCIInfo(ctx context.Context) (*ctypes.ResultABCIInfo, error) {
 	return c.next.ABCIInfo(ctx)
 }
 
+// ABCIQuery requests proof by default.
 func (c *Client) ABCIQuery(ctx context.Context, path string, data tmbytes.HexBytes) (*ctypes.ResultABCIQuery, error) {
 	return c.ABCIQueryWithOptions(ctx, path, data, rpcclient.DefaultABCIQueryOptions)
 }
 
-// GetWithProofOptions is useful if you want full access to the ABCIQueryOptions.
-// XXX Usage of path?  It's not used, and sometimes it's /, sometimes /key, sometimes /store.
+// ABCIQueryWithOptions returns an error if opts.Prove is false.
 func (c *Client) ABCIQueryWithOptions(ctx context.Context, path string, data tmbytes.HexBytes,
 	opts rpcclient.ABCIQueryOptions) (*ctypes.ResultABCIQuery, error) {
 
+	// always request the proof
+	opts.Prove = true
+
 	res, err := c.next.ABCIQueryWithOptions(ctx, path, data, opts)
 	if err != nil {
 		return nil, err
@@ -89,8 +118,11 @@ func (c *Client) ABCIQueryWithOptions(ctx context.Context, path string, data tmb
 	if resp.IsErr() {
 		return nil, fmt.Errorf("err response code: %v", resp.Code)
 	}
-	if len(resp.Key) == 0 || resp.ProofOps == nil {
-		return nil, errors.New("empty tree")
+	if len(resp.Key) == 0 {
+		return nil, errors.New("empty key")
+	}
+	if resp.ProofOps == nil || len(resp.ProofOps.Ops) == 0 {
+		return nil, errors.New("no proof ops")
 	}
 	if resp.Height <= 0 {
 		return nil, errNegOrZeroHeight
@@ -105,28 +137,28 @@ func (c *Client) ABCIQueryWithOptions(ctx context.Context, path string, data tmb
 
 	// Validate the value proof against the trusted header.
 	if resp.Value != nil {
-		// Value exists
-		// XXX How do we encode the key into a string...
-		storeName, err := parseQueryStorePath(path)
-		if err != nil {
-			return nil, err
+		// 1) build a Merkle key path from path and resp.Key
+		if c.keyPathFn == nil {
+			return nil, errors.New("please configure Client with KeyPathFn option")
 		}
-		kp := merkle.KeyPath{}
-		kp = kp.AppendKey([]byte(storeName), merkle.KeyEncodingURL)
-		kp = kp.AppendKey(resp.Key, merkle.KeyEncodingURL)
+
+		kp, err := c.keyPathFn(path, resp.Key)
+		if err != nil {
+			return nil, fmt.Errorf("can't build merkle key path: %w", err)
+		}
+
+		// 2) verify value
 		err = c.prt.VerifyValue(resp.ProofOps, l.AppHash, kp.String(), resp.Value)
 		if err != nil {
 			return nil, fmt.Errorf("verify value proof: %w", err)
 		}
-		return &ctypes.ResultABCIQuery{Response: resp}, nil
+	} else { // OR validate the absence proof against the trusted header.
+		err = c.prt.VerifyAbsence(resp.ProofOps, l.AppHash, string(resp.Key))
+		if err != nil {
+			return nil, fmt.Errorf("verify absence proof: %w", err)
+		}
 	}
 
-	// OR validate the ansence proof against the trusted header.
-	// XXX How do we encode the key into a string...
-	err = c.prt.VerifyAbsence(resp.ProofOps, l.AppHash, string(resp.Key))
-	if err != nil {
-		return nil, fmt.Errorf("verify absence proof: %w", err)
-	}
 	return &ctypes.ResultABCIQuery{Response: resp}, nil
 }
 
@@ -521,24 +553,6 @@ func (c *Client) UnsubscribeAllWS(ctx *rpctypes.Context) (*ctypes.ResultUnsubscr
 	return &ctypes.ResultUnsubscribe{}, nil
 }
 
-func parseQueryStorePath(path string) (storeName string, err error) {
-	if !strings.HasPrefix(path, "/") {
-		return "", errors.New("expected path to start with /")
-	}
-
-	paths := strings.SplitN(path[1:], "/", 3)
-	switch {
-	case len(paths) != 3:
-		return "", errors.New("expected format like /store/<storeName>/key")
-	case paths[0] != "store":
-		return "", errors.New("expected format like /store/<storeName>/key")
-	case paths[2] != "key":
-		return "", errors.New("expected format like /store/<storeName>/key")
-	}
-
-	return paths[1], nil
-}
-
 // XXX: Copied from rpc/core/env.go
 const (
 	// see README

light/rpc/client_test.go

@@ -0,0 +1,152 @@
+package rpc
+
+import (
+	"context"
+	"encoding/hex"
+	"fmt"
+	"testing"
+
+	ics23 "github.com/confio/ics23/go"
+	"github.com/cosmos/iavl"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+	dbm "github.com/tendermint/tm-db"
+
+	abci "github.com/tendermint/tendermint/abci/types"
+	"github.com/tendermint/tendermint/crypto/merkle"
+	"github.com/tendermint/tendermint/libs/bytes"
+	lcmock "github.com/tendermint/tendermint/light/rpc/mocks"
+	tmcrypto "github.com/tendermint/tendermint/proto/tendermint/crypto"
+	rpcmock "github.com/tendermint/tendermint/rpc/client/mocks"
+	ctypes "github.com/tendermint/tendermint/rpc/core/types"
+	"github.com/tendermint/tendermint/types"
+)
+
+// TestABCIQuery tests ABCIQuery requests and verifies proofs. HAPPY PATH 😀
+func TestABCIQuery(t *testing.T) {
+	tree, err := iavl.NewMutableTree(dbm.NewMemDB(), 100)
+	require.NoError(t, err)
+
+	var (
+		key   = []byte("foo")
+		value = []byte("bar")
+	)
+	tree.Set(key, value)
+
+	commitmentProof, err := tree.GetMembershipProof(key)
+	require.NoError(t, err)
+
+	op := &testOp{
+		Spec:  ics23.IavlSpec,
+		Key:   key,
+		Proof: commitmentProof,
+	}
+
+	next := &rpcmock.Client{}
+	next.On(
+		"ABCIQueryWithOptions",
+		context.Background(),
+		mock.AnythingOfType("string"),
+		bytes.HexBytes(key),
+		mock.AnythingOfType("client.ABCIQueryOptions"),
+	).Return(&ctypes.ResultABCIQuery{
+		Response: abci.ResponseQuery{
+			Code:   0,
+			Key:    key,
+			Value:  value,
+			Height: 1,
+			ProofOps: &tmcrypto.ProofOps{
+				Ops: []tmcrypto.ProofOp{op.ProofOp()},
+			},
+		},
+	}, nil)
+
+	lc := &lcmock.LightClient{}
+	appHash, _ := hex.DecodeString("5EFD44055350B5CC34DBD26085347A9DBBE44EA192B9286A9FC107F40EA1FAC5")
+	lc.On("VerifyLightBlockAtHeight", context.Background(), int64(2), mock.AnythingOfType("time.Time")).Return(
+		&types.LightBlock{
+			SignedHeader: &types.SignedHeader{
+				Header: &types.Header{AppHash: appHash},
+			},
+		},
+		nil,
+	)
+
+	c := NewClient(next, lc,
+		KeyPathFn(func(_ string, key []byte) (merkle.KeyPath, error) {
+			kp := merkle.KeyPath{}
+			kp = kp.AppendKey(key, merkle.KeyEncodingURL)
+			return kp, nil
+		}))
+	c.RegisterOpDecoder("ics23:iavl", testOpDecoder)
+	res, err := c.ABCIQuery(context.Background(), "/store/accounts/key", key)
+	require.NoError(t, err)
+
+	assert.NotNil(t, res)
+}
+
+type testOp struct {
+	Spec  *ics23.ProofSpec
+	Key   []byte
+	Proof *ics23.CommitmentProof
+}
+
+var _ merkle.ProofOperator = testOp{}
+
+func (op testOp) GetKey() []byte {
+	return op.Key
+}
+
+func (op testOp) ProofOp() tmcrypto.ProofOp {
+	bz, err := op.Proof.Marshal()
+	if err != nil {
+		panic(err.Error())
+	}
+	return tmcrypto.ProofOp{
+		Type: "ics23:iavl",
+		Key:  op.Key,
+		Data: bz,
+	}
+}
+
+func (op testOp) Run(args [][]byte) ([][]byte, error) {
+	// calculate root from proof
+	root, err := op.Proof.Calculate()
+	if err != nil {
+		return nil, fmt.Errorf("could not calculate root for proof: %v", err)
+	}
+	// Only support an existence proof or nonexistence proof (batch proofs currently unsupported)
+	switch len(args) {
+	case 0:
+		// Args are nil, so we verify the absence of the key.
+		absent := ics23.VerifyNonMembership(op.Spec, root, op.Proof, op.Key)
+		if !absent {
+			return nil, fmt.Errorf("proof did not verify absence of key: %s", string(op.Key))
+		}
+	case 1:
+		// Args is length 1, verify existence of key with value args[0]
+		if !ics23.VerifyMembership(op.Spec, root, op.Proof, op.Key, args[0]) {
+			return nil, fmt.Errorf("proof did not verify existence of key %s with given value %x", op.Key, args[0])
+		}
+	default:
+		return nil, fmt.Errorf("args must be length 0 or 1, got: %d", len(args))
+	}
+
+	return [][]byte{root}, nil
+}
+
+func testOpDecoder(pop tmcrypto.ProofOp) (merkle.ProofOperator, error) {
+	proof := &ics23.CommitmentProof{}
+	err := proof.Unmarshal(pop.Data)
+	if err != nil {
+		return nil, err
+	}
+
+	op := testOp{
+		Key:   pop.Key,
+		Spec:  ics23.IavlSpec,
+		Proof: proof,
+	}
+	return op, nil
+}

light/rpc/mocks/light_client.go

@@ -0,0 +1,78 @@
+// Code generated by mockery v2.3.0. DO NOT EDIT.
+
+package mocks
+
+import (
+	context "context"
+
+	mock "github.com/stretchr/testify/mock"
+
+	time "time"
+
+	types "github.com/tendermint/tendermint/types"
+)
+
+// LightClient is an autogenerated mock type for the LightClient type
+type LightClient struct {
+	mock.Mock
+}
+
+// ChainID provides a mock function with given fields:
+func (_m *LightClient) ChainID() string {
+	ret := _m.Called()
+
+	var r0 string
+	if rf, ok := ret.Get(0).(func() string); ok {
+		r0 = rf()
+	} else {
+		r0 = ret.Get(0).(string)
+	}
+
+	return r0
+}
+
+// TrustedLightBlock provides a mock function with given fields: height
+func (_m *LightClient) TrustedLightBlock(height int64) (*types.LightBlock, error) {
+	ret := _m.Called(height)
+
+	var r0 *types.LightBlock
+	if rf, ok := ret.Get(0).(func(int64) *types.LightBlock); ok {
+		r0 = rf(height)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(*types.LightBlock)
+		}
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(int64) error); ok {
+		r1 = rf(height)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// VerifyLightBlockAtHeight provides a mock function with given fields: ctx, height, now
+func (_m *LightClient) VerifyLightBlockAtHeight(ctx context.Context, height int64, now time.Time) (*types.LightBlock, error) {
+	ret := _m.Called(ctx, height, now)
+
+	var r0 *types.LightBlock
+	if rf, ok := ret.Get(0).(func(context.Context, int64, time.Time) *types.LightBlock); ok {
+		r0 = rf(ctx, height, now)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(*types.LightBlock)
+		}
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(context.Context, int64, time.Time) error); ok {
+		r1 = rf(ctx, height, now)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}

light/rpc/proof.go

@@ -1,14 +0,0 @@
-package rpc
-
-import (
-	"github.com/tendermint/tendermint/crypto/merkle"
-)
-
-func defaultProofRuntime() *merkle.ProofRuntime {
-	prt := merkle.NewProofRuntime()
-	prt.RegisterOpDecoder(
-		merkle.ProofOpValue,
-		merkle.ValueOpDecoder,
-	)
-	return prt
-}

light/rpc/query_test.go

@@ -1,160 +0,0 @@
-package rpc
-
-//import (
-//	"fmt"
-//	"os"
-//	"testing"
-//	"time"
-
-//	"github.com/stretchr/testify/assert"
-//	"github.com/stretchr/testify/require"
-
-//	"github.com/tendermint/tendermint/abci/example/kvstore"
-//	"github.com/tendermint/tendermint/crypto/merkle"
-//	nm "github.com/tendermint/tendermint/node"
-//	"github.com/tendermint/tendermint/rpc/client"
-//	rpctest "github.com/tendermint/tendermint/rpc/test"
-//	"github.com/tendermint/tendermint/types"
-//)
-
-//var node *nm.Node
-//var chainID = "tendermint_test" // TODO use from config.
-////nolint:unused
-//var waitForEventTimeout = 5 * time.Second
-
-//// TODO fix tests!!
-
-//func TestMain(m *testing.M) {
-//	app := kvstore.NewKVStoreApplication()
-//	node = rpctest.StartTendermint(app)
-
-//	code := m.Run()
-
-//	rpctest.StopTendermint(node)
-//	os.Exit(code)
-//}
-
-//func kvstoreTx(k, v []byte) []byte {
-//	return []byte(fmt.Sprintf("%s=%s", k, v))
-//}
-
-//// TODO: enable it after general proof format has been adapted
-//// in abci/examples/kvstore.go
-////nolint:unused,deadcode
-//func _TestAppProofs(t *testing.T) {
-//	assert, require := assert.New(t), require.New(t)
-
-//	prt := defaultProofRuntime()
-//	cl := client.NewLocal(node)
-//	client.WaitForHeight(cl, 1, nil)
-
-//	// This sets up our trust on the node based on some past point.
-//	source := certclient.NewProvider(chainID, cl)
-//	seed, err := source.LatestFullCommit(chainID, 1, 1)
-//	require.NoError(err, "%#v", err)
-//	cert := lite.NewBaseVerifier(chainID, seed.Height(), seed.Validators)
-
-//	// Wait for tx confirmation.
-//	done := make(chan int64)
-//	go func() {
-//		evtTyp := types.EventTx
-//		_, err = client.WaitForOneEvent(cl, evtTyp, waitForEventTimeout)
-//		require.Nil(err, "%#v", err)
-//		close(done)
-//	}()
-
-//	// Submit a transaction.
-//	k := []byte("my-key")
-//	v := []byte("my-value")
-//	tx := kvstoreTx(k, v)
-//	br, err := cl.BroadcastTxCommit(tx)
-//	require.NoError(err, "%#v", err)
-//	require.EqualValues(0, br.CheckTx.Code, "%#v", br.CheckTx)
-//	require.EqualValues(0, br.DeliverTx.Code)
-//	brh := br.Height
-
-//	// Fetch latest after tx commit.
-//	<-done
-//	latest, err := source.LatestFullCommit(chainID, 1, 1<<63-1)
-//	require.NoError(err, "%#v", err)
-//	rootHash := latest.SignedHeader.AppHash
-//	if rootHash == nil {
-//		// Fetch one block later, AppHash hasn't been committed yet.
-//		// TODO find a way to avoid doing this.
-//		client.WaitForHeight(cl, latest.SignedHeader.Height+1, nil)
-//		latest, err = source.LatestFullCommit(chainID, latest.SignedHeader.Height+1, 1<<63-1)
-//		require.NoError(err, "%#v", err)
-//		rootHash = latest.SignedHeader.AppHash
-//	}
-//	require.NotNil(rootHash)
-
-//	// verify a query before the tx block has no data (and valid non-exist proof)
-//	bs, height, proof, err := GetWithProof(prt, k, brh-1, cl, cert)
-//	require.NoError(err, "%#v", err)
-//	require.NotNil(proof)
-//	require.Equal(height, brh-1)
-//	// require.NotNil(proof)
-//	// TODO: Ensure that *some* keys will be there, ensuring that proof is nil,
-//	// (currently there's a race condition)
-//	// and ensure that proof proves absence of k.
-//	require.Nil(bs)
-
-//	// but given that block it is good
-//	bs, height, proof, err = GetWithProof(prt, k, brh, cl, cert)
-//	require.NoError(err, "%#v", err)
-//	require.NotNil(proof)
-//	require.Equal(height, brh)
-
-//	assert.EqualValues(v, bs)
-//	err = prt.VerifyValue(proof, rootHash, string(k), bs) // XXX key encoding
-//	assert.NoError(err, "%#v", err)
-
-//	// Test non-existing key.
-//	missing := []byte("my-missing-key")
-//	bs, _, proof, err = GetWithProof(prt, missing, 0, cl, cert)
-//	require.NoError(err)
-//	require.Nil(bs)
-//	require.NotNil(proof)
-//	err = prt.VerifyAbsence(proof, rootHash, string(missing)) // XXX VerifyAbsence(), keyencoding
-//	assert.NoError(err, "%#v", err)
-//	err = prt.VerifyAbsence(proof, rootHash, string(k)) // XXX VerifyAbsence(), keyencoding
-//	assert.Error(err, "%#v", err)
-//}
-
-//func TestTxProofs(t *testing.T) {
-//	assert, require := assert.New(t), require.New(t)
-
-//	cl := client.NewLocal(node)
-//	client.WaitForHeight(cl, 1, nil)
-
-//	tx := kvstoreTx([]byte("key-a"), []byte("value-a"))
-//	br, err := cl.BroadcastTxCommit(tx)
-//	require.NoError(err, "%#v", err)
-//	require.EqualValues(0, br.CheckTx.Code, "%#v", br.CheckTx)
-//	require.EqualValues(0, br.DeliverTx.Code)
-//	brh := br.Height
-
-//	source := certclient.NewProvider(chainID, cl)
-//	seed, err := source.LatestFullCommit(chainID, brh-2, brh-2)
-//	require.NoError(err, "%#v", err)
-//	cert := lite.NewBaseVerifier(chainID, seed.Height(), seed.Validators)
-
-//	// First let's make sure a bogus transaction hash returns a valid non-existence proof.
-//	key := types.Tx([]byte("bogus")).Hash()
-//	_, err = cl.Tx(key, true)
-//	require.NotNil(err)
-//	require.Contains(err.Error(), "not found")
-
-//	// Now let's check with the real tx root hash.
-//	key = types.Tx(tx).Hash()
-//	res, err := cl.Tx(key, true)
-//	require.NoError(err, "%#v", err)
-//	require.NotNil(res)
-//	keyHash := merkle.SimpleHashFromByteSlices([][]byte{key})
-//	err = res.Proof.Validate(keyHash)
-//	assert.NoError(err, "%#v", err)
-
-//	commit, err := GetCertifiedCommit(br.Height, cl, cert)
-//	require.Nil(err, "%#v", err)
-//	require.Equal(res.Proof.RootHash, commit.Header.DataHash)
-//}