From bcaf7378adfb1235032ecc43925cc83c077e225a Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 29 Apr 2025 18:26:37 +0000 Subject: [PATCH] chore: audit epoch (backport #24610) (#24613) Co-authored-by: Alex | Interchain Labs --- CHANGELOG.md | 1 + x/epochs/keeper/abci.go | 10 ++-- x/epochs/keeper/abci_test.go | 14 +++-- x/epochs/keeper/epoch.go | 8 ++- x/epochs/keeper/epoch_test.go | 107 ++++++++++++++++++++++++++++++++++ x/epochs/types/genesis.go | 5 +- 6 files changed, 129 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c634c72376..27b05d57f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -109,6 +109,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/auth) [#23741](https://github.com/cosmos/cosmos-sdk/pull/23741) Support legacy global AccountNumber for legacy compatibility. * (baseapp) [#24526](https://github.com/cosmos/cosmos-sdk/pull/24526) Fix incorrect retention height when `commitHeight` equals `minRetainBlocks`. * (x/protocolpool) [#24594](https://github.com/cosmos/cosmos-sdk/pull/24594) Fix NPE when initializing module via depinject. +* (x/epochs) [#24610](https://github.com/cosmos/cosmos-sdk/pull/24610) Fix semantics of `CurrentEpochStartHeight` being set before epoch has started. ## [v0.50.13](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.13) - 2025-03-12 diff --git a/x/epochs/keeper/abci.go b/x/epochs/keeper/abci.go index 2685cb950f..3e266a2608 100644 --- a/x/epochs/keeper/abci.go +++ b/x/epochs/keeper/abci.go @@ -13,25 +13,27 @@ func (k *Keeper) BeginBlocker(ctx sdk.Context) error { start := telemetry.Now() defer telemetry.ModuleMeasureSince(types.ModuleName, start, telemetry.MetricKeyBeginBlocker) - header := ctx.BlockHeader() + blockTime := ctx.BlockTime() + blockHeight := ctx.BlockHeight() + err := k.EpochInfo.Walk( ctx, nil, func(key string, epochInfo types.EpochInfo) (stop bool, err error) { // If blocktime < initial epoch start time, return - if header.Time.Before(epochInfo.StartTime) { + if blockTime.Before(epochInfo.StartTime) { return false, nil } // if epoch counting hasn't started, signal we need to start. shouldInitialEpochStart := !epochInfo.EpochCountingStarted epochEndTime := epochInfo.CurrentEpochStartTime.Add(epochInfo.Duration) - shouldEpochStart := (header.Time.After(epochEndTime)) || shouldInitialEpochStart + shouldEpochStart := (blockTime.After(epochEndTime)) || shouldInitialEpochStart if !shouldEpochStart { return false, nil } - epochInfo.CurrentEpochStartHeight = header.Height + epochInfo.CurrentEpochStartHeight = blockHeight if shouldInitialEpochStart { epochInfo.EpochCountingStarted = true diff --git a/x/epochs/keeper/abci_test.go b/x/epochs/keeper/abci_test.go index 154a145a78..c4c0512e7e 100644 --- a/x/epochs/keeper/abci_test.go +++ b/x/epochs/keeper/abci_test.go @@ -15,10 +15,12 @@ import ( // of their initial conditions, and subsequent block height / times. func (suite *KeeperTestSuite) TestEpochInfoBeginBlockChanges() { block1Time := time.Unix(1656907200, 0).UTC() - const defaultIdentifier = "hourly" - const defaultDuration = time.Hour - // eps is short for epsilon - in this case a negligible amount of time. - const eps = time.Nanosecond + const ( + defaultIdentifier = "hourly" + defaultDuration = time.Hour + // eps is short for epsilon - in this case a negligible amount of time. + eps = time.Nanosecond + ) tests := map[string]struct { // if identifier, duration is not set, we make it defaultIdentifier and defaultDuration. @@ -69,8 +71,8 @@ func (suite *KeeperTestSuite) TestEpochInfoBeginBlockChanges() { }, "StartTime in future won't get ticked on first block": { initialEpochInfo: types.EpochInfo{StartTime: block1Time.Add(time.Second), CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}}, - // currentEpochStartHeight is 1 because that's when the timer was created on-chain - expEpochInfo: types.EpochInfo{StartTime: block1Time.Add(time.Second), CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}, CurrentEpochStartHeight: 1}, + // currentEpochStartHeight is 0 since it hasn't started or been triggered + expEpochInfo: types.EpochInfo{StartTime: block1Time.Add(time.Second), CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}, CurrentEpochStartHeight: 0}, }, "StartTime in past will get ticked on first block": { initialEpochInfo: types.EpochInfo{StartTime: block1Time.Add(-time.Second), CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}}, diff --git a/x/epochs/keeper/epoch.go b/x/epochs/keeper/epoch.go index 45f5ef7202..f575351cd4 100644 --- a/x/epochs/keeper/epoch.go +++ b/x/epochs/keeper/epoch.go @@ -33,7 +33,7 @@ func (k *Keeper) AddEpochInfo(ctx sdk.Context, epoch types.EpochInfo) error { if epoch.StartTime.IsZero() { epoch.StartTime = ctx.BlockTime() } - if epoch.CurrentEpochStartHeight == 0 { + if epoch.CurrentEpochStartHeight == 0 && !epoch.StartTime.After(ctx.BlockTime()) { epoch.CurrentEpochStartHeight = ctx.BlockHeight() } return k.EpochInfo.Set(ctx, epoch.Identifier, epoch) @@ -41,7 +41,7 @@ func (k *Keeper) AddEpochInfo(ctx sdk.Context, epoch types.EpochInfo) error { // AllEpochInfos iterate through epochs to return all epochs info. func (k *Keeper) AllEpochInfos(ctx sdk.Context) ([]types.EpochInfo, error) { - epochs := []types.EpochInfo{} + var epochs []types.EpochInfo err := k.EpochInfo.Walk( ctx, nil, @@ -62,5 +62,9 @@ func (k *Keeper) NumBlocksSinceEpochStart(ctx sdk.Context, identifier string) (i if err != nil { return 0, fmt.Errorf("epoch with identifier %s not found", identifier) } + if ctx.BlockTime().Before(epoch.StartTime) { + return 0, fmt.Errorf("epoch with identifier %s has not started yet: start time: %s", identifier, epoch.StartTime) + } + return ctx.BlockHeight() - epoch.CurrentEpochStartHeight, nil } diff --git a/x/epochs/keeper/epoch_test.go b/x/epochs/keeper/epoch_test.go index defd2dbd61..f14b4a3c8c 100644 --- a/x/epochs/keeper/epoch_test.go +++ b/x/epochs/keeper/epoch_test.go @@ -49,6 +49,27 @@ func (s *KeeperTestSuite) TestAddEpochInfo() { }, expErr: true, }, + "start in future": { + addedEpochInfo: types.EpochInfo{ + Identifier: defaultIdentifier, + StartTime: startBlockTime.Add(time.Hour), + Duration: defaultDuration, + CurrentEpoch: 0, + CurrentEpochStartHeight: 0, + CurrentEpochStartTime: time.Time{}, + EpochCountingStarted: false, + }, + expEpochInfo: types.EpochInfo{ + Identifier: defaultIdentifier, + StartTime: startBlockTime.Add(time.Hour), + Duration: defaultDuration, + CurrentEpoch: 0, + CurrentEpochStartHeight: 0, + CurrentEpochStartTime: time.Time{}, + EpochCountingStarted: false, + }, + expErr: false, + }, } for name, test := range tests { s.Run(name, func() { @@ -99,3 +120,89 @@ func (s *KeeperTestSuite) TestEpochLifeCycle() { s.Require().Equal(allEpochs[3].Identifier, "monthly") s.Require().Equal(allEpochs[4].Identifier, "week") } + +func (s *KeeperTestSuite) TestNumBlocksSinceEpochStart() { + s.SetupTest() + + startBlockHeight := int64(100) + startBlockTime := time.Unix(1656907200, 0).UTC() + duration := time.Hour + + s.Ctx = s.Ctx.WithBlockHeight(startBlockHeight).WithBlockTime(startBlockTime) + + tests := map[string]struct { + setupEpoch types.EpochInfo + advanceBlockDelta int64 + advanceTimeDelta time.Duration + expErr bool + expBlocksSince int64 + }{ + "same block as start": { + setupEpoch: types.EpochInfo{ + Identifier: "epoch_same_block", + StartTime: startBlockTime, + Duration: duration, + CurrentEpoch: 0, + CurrentEpochStartHeight: startBlockHeight, + CurrentEpochStartTime: startBlockTime, + EpochCountingStarted: true, + }, + advanceBlockDelta: 0, + advanceTimeDelta: 0, + expErr: false, + expBlocksSince: 0, + }, + "after 5 blocks": { + setupEpoch: types.EpochInfo{ + Identifier: "epoch_after_five", + StartTime: startBlockTime, + Duration: duration, + CurrentEpoch: 0, + CurrentEpochStartHeight: startBlockHeight, + CurrentEpochStartTime: startBlockTime, + EpochCountingStarted: true, + }, + advanceBlockDelta: 5, + advanceTimeDelta: time.Minute * 5, // just to simulate realistic advancement + expErr: false, + expBlocksSince: 5, + }, + "epoch not started yet": { + setupEpoch: types.EpochInfo{ + Identifier: "epoch_future", + StartTime: startBlockTime.Add(time.Hour), + Duration: duration, + CurrentEpoch: 0, + CurrentEpochStartHeight: 0, + CurrentEpochStartTime: time.Time{}, + EpochCountingStarted: false, + }, + advanceBlockDelta: 0, + advanceTimeDelta: 0, + expErr: true, + expBlocksSince: 0, + }, + } + + for name, tc := range tests { + s.Run(name, func() { + s.SetupTest() + s.Ctx = s.Ctx.WithBlockHeight(startBlockHeight).WithBlockTime(startBlockTime) + + err := s.EpochsKeeper.AddEpochInfo(s.Ctx, tc.setupEpoch) + s.Require().NoError(err) + + // Advance block height and time if needed + s.Ctx = s.Ctx.WithBlockHeight(startBlockHeight + tc.advanceBlockDelta). + WithBlockTime(startBlockTime.Add(tc.advanceTimeDelta)) + + blocksSince, err := s.EpochsKeeper.NumBlocksSinceEpochStart(s.Ctx, tc.setupEpoch.Identifier) + if tc.expErr { + s.Require().Error(err) + } else { + s.Require().NoError(err) + s.Require().Equal(tc.expBlocksSince, blocksSince) + } + }) + } +} diff --git a/x/epochs/types/genesis.go b/x/epochs/types/genesis.go index 1cdea892df..99632fcc6f 100644 --- a/x/epochs/types/genesis.go +++ b/x/epochs/types/genesis.go @@ -5,14 +5,11 @@ import ( "time" ) -// DefaultIndex is the default capability global index. -const DefaultIndex uint64 = 1 - func NewGenesisState(epochs []EpochInfo) *GenesisState { return &GenesisState{Epochs: epochs} } -// DefaultGenesis returns the default Capability genesis state. +// DefaultGenesis returns the default Epochs genesis state. func DefaultGenesis() *GenesisState { epochs := []EpochInfo{ NewGenesisEpochInfo("day", time.Hour*24), // alphabetical order