Skip to content

Commit 4c5d247

Browse files
fix(state): Verify the state inclusion proof without splitting keys (#1483)
## Overview There has to be better way of fixing this, but I'm unsure of how we want to handle it, so submitting this for now. The bug is that, when verifying state inclusion proofs, tendermint will perform [some processing](https://github.com/celestiaorg/celestia-core/blob/a0ccbd068927b638705f63acdf6303d3091da985/crypto/merkle/proof_key_path.go#L85-L110) on the keypath that you give it that the sdk simply doesn't have to do. The iavl doesn't do this, as we just pass it the key it wants, not the path. It just sticks the key that you give in the [request right back](https://github.com/cosmos/cosmos-sdk/blob/b46d83850270b82496e4cd61421d9ebc8bac87f2/store/iavl/store.go#L324-L326), and because the processing in tendermint sometimes results in a different key, the proof verification [fails](https://github.com/celestiaorg/celestia-core/blob/a0ccbd068927b638705f63acdf6303d3091da985/crypto/merkle/proof_op.go#L52). Specifically, the string encoding of the bytes we pass it sometimes have the string `/` and `%` in them from the address, the processing tendermint does thinks its supposed to do something when it actually isn't, and then the correct key returned by the sdk no longer matches. This is why we're only seeing some addresses break! [bad processing point 1](https://github.com/celestiaorg/celestia-core/blob/a0ccbd068927b638705f63acdf6303d3091da985/crypto/merkle/proof_key_path.go#L91) [bad processing point 2](https://github.com/celestiaorg/celestia-core/blob/a0ccbd068927b638705f63acdf6303d3091da985/crypto/merkle/proof_key_path.go#L102) (idek why this is there) My quick and dirty solution was to copy paste fork ~~the file from tendermint~~ a few methods that verify proofs, then change the api so we don't have to split the string 😆 . We should fix it properly soon. This might be a bug with tendermint, but I was admittedly too tired to look into it or write up an issue. We might just need to pass a different formatted string(?) as a bonus, I added the foundation for an integration test for the core accessor, that should make it waaaaaaaayyyy easier to round trip test things! 🙂 🎄 pls feel to edit this PR and add whatever else is needed to get this across the finish line closes #1461 ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords Co-authored-by: rene <[email protected]>
1 parent 4734aac commit 4c5d247

File tree

4 files changed

+151
-7
lines changed

4 files changed

+151
-7
lines changed

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,5 +327,5 @@ replace (
327327
github.com/filecoin-project/dagstore => github.com/celestiaorg/dagstore v0.0.0-20221014072825-395797efb659
328328
github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1
329329
github.com/libp2p/go-libp2p-pubsub v0.7.0 => github.com/celestiaorg/go-libp2p-pubsub v0.6.2-0.20220812132010-46b2a019f2f2
330-
github.com/tendermint/tendermint => github.com/celestiaorg/celestia-core v1.5.0-tm-v0.34.20
330+
github.com/tendermint/tendermint => github.com/celestiaorg/celestia-core v1.5.0-tm-v0.34.20-verify-key-patch
331331
)

go.sum

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -218,8 +218,8 @@ github.com/c-bata/go-prompt v0.2.2/go.mod h1:VzqtzE2ksDBcdln8G7mk2RX9QyGjH+OVqOC
218218
github.com/casbin/casbin/v2 v2.1.2/go.mod h1:YcPU1XXisHhLzuxH9coDNf2FbKpjGlbCg3n9yuLkIJQ=
219219
github.com/celestiaorg/celestia-app v0.11.0 h1:F76mBdLJZ1LLjWEPVEjoFPJA4skaY7Wrg+5o1LVlHGY=
220220
github.com/celestiaorg/celestia-app v0.11.0/go.mod h1:6k/zcNDEgOyJRGnAgWw1VsrwTKcVjOyYG5LPTHcZR+w=
221-
github.com/celestiaorg/celestia-core v1.5.0-tm-v0.34.20 h1:BqlcOQqL2UqdDTcdCtrOLXDlmwxIA8DiKiY79oahxkQ=
222-
github.com/celestiaorg/celestia-core v1.5.0-tm-v0.34.20/go.mod h1:f4R8qNJrP1CDH0SNwj4jA3NymBLQM4lNdx6Ijmfllbw=
221+
github.com/celestiaorg/celestia-core v1.5.0-tm-v0.34.20-verify-key-patch h1:b0swFbc0JqivwVz1UwnzFPlXYsVFkoHAmJ3LIfP4/2o=
222+
github.com/celestiaorg/celestia-core v1.5.0-tm-v0.34.20-verify-key-patch/go.mod h1:f4R8qNJrP1CDH0SNwj4jA3NymBLQM4lNdx6Ijmfllbw=
223223
github.com/celestiaorg/cosmos-sdk v1.4.0-sdk-v0.46.0 h1:65gnQ92mfz+9XNVTHeVwMp+SZuBqmToEnz8+WdDRmQ8=
224224
github.com/celestiaorg/cosmos-sdk v1.4.0-sdk-v0.46.0/go.mod h1:ByQ2rOrZs7s2OnPfeaiTMC8IOlcrT195xIRPgevydCI=
225225
github.com/celestiaorg/dagstore v0.0.0-20221014072825-395797efb659 h1:f3205vw3GYBtMiNoS+qB6IuHSs50Iwqsm9lNIikLTCk=

state/core_access.go

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,13 @@ import (
77
"time"
88

99
"github.com/cosmos/cosmos-sdk/api/tendermint/abci"
10-
"github.com/cosmos/cosmos-sdk/store/rootmulti"
10+
storetypes "github.com/cosmos/cosmos-sdk/store/types"
1111
sdktypes "github.com/cosmos/cosmos-sdk/types"
1212
sdktx "github.com/cosmos/cosmos-sdk/types/tx"
1313
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
1414
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
1515
logging "github.com/ipfs/go-log/v2"
16+
"github.com/tendermint/tendermint/crypto/merkle"
1617
rpcclient "github.com/tendermint/tendermint/rpc/client"
1718
"github.com/tendermint/tendermint/rpc/client/http"
1819
"google.golang.org/grpc"
@@ -44,6 +45,8 @@ type CoreAccessor struct {
4445
stakingCli stakingtypes.QueryClient
4546
rpcCli rpcclient.ABCIClient
4647

48+
prt *merkle.ProofRuntime
49+
4750
coreConn *grpc.ClientConn
4851
coreIP string
4952
rpcPort string
@@ -63,12 +66,17 @@ func NewCoreAccessor(
6366
rpcPort string,
6467
grpcPort string,
6568
) *CoreAccessor {
69+
// create verifier
70+
prt := merkle.DefaultProofRuntime()
71+
prt.RegisterOpDecoder(storetypes.ProofOpIAVLCommitment, storetypes.CommitmentOpDecoder)
72+
prt.RegisterOpDecoder(storetypes.ProofOpSimpleMerkleCommitment, storetypes.CommitmentOpDecoder)
6673
return &CoreAccessor{
6774
signer: signer,
6875
getter: getter,
6976
coreIP: coreIP,
7077
rpcPort: rpcPort,
7178
grpcPort: grpcPort,
79+
prt: prt,
7280
}
7381
}
7482

@@ -97,6 +105,7 @@ func (ca *CoreAccessor) Start(ctx context.Context) error {
97105
return err
98106
}
99107
ca.rpcCli = cli
108+
100109
return nil
101110
}
102111

@@ -221,9 +230,12 @@ func (ca *CoreAccessor) BalanceForAddress(ctx context.Context, addr Address) (*B
221230
return nil, fmt.Errorf("cannot convert %s into sdktypes.Int", string(value))
222231
}
223232
// verify balance
224-
path := fmt.Sprintf("/%s/%s", banktypes.StoreKey, string(prefixedAccountKey))
225-
prt := rootmulti.DefaultProofRuntime()
226-
err = prt.VerifyValue(result.Response.GetProofOps(), head.AppHash, path, value)
233+
err = ca.prt.VerifyValueKeys(
234+
result.Response.GetProofOps(),
235+
head.AppHash,
236+
[][]byte{[]byte(banktypes.StoreKey),
237+
prefixedAccountKey,
238+
}, value)
227239
if err != nil {
228240
return nil, err
229241
}

state/integration_test.go

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
package state
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
sdk "github.com/cosmos/cosmos-sdk/types"
8+
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
9+
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
10+
"github.com/stretchr/testify/require"
11+
"github.com/stretchr/testify/suite"
12+
tmrand "github.com/tendermint/tendermint/libs/rand"
13+
rpcclient "github.com/tendermint/tendermint/rpc/client"
14+
"google.golang.org/grpc"
15+
16+
"github.com/celestiaorg/celestia-app/app"
17+
"github.com/celestiaorg/celestia-app/testutil/testnode"
18+
blobtypes "github.com/celestiaorg/celestia-app/x/payment/types"
19+
20+
"github.com/celestiaorg/celestia-node/header"
21+
)
22+
23+
func TestIntegrationTestSuite(t *testing.T) {
24+
suite.Run(t, new(IntegrationTestSuite))
25+
}
26+
27+
type IntegrationTestSuite struct {
28+
suite.Suite
29+
30+
cleanups []func() error
31+
accounts []string
32+
cctx testnode.Context
33+
34+
accessor *CoreAccessor
35+
}
36+
37+
func (s *IntegrationTestSuite) SetupSuite() {
38+
if testing.Short() {
39+
s.T().Skip("skipping test in unit-tests or race-detector mode.")
40+
}
41+
42+
s.T().Log("setting up integration test suite")
43+
require := s.Require()
44+
45+
// we create an arbitrary number of funded accounts
46+
for i := 0; i < 25; i++ {
47+
s.accounts = append(s.accounts, tmrand.Str(9))
48+
}
49+
50+
tmNode, app, cctx, err := testnode.New(
51+
s.T(),
52+
testnode.DefaultParams(),
53+
testnode.DefaultTendermintConfig(),
54+
false,
55+
s.accounts...,
56+
)
57+
require.NoError(err)
58+
59+
cctx, stopNode, err := testnode.StartNode(tmNode, cctx)
60+
require.NoError(err)
61+
s.cleanups = append(s.cleanups, stopNode)
62+
63+
cctx, cleanupGRPC, err := testnode.StartGRPCServer(app, testnode.DefaultAppConfig(), cctx)
64+
require.NoError(err)
65+
s.cleanups = append(s.cleanups, cleanupGRPC)
66+
67+
s.cctx = cctx
68+
require.NoError(cctx.WaitForNextBlock())
69+
70+
signer := blobtypes.NewKeyringSigner(s.cctx.Keyring, s.accounts[0], cctx.ChainID)
71+
72+
accessor := NewCoreAccessor(signer, localHeader{s.cctx.Client}, "", "", "")
73+
setClients(accessor, s.cctx.GRPCClient, s.cctx.Client)
74+
s.accessor = accessor
75+
}
76+
77+
func setClients(ca *CoreAccessor, conn *grpc.ClientConn, abciCli rpcclient.ABCIClient) {
78+
ca.coreConn = conn
79+
// create the query client
80+
queryCli := banktypes.NewQueryClient(ca.coreConn)
81+
ca.queryCli = queryCli
82+
// create the staking query client
83+
stakingCli := stakingtypes.NewQueryClient(ca.coreConn)
84+
ca.stakingCli = stakingCli
85+
86+
ca.rpcCli = abciCli
87+
}
88+
89+
func (s *IntegrationTestSuite) TearDownSuite() {
90+
s.T().Log("tearing down integration test suite")
91+
require := s.Require()
92+
require.NoError(s.accessor.Stop(s.cctx.GoContext()))
93+
for _, c := range s.cleanups {
94+
err := c()
95+
require.NoError(err)
96+
}
97+
}
98+
99+
func (s *IntegrationTestSuite) getAddress(acc string) sdk.Address {
100+
rec, err := s.cctx.Keyring.Key(acc)
101+
require.NoError(s.T(), err)
102+
103+
addr, err := rec.GetAddress()
104+
require.NoError(s.T(), err)
105+
106+
return addr
107+
}
108+
109+
type localHeader struct {
110+
client rpcclient.Client
111+
}
112+
113+
func (l localHeader) Head(ctx context.Context) (*header.ExtendedHeader, error) {
114+
latest, err := l.client.Block(ctx, nil)
115+
if err != nil {
116+
return nil, err
117+
}
118+
h := &header.ExtendedHeader{
119+
RawHeader: latest.Block.Header,
120+
}
121+
return h, nil
122+
}
123+
124+
func (s *IntegrationTestSuite) TestGetBalance() {
125+
require := s.Require()
126+
expectedBal := sdk.NewCoin(app.BondDenom, sdk.NewInt(int64(99999999999999999)))
127+
for _, acc := range s.accounts {
128+
bal, err := s.accessor.BalanceForAddress(context.Background(), s.getAddress(acc))
129+
require.NoError(err)
130+
require.Equal(&expectedBal, bal)
131+
}
132+
}

0 commit comments

Comments
 (0)