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

Store historic header information for IBC #93

Merged
merged 5 commits into from
Aug 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ func NewTgradeApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest

// Create IBC Keeper
app.ibcKeeper = ibckeeper.NewKeeper(
appCodec, keys[ibchost.StoreKey], app.getSubspace(ibchost.ModuleName), stakingKeeper, scopedIBCKeeper,
appCodec, keys[ibchost.StoreKey], app.getSubspace(ibchost.ModuleName), &app.poeKeeper, scopedIBCKeeper,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah.. and now it has state once again 😄

But... how can app.poeKeeper be used on line 272 when it is set in 352? This smells funny

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A ref is parsed to the ibc keeper that is updated later. I will add a test that provides more confidence

Copy link
Contributor

Choose a reason for hiding this comment

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

A ref is passed to app.poeKeeper (probably the zero value).

Later we set app.poeKeeper = &stakingtypes.Adaptor{} or something like that, which does not update the other reference.

If you did *app.poeKeeper = staking types.Adaptor{}, then this would update the data that was pointed at by the same reference app.ibcKeeper has.

At least this is my understanding of pointers. As it is now, it points into space. This is also a bit harder to reason about when most of these are just empty structs, and return errors on all calls... when there is real state and logic, it will be most difficult

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note app.poeKeeper is of type poekeeper.Keeper not a pointer.
When I pass the ref here it points to the empty (not nil) app.poeKeeper memory address. When the poeKeeper is instantiated, it is still the same memory address.
https://play.golang.org/p/y9VGNrLUUVC

Copy link
Contributor

Choose a reason for hiding this comment

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

I need to look into this...

Copy link
Contributor

Choose a reason for hiding this comment

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

That didn't quite convince me, so I played with it a bit more, to replicate more precisely how we use pointers: https://play.golang.org/p/xBlHLRh_ye_K

And that still matches your expected behaviour.

I guess I really don't understand how all these pointers and interfaces are implemented under the hood in Go, but your code is correct in spite of my confusion.

)

twasmConfig, err := twasm.ReadWasmConfig(appOpts)
Expand Down Expand Up @@ -349,7 +349,7 @@ func NewTgradeApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest
govRouter,
)

app.poeKeeper = poekeeper.NewKeeper(appCodec, keys[poe.StoreKey])
app.poeKeeper = poekeeper.NewKeeper(appCodec, keys[poe.StoreKey], app.getSubspace(poe.ModuleName), app.twasmKeeper)
/**** Module Options ****/

