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
164 changes: 162 additions & 2 deletions modules/light-clients/07-tendermint/types/store.go
Original file line number Diff line number Diff line change
@@ -1,18 +1,42 @@
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")
/*
This file contains the logic for storage and iteration over `IterationKey` metadata that is stored
for each consensus state. The consensus state key specified in ICS-24 and expected by counterparty chains
stores the consensus state under the key: `consensusStates/{revision_number}-{revision_height}`, with each number
represented as a string.
While this works fine for IBC proof verification, it makes efficient iteration difficult since the lexicographic order
of the consensus state keys do not match the height order of consensus states. This makes consensus state pruning and
monotonic time enforcement difficult since it is inefficient to find the earliest consensus state or to find the neigboring
consensus states given a consensus state height.
Changing the ICS-24 representation will be a major breaking change that requires counterparty chains to accept a new key format.
Thus to avoid breaking IBC, we can store a lookup from a more efficiently formatted key: `iterationKey` to the consensus state key which
stores the underlying consensus state. This efficient iteration key will be formatted like so: `iterateConsensusStates{BigEndianRevisionBytes}{BigEndianHeightBytes}`.
This ensures that the lexicographic order of iteration keys match the height order of the consensus states. Thus, we can use the SDK store's
Iterators to iterate over the consensus states in ascending/descending order by providing a mapping from `iterationKey -> consensusStateKey -> ConsensusState`.
A future version of IBC may choose to replace the ICS24 ConsensusState path with the more efficient format and make this indirection unnecessary.
*/
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

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 +72,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 +124,133 @@ 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)
}

// GetIterationKey returns the consensus state key stored under the efficient iteration key.
// NOTE: This function is currently only used for testing purposes
func GetIterationKey(clientStore sdk.KVStore, height exported.Height) []byte {
key := IterationKey(height)
return clientStore.Get(key)
}

// 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
}

// GetNextConsensusState returns the lowest consensus state that is larger than the given height.
// The Iterator returns a storetypes.Iterator which iterates from start (inclusive) to end (exclusive).
// Thus, to get the next consensus state, we must first call iterator.Next() and then get the value.
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
}

// GetPreviousConsensusState returns the highest consensus state that is lower than the given height.
// The Iterator returns a storetypes.Iterator which iterates from the end (exclusive) to start (inclusive).
// Thus to get previous consensus state we call iterator.Value() immediately.
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"
"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)
}
34 changes: 34 additions & 0 deletions modules/light-clients/07-tendermint/types/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ import (
// number must be the same. To update to a new revision, use a separate upgrade path
// Tendermint client validity checking uses the bisection algorithm described
// in the [Tendermint spec](https://github.com/tendermint/spec/blob/master/spec/consensus/light-client.md).
//
// Pruning:
// UpdateClient will additionally retrieve the earliest consensus state for this clientID and check if it is expired. If it is,
// that consensus state will be pruned from store along with all associated metadata. This will prevent the client store from
// becoming bloated with expired consensus states that can no longer be used for updates and packet verification.
func (cs ClientState) CheckHeaderAndUpdateState(
ctx sdk.Context, cdc codec.BinaryMarshaler, clientStore sdk.KVStore,
header exported.Header,
Expand All @@ -60,6 +65,35 @@ 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)
// this error should never occur
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 cs.IsExpired(consState.Timestamp, ctx.BlockTime()) {
pruneHeight = height
}
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
Loading