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

Efficient Consensus State Iteration #125

Merged
merged 12 commits into from
Apr 23, 2021
6 changes: 6 additions & 0 deletions modules/light-clients/07-tendermint/types/consensus_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ func (cs ConsensusState) GetTimestamp() uint64 {
return uint64(cs.Timestamp.UnixNano())
}

// IsExpired returns whether this consensus state is expired given the trusting period and current timestamp
func (cs ConsensusState) IsExpired(trustingPeriod time.Duration, now time.Time) bool {
expirationTime := cs.Timestamp.Add(trustingPeriod)
return !expirationTime.After(now)
}
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved

// ValidateBasic defines a basic validation for the tendermint consensus state.
// NOTE: ProcessedTimestamp may be zero if this is an initial consensus state passed in by relayer
// as opposed to a consensus state constructed by the chain.
Expand Down
165 changes: 163 additions & 2 deletions modules/light-clients/07-tendermint/types/store.go
Original file line number Diff line number Diff line change
@@ -1,18 +1,25 @@
package types

import (
"encoding/binary"
"strings"

"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/store/prefix"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
host "github.com/cosmos/ibc-go/modules/core/24-host"
"github.com/cosmos/ibc-go/modules/core/exported"
)

// KeyProcessedTime is appended to consensus state key to store the processed time
var KeyProcessedTime = []byte("/processedTime")
const KeyIterateConsensusStatePrefix = "iterateConsensusStates"

var (
// KeyProcessedTime is appended to consensus state key to store the processed time
KeyProcessedTime = []byte("/processedTime")
KeyIteration = []byte("/iterationKey")
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
)