// NOTE: we may consider parsing `appOpts` inside module constructors. For the moment
Expand Down Expand Up @@ -381,6 +381,7 @@ func NewTgradeApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest
// CanWithdrawInvariant invariant.
// NOTE: staking module is required if HistoricalEntries param > 0
app.mm.SetOrderBeginBlockers(
poe.ModuleName,
upgradetypes.ModuleName,
evidencetypes.ModuleName, ibchost.ModuleName,
twasm.ModuleName,
Expand Down Expand Up @@ -600,6 +601,7 @@ func initParamsKeeper(appCodec codec.BinaryMarshaler, legacyAmino *codec.LegacyA
paramsKeeper.Subspace(ibchost.ModuleName)
paramsKeeper.Subspace(twasm.ModuleName)
paramsKeeper.Subspace(globalfee.ModuleName)
paramsKeeper.Subspace(poe.ModuleName)

return paramsKeeper
}
43 changes: 42 additions & 1 deletion app/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
ibcclienttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types"
"github.com/stretchr/testify/assert"
"github.com/tendermint/tendermint/libs/rand"
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
"os"
"testing"
"time"
Expand All @@ -22,7 +25,6 @@ import (
var emptyWasmOpts []wasm.Option = nil

func TestTgradeExport(t *testing.T) {
t.Skip("Alex, this is not implemented")
db := db.NewMemDB()
gapp := NewTgradeApp(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, nil, true, map[int64]bool{}, DefaultNodeHome, 0, EmptyBaseAppOptions{}, emptyWasmOpts)
genesisState := NewDefaultGenesisState()
Expand All @@ -44,6 +46,8 @@ func TestTgradeExport(t *testing.T) {

// Making a new app object with the db, so that initchain hasn't been called
newGapp := NewTgradeApp(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, nil, true, map[int64]bool{}, DefaultNodeHome, 0, EmptyBaseAppOptions{}, emptyWasmOpts)

t.Skip("Alex, export is not implemented")
_, err = newGapp.ExportAppStateAndValidators(false, []string{})
require.NoError(t, err, "ExportAppStateAndValidators should not have an error")
}
Expand Down Expand Up @@ -119,3 +123,40 @@ func setGenesis(gapp *TgradeApp) error {
gapp.Commit()
return nil
}

func TestIBCKeeperLazyInitialization(t *testing.T) {
db := db.NewMemDB()
gapp := NewTgradeApp(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, nil, true, map[int64]bool{}, DefaultNodeHome, 0, EmptyBaseAppOptions{}, emptyWasmOpts)
genesisState := NewDefaultGenesisState()

setupWithSingleValidatorGenTX(t, genesisState)

stateBytes, err := json.MarshalIndent(genesisState, "", " ")
require.NoError(t, err)

// Initialize the chain
now := time.Now().UTC()
gapp.InitChain(
abci.RequestInitChain{
Time: now,
Validators: []abci.ValidatorUpdate{},
AppStateBytes: stateBytes,
},
)
gapp.Commit()
// store some historic information
header := tmproto.Header{ChainID: "testing-1", Height: 2, Time: now, AppHash: []byte("myAppHash")}
gapp.BaseApp.BeginBlock(abci.RequestBeginBlock{Header: header})
gapp.Commit()

ctx := gapp.BaseApp.NewContext(true, header)
height := ibcclienttypes.Height{RevisionNumber: 1, RevisionHeight: 2}

// when
// https://github.com/cosmos/cosmos-sdk/blob/v0.42.9/x/ibc/core/02-client/keeper/keeper.go#L252
state, found := gapp.ibcKeeper.ClientKeeper.GetSelfConsensusState(ctx, height)
Copy link
Contributor

Choose a reason for hiding this comment

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

This uses the staking adaptor? Can you link to where that call happens?

Can we assert that data that we expect to be returned from such an adaptor (unbonding period?) is actually set properly? Not just that it returns "some data"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have extended the test to verify that the data returned is what was persisted before on begin block

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you. Looks convincing.

// then
require.True(t, found)
assert.Equal(t, []byte("myAppHash"), state.GetRoot().GetHash())
assert.Equal(t, uint64(now.UnixNano()), state.GetTimestamp())
}
18 changes: 18 additions & 0 deletions docs/proto/proto-docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
- [Query](#confio.globalfee.v1beta1.Query)

- [confio/poe/v1beta1/poe.proto](#confio/poe/v1beta1/poe.proto)
- [Params](#confio.poe.v1beta1.Params)

- [PoEContractType](#confio.poe.v1beta1.PoEContractType)

- [confio/poe/v1beta1/genesis.proto](#confio/poe/v1beta1/genesis.proto)
Expand Down Expand Up @@ -166,6 +168,21 @@ Query defines the gRPC querier service.
## confio/poe/v1beta1/poe.proto



<a name="confio.poe.v1beta1.Params"></a>

### Params
Params defines the parameters for the PoE module.


| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `historical_entries` | [uint32](#uint32) | | historical_entries is the number of historical entries to persist. |





<!-- end messages -->


Expand Down Expand Up @@ -206,6 +223,7 @@ GenesisState - initial state of module

| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `params` | [Params](#confio.poe.v1beta1.Params) | | params defines all the parameter of the module |
| `seed_contracts` | [bool](#bool) | | SeedContracts when enabled stores and instantiates the Proof of Engagement contracts on the chain. |
| `gen_txs` | [bytes](#bytes) | repeated | GenTxs defines the genesis transactions to create a validator. |
| `system_admin_address` | [string](#string) | | SystemAdminAddress single address that is set as admin for the PoE contracts in seed mode. |
Expand Down
16 changes: 10 additions & 6 deletions proto/confio/poe/v1beta1/genesis.proto
Original file line number Diff line number Diff line change
Expand Up @@ -10,37 +10,41 @@ option go_package = "github.com/confio/tgrade/x/poe/types";

// GenesisState - initial state of module
message GenesisState {

// params defines all the parameter of the module
Params params = 1 [ (gogoproto.nullable) = false ];

// SeedContracts when enabled stores and instantiates the Proof of Engagement
// contracts on the chain.
bool seed_contracts = 1;
bool seed_contracts = 2;

// GenTxs defines the genesis transactions to create a validator.
repeated bytes gen_txs = 2 [
repeated bytes gen_txs = 3 [
(gogoproto.casttype) = "encoding/json.RawMessage",
(gogoproto.jsontag) = "gentxs",
(gogoproto.moretags) = "yaml:\"gentxs\""
];

// SystemAdminAddress single address that is set as admin for the PoE
// contracts in seed mode.
string system_admin_address = 3;
string system_admin_address = 4;

// Contracts Poe contract addresses and types when used with state dump in non
// seed mode.
repeated PoEContract contracts = 4 [
repeated PoEContract contracts = 5 [
(gogoproto.nullable) = false,
(gogoproto.jsontag) = "contracts,omitempty"
];

// Engagement weighted members of the engagement group. Validators should be
// in here.
repeated TG4Member engagement = 5 [
repeated TG4Member engagement = 6 [
(gogoproto.nullable) = false,
(gogoproto.jsontag) = "engagement,omitempty"
];

// BondDenom defines the bondable coin denomination.
string bond_denom = 6 [ (gogoproto.moretags) = "yaml:\"bond_denom\"" ];
string bond_denom = 7 [ (gogoproto.moretags) = "yaml:\"bond_denom\"" ];
}

// PoEContract address and type information
Expand Down
9 changes: 9 additions & 0 deletions proto/confio/poe/v1beta1/poe.proto
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,12 @@ enum PoEContractType {
[ (gogoproto.enumvalue_customname) = "PoEContractTypeEngagement" ];
MIXER = 4 [ (gogoproto.enumvalue_customname) = "PoEContractTypeMixer" ];
}

// Params defines the parameters for the PoE module.
message Params {
option (gogoproto.equal) = true;
option (gogoproto.goproto_stringer) = false;
// historical_entries is the number of historical entries to persist.
uint32 historical_entries = 4
[ (gogoproto.moretags) = "yaml:\"historical_entries\"" ];
}
10 changes: 8 additions & 2 deletions x/poe/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@ import (
"time"
)

type abciKeeper interface {
type endBlockKeeper interface {
types.Sudoer
IteratePrivilegedContractsByType(ctx sdk.Context, privilegeType twasmtypes.PrivilegeType, cb func(prio uint8, contractAddr sdk.AccAddress) bool)
}

// EndBlocker calls the Valset contract for the validator diff.
func EndBlocker(parentCtx sdk.Context, k abciKeeper) []abci.ValidatorUpdate {
func EndBlocker(parentCtx sdk.Context, k endBlockKeeper) []abci.ValidatorUpdate {
defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyEndBlocker)
logger := keeper.ModuleLogger(parentCtx)

Expand All @@ -40,3 +40,9 @@ func EndBlocker(parentCtx sdk.Context, k abciKeeper) []abci.ValidatorUpdate {
})
return diff
}

func BeginBlocker(ctx sdk.Context, k interface{ TrackHistoricalInfo(ctx sdk.Context) }) {
defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker)

k.TrackHistoricalInfo(ctx)
}
2 changes: 2 additions & 0 deletions x/poe/keeper/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ type DeliverTxFn func(abci.RequestDeliverTx) abci.ResponseDeliverTx
type initer interface {
SetPoEContractAddress(ctx sdk.Context, ctype types.PoEContractType, contractAddr sdk.AccAddress)
setPoESystemAdminAddress(ctx sdk.Context, admin sdk.AccAddress)
setParams(ctx sdk.Context, params types.Params)
}

// InitGenesis - initialize accounts and deliver genesis transactions
Expand All @@ -30,6 +31,7 @@ func InitGenesis(
// addr, _ := sdk.AccAddressFromBech32(v.Address)
// keeper.SetPoEContractAddress(ctx, v.ContractType, addr)
//}
keeper.setParams(ctx, genesisState.Params)
admin, err := sdk.AccAddressFromBech32(genesisState.SystemAdminAddress)
if err != nil {
return sdkerrors.Wrap(err, "admin")
Expand Down
7 changes: 7 additions & 0 deletions x/poe/keeper/genesis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ func TestInitGenesis(t *testing.T) {
expErr bool
expDeliveredGenTxCount int
expContracts []CapturedPoEContractAddress
expParams types.Params
}{
"all good": {
src: types.GenesisStateFixture(func(m *types.GenesisState) {
Expand All @@ -30,6 +31,7 @@ func TestInitGenesis(t *testing.T) {
},
),
expDeliveredGenTxCount: 1,
expParams: types.DefaultParams(),
},
"deliver genTX failed": {
src: types.GenesisStateFixture(func(m *types.GenesisState) {
Expand All @@ -50,11 +52,15 @@ func TestInitGenesis(t *testing.T) {
ctx := sdk.Context{}
cFn, capAddrs := CaptureSetPoEContractAddressFn()
var capturedAdminAddr sdk.AccAddress
var capaturedParams types.Params
m := PoEKeeperMock{
SetPoEContractAddressFn: cFn,
setPoESystemAdminAddressFn: func(ctx sdk.Context, admin sdk.AccAddress) {
capturedAdminAddr = admin
},
setParamsFn: func(ctx sdk.Context, params types.Params) {
capaturedParams = params
},
}
gotErr := InitGenesis(ctx, m, captureTx, spec.src, txConfig)
if spec.expErr {
Expand All @@ -65,6 +71,7 @@ func TestInitGenesis(t *testing.T) {
assert.Len(t, capturedTxs, spec.expDeliveredGenTxCount)
assert.Equal(t, spec.expContracts, *capAddrs)
assert.Equal(t, spec.src.SystemAdminAddress, capturedAdminAddr.String())
assert.Equal(t, spec.expParams, capaturedParams)
})
}
}
Expand Down
110 changes: 110 additions & 0 deletions x/poe/keeper/historical_info.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package keeper

import (
"fmt"
"github.com/confio/tgrade/x/poe/types"
sdk "github.com/cosmos/cosmos-sdk/types"
ibccoretypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
"strconv"
)

var _ ibccoretypes.StakingKeeper = &Keeper{}

// GetHistoricalInfo gets the historical info at a given height
func (k Keeper) GetHistoricalInfo(ctx sdk.Context, height int64) (stakingtypes.HistoricalInfo, bool) {
store := ctx.KVStore(k.storeKey)
key := getHistoricalInfoKey(height)

value := store.Get(key)
if value == nil {
return stakingtypes.HistoricalInfo{}, false
}

return stakingtypes.MustUnmarshalHistoricalInfo(k.marshaler, value), true
}

// SetHistoricalInfo sets the historical info at a given height
func (k Keeper) SetHistoricalInfo(ctx sdk.Context, height int64, hi *stakingtypes.HistoricalInfo) {
store := ctx.KVStore(k.storeKey)
key := getHistoricalInfoKey(height)
value := k.marshaler.MustMarshalBinaryBare(hi)
store.Set(key, value)
}

// DeleteHistoricalInfo deletes the historical info at a given height
func (k Keeper) DeleteHistoricalInfo(ctx sdk.Context, height int64) {
store := ctx.KVStore(k.storeKey)
key := getHistoricalInfoKey(height)

store.Delete(key)
}

// iterateHistoricalInfo provides an interator over all stored HistoricalInfo
// objects. For each HistoricalInfo object, cb will be called. If the cb returns
// true, the iterator will close and stop.
func (k Keeper) iterateHistoricalInfo(ctx sdk.Context, cb func(stakingtypes.HistoricalInfo) bool) {
store := ctx.KVStore(k.storeKey)

iterator := sdk.KVStorePrefixIterator(store, types.HistoricalInfoKey)
defer iterator.Close()

for ; iterator.Valid(); iterator.Next() {
histInfo := stakingtypes.MustUnmarshalHistoricalInfo(k.marshaler, iterator.Value())
if cb(histInfo) {
break
}
}
}

// getAllHistoricalInfo returns all stored HistoricalInfo objects.
func (k Keeper) getAllHistoricalInfo(ctx sdk.Context) []stakingtypes.HistoricalInfo {
var infos []stakingtypes.HistoricalInfo

k.iterateHistoricalInfo(ctx, func(histInfo stakingtypes.HistoricalInfo) bool {
infos = append(infos, histInfo)
return false
})

return infos
}

// TrackHistoricalInfo saves the latest historical-info and deletes the oldest
// heights that are below pruning height
func (k Keeper) TrackHistoricalInfo(ctx sdk.Context) {
alpe marked this conversation as resolved.
Show resolved Hide resolved
entryNum := k.HistoricalEntries(ctx)

// Prune store to ensure we only have parameter-defined historical entries.
// In most cases, this will involve removing a single historical entry.
// In the rare scenario when the historical entries gets reduced to a lower value k'
// from the original value k. k - k' entries must be deleted from the store.
// Since the entries to be deleted are always in a continuous range, we can iterate
// over the historical entries starting from the most recent version to be pruned
// and then return at the first empty entry.
fmt.Printf("++ height: %d", ctx.BlockHeight())
for i := ctx.BlockHeight() - int64(entryNum); i >= 0; i-- {
_, found := k.GetHistoricalInfo(ctx, i)
if found {
k.DeleteHistoricalInfo(ctx, i)
} else {
break
}
}

// if there is no need to persist historicalInfo, return
if entryNum == 0 {
alpe marked this conversation as resolved.
Show resolved Hide resolved
return
}

// Create HistoricalInfo struct
var valSet stakingtypes.Validators // not used by IBC so we keep it empty
historicalEntry := stakingtypes.NewHistoricalInfo(ctx.BlockHeader(), valSet)

// Set latest HistoricalInfo at current height
k.SetHistoricalInfo(ctx, ctx.BlockHeight(), &historicalEntry)
}

// getHistoricalInfoKey returns a key prefix for indexing HistoricalInfo objects.
func getHistoricalInfoKey(height int64) []byte {
return append(types.HistoricalInfoKey, []byte(strconv.FormatInt(height, 10))...)
}
Loading