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

refactor: deprecate Voteinfo in favour of Cometinfo on Context #17670

Merged
merged 19 commits into from
Sep 14, 2023
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (rpc) [#17470](https://github.com/cosmos/cosmos-sdk/pull/17470) Avoid open 0.0.0.0 to public by default and add `listen-ip-address` argument for `testnet init-files` cmd.
* (types/module) [#17554](https://github.com/cosmos/cosmos-sdk/pull/17554) Introduce `HasABCIGenesis` which is implemented by a module only when a validatorset update needs to be returned
* (baseapp) [#17667](https://github.com/cosmos/cosmos-sdk/pull/17667) Close databases opened by cosmos-sdk during BaseApp shutdown
* (types) [#17670](https://github.com/cosmos/cosmos-sdk/pull/17670) Use `ctx.CometInfo` in place of `ctx.VoteInfos`
* [#17733](https://github.com/cosmos/cosmos-sdk/pull/17733) Ensure `buf export` exports all proto dependencies

### Bug Fixes
Expand Down Expand Up @@ -143,6 +144,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (types) [#17426](https://github.com/cosmos/cosmos-sdk/pull/17426) `NewContext` does not take a `cmtproto.Header{}` any longer.
* `WithChainID` / `WithBlockHeight` / `WithBlockHeader` must be used to set values on the context
* (x/bank) [#17569](https://github.com/cosmos/cosmos-sdk/pull/17569) `BurnCoins` takes an address instead of a module name
* (x/distribution) [#17670](https://github.com/cosmos/cosmos-sdk/pull/17670) `AllocateTokens` takes `comet.VoteInfos` instead of `[]abci.VoteInfo`

### CLI Breaking Changes

Expand Down
23 changes: 13 additions & 10 deletions tests/integration/distribution/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@ import (
"fmt"
"testing"

cmtabcitypes "github.com/cometbft/cometbft/abci/types"
"github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/stretchr/testify/require"
"gotest.tools/v3/assert"

"cosmossdk.io/collections"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/core/comet"
"cosmossdk.io/log"
"cosmossdk.io/math"
storetypes "cosmossdk.io/store/types"
Expand Down Expand Up @@ -116,13 +115,17 @@ func initFixture(tb testing.TB) *fixture {
valConsAddr := sdk.ConsAddress(valConsPk0.Address())

// set proposer and vote infos
ctx := newCtx.WithProposer(valConsAddr).WithVoteInfos([]cmtabcitypes.VoteInfo{
{
Validator: cmtabcitypes.Validator{
Address: valAddr,
Power: 100,
ctx := newCtx.WithProposer(valConsAddr).WithCometInfo(comet.Info{
LastCommit: comet.CommitInfo{
Votes: []comet.VoteInfo{
{
Validator: comet.Validator{
Address: valAddr,
Power: 100,
},
BlockIDFlag: comet.BlockIDFlagCommit,
},
},
BlockIdFlag: types.BlockIDFlagCommit,
},
})

Expand Down Expand Up @@ -322,8 +325,8 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) {
assert.Assert(t, prevProposerConsAddr.Empty() == false)
assert.DeepEqual(t, prevProposerConsAddr, valConsAddr)
var previousTotalPower int64
for _, voteInfo := range f.sdkCtx.VoteInfos() {
previousTotalPower += voteInfo.Validator.Power
for _, vote := range f.sdkCtx.CometInfo().LastCommit.Votes {
previousTotalPower += vote.Validator.Power
}
assert.Equal(t, previousTotalPower, int64(100))
})
Expand Down
2 changes: 1 addition & 1 deletion testutil/integration/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func (app *App) RunMsg(msg sdk.Msg, option ...Option) (*codectypes.Any, error) {

if cfg.AutomaticFinalizeBlock {
height := app.LastBlockHeight() + 1
if _, err := app.FinalizeBlock(&cmtabcitypes.RequestFinalizeBlock{Height: height}); err != nil {
if _, err := app.FinalizeBlock(&cmtabcitypes.RequestFinalizeBlock{Height: height, DecidedLastCommit: cmtabcitypes.CommitInfo{Votes: []cmtabcitypes.VoteInfo{{}}}}); err != nil {
return nil, fmt.Errorf("failed to run finalize block: %w", err)
}
}
Expand Down
32 changes: 16 additions & 16 deletions types/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,14 @@ but please do not over-use it. We try to keep all data structured
and standard additions here would be better just to add to the Context struct
*/
type Context struct {
baseCtx context.Context
ms storetypes.MultiStore
// Deprecated: Use HeaderService for height, time, and chainID and CometService for the rest
header cmtproto.Header
// Deprecated: Use HeaderService for hash
headerHash []byte
// Deprecated: Use HeaderService for chainID and CometService for the rest
chainID string
baseCtx context.Context
ms storetypes.MultiStore
header cmtproto.Header // Deprecated: Use HeaderService for height, time, and chainID and CometService for the rest
headerHash []byte // Deprecated: Use HeaderService for hash
chainID string // Deprecated: Use HeaderService for chainID and CometService for the rest
txBytes []byte
logger log.Logger
voteInfo []abci.VoteInfo
voteInfo []abci.VoteInfo // Deprecated: use Cometinfo.GetLastCommit().Votes() instead, will be removed in 0.51
gasMeter storetypes.GasMeter
blockGasMeter storetypes.GasMeter
checkTx bool
Expand All @@ -69,13 +66,15 @@ type Context struct {
type Request = Context

// Read-only accessors
func (c Context) Context() context.Context { return c.baseCtx }
func (c Context) MultiStore() storetypes.MultiStore { return c.ms }
func (c Context) BlockHeight() int64 { return c.header.Height }
func (c Context) BlockTime() time.Time { return c.header.Time }
func (c Context) ChainID() string { return c.chainID }
func (c Context) TxBytes() []byte { return c.txBytes }
func (c Context) Logger() log.Logger { return c.logger }
func (c Context) Context() context.Context { return c.baseCtx }
func (c Context) MultiStore() storetypes.MultiStore { return c.ms }
func (c Context) BlockHeight() int64 { return c.header.Height }
func (c Context) BlockTime() time.Time { return c.header.Time }
func (c Context) ChainID() string { return c.chainID }
func (c Context) TxBytes() []byte { return c.txBytes }
func (c Context) Logger() log.Logger { return c.logger }

// Deprecated: use Cometinfo.GetLastCommit().Votes() instead, will be removed after 0.51
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
func (c Context) VoteInfos() []abci.VoteInfo { return c.voteInfo }
func (c Context) GasMeter() storetypes.GasMeter { return c.gasMeter }
func (c Context) BlockGasMeter() storetypes.GasMeter { return c.blockGasMeter }
Expand Down Expand Up @@ -216,6 +215,7 @@ func (c Context) WithLogger(logger log.Logger) Context {
}

// WithVoteInfos returns a Context with an updated consensus VoteInfo.
// Deprecated: use WithCometinfo() instead, will be removed after 0.51
func (c Context) WithVoteInfos(voteInfo []abci.VoteInfo) Context {
c.voteInfo = voteInfo
return c
Expand Down
6 changes: 3 additions & 3 deletions x/distribution/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@

// determine the total power signing the block
var previousTotalPower int64
for _, voteInfo := range ctx.VoteInfos() {
previousTotalPower += voteInfo.Validator.Power
for _, vote := range ctx.CometInfo().LastCommit.Votes {
previousTotalPower += vote.Validator.Power
}

// TODO this is Tendermint-dependent
// ref https://github.com/cosmos/cosmos-sdk/issues/3095
if ctx.BlockHeight() > 1 {
if err := k.AllocateTokens(ctx, previousTotalPower, ctx.VoteInfos()); err != nil {
if err := k.AllocateTokens(ctx, previousTotalPower, ctx.CometInfo().LastCommit.Votes); err != nil {

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
return err
}
}
Expand Down
6 changes: 3 additions & 3 deletions x/distribution/keeper/allocation.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@ import (
"context"
"errors"

abci "github.com/cometbft/cometbft/abci/types"

"cosmossdk.io/collections"
"cosmossdk.io/core/comet"
"cosmossdk.io/math"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand All @@ -16,7 +15,7 @@ import (

// AllocateTokens performs reward and fee distribution to all validators based
// on the F1 fee distribution specification.
func (k Keeper) AllocateTokens(ctx context.Context, totalPreviousPower int64, bondedVotes []abci.VoteInfo) error {
func (k Keeper) AllocateTokens(ctx context.Context, totalPreviousPower int64, bondedVotes []comet.VoteInfo) error {
// fetch and clear the collected fees for distribution, since this is
// called in BeginBlock, collected fees will be from the previous block
// (and distributed to the previous proposer)
Expand Down Expand Up @@ -58,6 +57,7 @@ func (k Keeper) AllocateTokens(ctx context.Context, totalPreviousPower int64, bo
//
// Ref: https://github.com/cosmos/cosmos-sdk/pull/3099#discussion_r246276376
for _, vote := range bondedVotes {

validator, err := k.stakingKeeper.ValidatorByConsAddr(ctx, vote.Validator.Address)
if err != nil {
return err
Expand Down
18 changes: 10 additions & 8 deletions x/distribution/keeper/allocation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ import (
"testing"
"time"

abci "github.com/cometbft/cometbft/abci/types"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/require"

"cosmossdk.io/collections"
"cosmossdk.io/core/comet"
"cosmossdk.io/math"
storetypes "cosmossdk.io/store/types"

Expand Down Expand Up @@ -128,11 +128,11 @@ func TestAllocateTokensToManyValidators(t *testing.T) {
val1.Commission = stakingtypes.NewCommission(math.LegacyNewDec(0), math.LegacyNewDec(0), math.LegacyNewDec(0))
stakingKeeper.EXPECT().ValidatorByConsAddr(gomock.Any(), sdk.GetConsAddress(valConsPk1)).Return(val1, nil).AnyTimes()

abciValA := abci.Validator{
abciValA := comet.Validator{
Address: valConsPk0.Address(),
Power: 100,
}
abciValB := abci.Validator{
abciValB := comet.Validator{
Address: valConsPk1.Address(),
Power: 100,
}
Expand Down Expand Up @@ -165,14 +165,15 @@ func TestAllocateTokensToManyValidators(t *testing.T) {
bankKeeper.EXPECT().GetAllBalances(gomock.Any(), feeCollectorAcc.GetAddress()).Return(fees)
bankKeeper.EXPECT().SendCoinsFromModuleToModule(gomock.Any(), "fee_collector", disttypes.ModuleName, fees)

votes := []abci.VoteInfo{
votes := []comet.VoteInfo{
{
Validator: abciValA,
},
{
Validator: abciValB,
},
}

require.NoError(t, distrKeeper.AllocateTokens(ctx, 200, votes))

// 98 outstanding rewards (100 less 2 to community pool)
Expand Down Expand Up @@ -262,15 +263,15 @@ func TestAllocateTokensTruncation(t *testing.T) {
val2.Commission = stakingtypes.NewCommission(math.LegacyNewDecWithPrec(1, 1), math.LegacyNewDecWithPrec(1, 1), math.LegacyNewDec(0))
stakingKeeper.EXPECT().ValidatorByConsAddr(gomock.Any(), sdk.GetConsAddress(valConsPk2)).Return(val2, nil).AnyTimes()

abciValA := abci.Validator{
abciValA := comet.Validator{
Address: valConsPk0.Address(),
Power: 11,
}
abciValB := abci.Validator{
abciValB := comet.Validator{
Address: valConsPk1.Address(),
Power: 10,
}
abciValC := abci.Validator{
abciValC := comet.Validator{
Address: valConsPk2.Address(),
Power: 10,
}
Expand Down Expand Up @@ -303,7 +304,7 @@ func TestAllocateTokensTruncation(t *testing.T) {
bankKeeper.EXPECT().GetAllBalances(gomock.Any(), feeCollectorAcc.GetAddress()).Return(fees)
bankKeeper.EXPECT().SendCoinsFromModuleToModule(gomock.Any(), "fee_collector", disttypes.ModuleName, fees)

votes := []abci.VoteInfo{
votes := []comet.VoteInfo{
{
Validator: abciValA,
},
Expand All @@ -314,6 +315,7 @@ func TestAllocateTokensTruncation(t *testing.T) {
Validator: abciValC,
},
}

require.NoError(t, distrKeeper.AllocateTokens(ctx, 31, votes))

val0OutstandingRewards, err := distrKeeper.ValidatorOutstandingRewards.Get(ctx, valAddr0)
Expand Down
7 changes: 3 additions & 4 deletions x/slashing/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
"context"
"time"

"cosmossdk.io/core/comet"

"github.com/cosmos/cosmos-sdk/telemetry"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/slashing/keeper"
Expand All @@ -21,8 +19,9 @@
// store whether or not they have actually signed it and slash/unbond any
// which have missed too many blocks in a row (downtime slashing)
sdkCtx := sdk.UnwrapSDKContext(ctx)
for _, voteInfo := range sdkCtx.VoteInfos() {
err := k.HandleValidatorSignature(ctx, voteInfo.Validator.Address, voteInfo.Validator.Power, comet.BlockIDFlag(voteInfo.BlockIdFlag))
for _, vote := range sdkCtx.CometInfo().LastCommit.Votes {

err := k.HandleValidatorSignature(ctx, vote.Validator.Address, vote.Validator.Power, vote.BlockIDFlag)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
if err != nil {
return err
}
Expand Down
30 changes: 14 additions & 16 deletions x/slashing/abci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,9 @@ import (
"testing"
"time"

abci "github.com/cometbft/cometbft/abci/types"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/stretchr/testify/require"

"cosmossdk.io/core/comet"
"cosmossdk.io/depinject"
"cosmossdk.io/log"

Expand Down Expand Up @@ -65,15 +64,17 @@ func TestBeginBlocker(t *testing.T) {
require.NoError(t, err)
require.Equal(t, amt, val.GetBondedTokens())

abciVal := abci.Validator{
abciVal := comet.Validator{
Address: pk.Address(),
Power: power,
}

ctx = ctx.WithVoteInfos([]abci.VoteInfo{{
Validator: abciVal,
BlockIdFlag: cmtproto.BlockIDFlagCommit,
}})
ctx = ctx.WithCometInfo(comet.Info{
LastCommit: comet.CommitInfo{Votes: []comet.VoteInfo{{
Validator: abciVal,
BlockIDFlag: comet.BlockIDFlagCommit,
}}},
})

err = slashing.BeginBlocker(ctx, slashingKeeper)
require.NoError(t, err)
Expand All @@ -91,11 +92,7 @@ func TestBeginBlocker(t *testing.T) {
require.NoError(t, err)
// for 100 blocks, mark the validator as having signed
for ; height < signedBlocksWindow; height++ {
ctx = ctx.WithBlockHeight(height).
WithVoteInfos([]abci.VoteInfo{{
Validator: abciVal,
BlockIdFlag: cmtproto.BlockIDFlagCommit,
}})
ctx = ctx.WithBlockHeight(height)

err = slashing.BeginBlocker(ctx, slashingKeeper)
require.NoError(t, err)
Expand All @@ -105,11 +102,12 @@ func TestBeginBlocker(t *testing.T) {
require.NoError(t, err)
// for 50 blocks, mark the validator as having not signed
for ; height < ((signedBlocksWindow * 2) - minSignedPerWindow + 1); height++ {
ctx = ctx.WithBlockHeight(height).
WithVoteInfos([]abci.VoteInfo{{
ctx = ctx.WithBlockHeight(height).WithCometInfo(comet.Info{
LastCommit: comet.CommitInfo{Votes: []comet.VoteInfo{{
Validator: abciVal,
BlockIdFlag: cmtproto.BlockIDFlagAbsent,
}})
BlockIDFlag: comet.BlockIDFlagAbsent,
}}},
})

err = slashing.BeginBlocker(ctx, slashingKeeper)
require.NoError(t, err)
Expand Down