// SetConsensusState stores the consensus state at the given height.
func SetConsensusState(clientStore sdk.KVStore, cdc codec.BinaryMarshaler, consensusState *ConsensusState, height exported.Height) {
Expand Down Expand Up @@ -48,6 +55,12 @@ func GetConsensusState(store sdk.KVStore, cdc codec.BinaryMarshaler, height expo
return consensusState, nil
}

// deleteConsensusState deletes the consensus state at the given height
func deleteConsensusState(clientStore sdk.KVStore, height exported.Height) {
key := host.ConsensusStateKey(height)
clientStore.Delete(key)
}

// IterateProcessedTime iterates through the prefix store and applies the callback.
// If the cb returns true, then iterator will close and stop.
func IterateProcessedTime(store sdk.KVStore, cb func(key, val []byte) bool) {
Expand Down Expand Up @@ -94,3 +107,151 @@ func GetProcessedTime(clientStore sdk.KVStore, height exported.Height) (uint64,
}
return sdk.BigEndianToUint64(bz), true
}

// deleteProcessedTime deletes the processedTime for a given height
func deleteProcessedTime(clientStore sdk.KVStore, height exported.Height) {
key := ProcessedTimeKey(height)
clientStore.Delete(key)
}

// Iteration Code

// IterationKey returns the key under which the consensus state key will be stored.
// The iteration key is a BigEndian representation of the consensus state key to support efficient iteration.
func IterationKey(height exported.Height) []byte {
heightBytes := bigEndianHeightBytes(height)
return append([]byte(KeyIterateConsensusStatePrefix), heightBytes...)
}

// SetIterationKey stores the consensus state key under a key that is more efficient for ordered iteration
func SetIterationKey(clientStore sdk.KVStore, height exported.Height) {
key := IterationKey(height)
val := host.ConsensusStateKey(height)
clientStore.Set(key, val)
}

// deleteIterationKey deletes the iteration key for a given height
func deleteIterationKey(clientStore sdk.KVStore, height exported.Height) {
key := IterationKey(height)
clientStore.Delete(key)
}

// GetHeightFromIterationKey takes an iteration key and returns the height that it references
func GetHeightFromIterationKey(iterKey []byte) exported.Height {
bigEndianBytes := iterKey[len([]byte(KeyIterateConsensusStatePrefix)):]
revisionBytes := bigEndianBytes[0:8]
heightBytes := bigEndianBytes[8:]
revision := binary.BigEndian.Uint64(revisionBytes)
height := binary.BigEndian.Uint64(heightBytes)
return clienttypes.NewHeight(revision, height)
}

func IterateConsensusStateAscending(clientStore sdk.KVStore, cb func(height exported.Height) (stop bool)) error {

iterator := sdk.KVStorePrefixIterator(clientStore, []byte(KeyIterateConsensusStatePrefix))
defer iterator.Close()

for ; iterator.Valid(); iterator.Next() {
iterKey := iterator.Key()
height := GetHeightFromIterationKey(iterKey)
if cb(height) {
return nil
}
}
return nil
}

func IterateConsensusStateDescending(clientStore sdk.KVStore, cdc codec.BinaryMarshaler,
cb func(cs ConsensusState) (stop bool)) error {

iterator := sdk.KVStorePrefixIterator(clientStore, []byte(KeyIterateConsensusStatePrefix))
defer iterator.Close()

for ; iterator.Valid(); iterator.Next() {
csKey := iterator.Value()
bz := clientStore.Get(csKey)

consensusStateI, err := clienttypes.UnmarshalConsensusState(cdc, bz)
if err != nil {
return sdkerrors.Wrapf(clienttypes.ErrInvalidConsensus, "unmarshal error: %v", err)
}

consensusState, ok := consensusStateI.(*ConsensusState)
if !ok {
return sdkerrors.Wrapf(
clienttypes.ErrInvalidConsensus,
"invalid consensus type %T, expected %T", consensusState, &ConsensusState{},
)
}

if cb(*consensusState) {
break
}
}
return nil
}
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved

func GetNextConsensusState(clientStore sdk.KVStore, cdc codec.BinaryMarshaler, height exported.Height) (*ConsensusState, bool) {
iterateStore := prefix.NewStore(clientStore, []byte(KeyIterateConsensusStatePrefix))
iterator := iterateStore.Iterator(bigEndianHeightBytes(height), nil)
defer iterator.Close()
// ignore the consensus state at current height and get next height
iterator.Next()
if !iterator.Valid() {
return nil, false
}

csKey := iterator.Value()
bz := clientStore.Get(csKey)
if bz == nil {
return nil, false
}

consensusStateI, err := clienttypes.UnmarshalConsensusState(cdc, bz)
if err != nil {
return nil, false
}

consensusState, ok := consensusStateI.(*ConsensusState)
if !ok {
return nil, false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

could we de-duplicate this?

Copy link
Contributor

Choose a reason for hiding this comment

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

these errors are also missing codecov, but I believe de duplicating logic from the existing GetConsensusState function could allow reusal of that test to ensure codecov


return consensusState, true
}

func GetPreviousConsensusState(clientStore sdk.KVStore, cdc codec.BinaryMarshaler, height exported.Height) (*ConsensusState, bool) {
iterateStore := prefix.NewStore(clientStore, []byte(KeyIterateConsensusStatePrefix))
iterator := iterateStore.ReverseIterator(nil, bigEndianHeightBytes(height))
defer iterator.Close()

if !iterator.Valid() {
return nil, false
}
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved

csKey := iterator.Value()
bz := clientStore.Get(csKey)
if bz == nil {
return nil, false
}

consensusStateI, err := clienttypes.UnmarshalConsensusState(cdc, bz)
if err != nil {
return nil, false
}

consensusState, ok := consensusStateI.(*ConsensusState)
if !ok {
return nil, false
}

return consensusState, true
}

func bigEndianHeightBytes(height exported.Height) []byte {
revisionBytes := make([]byte, 8)
heightBytes := make([]byte, 8)
binary.BigEndian.PutUint64(revisionBytes, height.GetRevisionNumber())
binary.BigEndian.PutUint64(heightBytes, height.GetRevisionHeight())
return append(revisionBytes, heightBytes...)
}
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
76 changes: 76 additions & 0 deletions modules/light-clients/07-tendermint/types/store_test.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package types_test

import (
math "math"
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
"time"

clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
commitmenttypes "github.com/cosmos/ibc-go/modules/core/23-commitment/types"
host "github.com/cosmos/ibc-go/modules/core/24-host"
"github.com/cosmos/ibc-go/modules/core/exported"
solomachinetypes "github.com/cosmos/ibc-go/modules/light-clients/06-solomachine/types"
Expand Down Expand Up @@ -111,3 +115,75 @@ func (suite *TendermintTestSuite) TestGetProcessedTime() {
_, ok = types.GetProcessedTime(store, clienttypes.NewHeight(1, 1))
suite.Require().False(ok, "retrieved processed time for a non-existent consensus state")
}

func (suite *TendermintTestSuite) TestIterationKey() {
testHeights := []exported.Height{
clienttypes.NewHeight(0, 1),
clienttypes.NewHeight(0, 1234),
clienttypes.NewHeight(7890, 4321),
clienttypes.NewHeight(math.MaxUint64, math.MaxUint64),
}
for _, h := range testHeights {
k := types.IterationKey(h)
retrievedHeight := types.GetHeightFromIterationKey(k)
suite.Require().Equal(h, retrievedHeight, "retrieving height from iteration key failed")
}
}

func (suite *TendermintTestSuite) TestIterateConsensusStates() {
nextValsHash := []byte("nextVals")

// Set iteration keys and consensus states
types.SetIterationKey(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), clienttypes.NewHeight(0, 1))
suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), "testClient", clienttypes.NewHeight(0, 1), types.NewConsensusState(time.Now(), commitmenttypes.NewMerkleRoot([]byte("hash0-1")), nextValsHash))
types.SetIterationKey(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), clienttypes.NewHeight(4, 9))
suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), "testClient", clienttypes.NewHeight(4, 9), types.NewConsensusState(time.Now(), commitmenttypes.NewMerkleRoot([]byte("hash4-9")), nextValsHash))
types.SetIterationKey(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), clienttypes.NewHeight(0, 10))
suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), "testClient", clienttypes.NewHeight(0, 10), types.NewConsensusState(time.Now(), commitmenttypes.NewMerkleRoot([]byte("hash0-10")), nextValsHash))
types.SetIterationKey(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), clienttypes.NewHeight(0, 4))
suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), "testClient", clienttypes.NewHeight(0, 4), types.NewConsensusState(time.Now(), commitmenttypes.NewMerkleRoot([]byte("hash0-4")), nextValsHash))
types.SetIterationKey(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), clienttypes.NewHeight(40, 1))
suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), "testClient", clienttypes.NewHeight(40, 1), types.NewConsensusState(time.Now(), commitmenttypes.NewMerkleRoot([]byte("hash40-1")), nextValsHash))

var testArr []string
cb := func(height exported.Height) bool {
testArr = append(testArr, height.String())
return false
}

types.IterateConsensusStateAscending(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), cb)
expectedArr := []string{"0-1", "0-4", "0-10", "4-9", "40-1"}
suite.Require().Equal(expectedArr, testArr)
}

func (suite *TendermintTestSuite) TestGetNeighboringConsensusStates() {
nextValsHash := []byte("nextVals")
cs01 := types.NewConsensusState(time.Now().UTC(), commitmenttypes.NewMerkleRoot([]byte("hash0-1")), nextValsHash)
cs04 := types.NewConsensusState(time.Now().UTC(), commitmenttypes.NewMerkleRoot([]byte("hash0-4")), nextValsHash)
cs49 := types.NewConsensusState(time.Now().UTC(), commitmenttypes.NewMerkleRoot([]byte("hash4-9")), nextValsHash)
height01 := clienttypes.NewHeight(0, 1)
height04 := clienttypes.NewHeight(0, 4)
height49 := clienttypes.NewHeight(4, 9)

// Set iteration keys and consensus states
types.SetIterationKey(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), height01)
suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), "testClient", height01, cs01)
types.SetIterationKey(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), height04)
suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), "testClient", height04, cs04)
types.SetIterationKey(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), height49)
suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), "testClient", height49, cs49)

prevCs01, ok := types.GetPreviousConsensusState(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), suite.chainA.Codec, height01)
suite.Require().Nil(prevCs01, "consensus state exists before lowest consensus state")
suite.Require().False(ok)
prevCs49, ok := types.GetPreviousConsensusState(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), suite.chainA.Codec, height49)
suite.Require().Equal(cs04, prevCs49, "previous consensus state is not returned correctly")
suite.Require().True(ok)

nextCs01, ok := types.GetNextConsensusState(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), suite.chainA.Codec, height01)
suite.Require().Equal(cs04, nextCs01, "next consensus state not returned correctly")
suite.Require().True(ok)
nextCs49, ok := types.GetNextConsensusState(suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), "testClient"), suite.chainA.Codec, height49)
suite.Require().Nil(nextCs49, "next consensus state exists after highest consensus state")
suite.Require().False(ok)
}
28 changes: 28 additions & 0 deletions modules/light-clients/07-tendermint/types/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,34 @@ func (cs ClientState) CheckHeaderAndUpdateState(
return nil, nil, err
}

// Check the earliest consensus state to see if it is expired, if so then set the prune height
// so that we can delete consensus state and all associated metadata.
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
var (
pruneHeight exported.Height
pruneError error
)
pruneCb := func(height exported.Height) bool {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
consState, err := GetConsensusState(clientStore, cdc, height)
if err != nil {
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
pruneError = err
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 maybe we should wrap this error with some message that says "please open an issue". I wouldn't want a relayer to get this issue trying to debug it since really there'd need to be something really wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm should I just log and ignore the issue. If this is caught in wild, it would suck that no updates can happen because clientstore can't prune properly. It should still just revert to old updating logic, while we implement a fix since this is a nice-to-have and not critical for IBC functioning

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with this. How do you plan to log the error?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could have used logger in context.

I decided against this because logging instead of returning error would make it easy for this to be missed in unit tests. I'd rather just ensure our test coverage makes us confident about this, rather than implementing with mistakes and not catching them until deployment

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like to correct decision to me. There is something severely wrong if this error occurs

return true
}
if consState.IsExpired(cs.TrustingPeriod, ctx.BlockTime()) {
pruneHeight = height
}
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
return true
}
IterateConsensusStateAscending(clientStore, pruneCb)
Copy link
Member Author

Choose a reason for hiding this comment

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

This will fetch earliest consensus state, we can implement pruning now without breaking changes.

Can similarly test for monotonic time as well

if pruneError != nil {
return nil, nil, pruneError
}
// if pruneHeight is set, delete consensus state and metadata
if pruneHeight != nil {
deleteConsensusState(clientStore, pruneHeight)
deleteProcessedTime(clientStore, pruneHeight)
deleteIterationKey(clientStore, pruneHeight)
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

newClientState, consensusState := update(ctx, clientStore, &cs, tmHeader)
return newClientState, consensusState, nil
}
Expand Down