Merge pull request #6303 from oasisprotocol/peternose/trivial/refactor-code

go/consensus/cometbft: Refactor code and improve consistency

Author: peternose

.changelog/6303.trivial.md

@@ -75,7 +75,7 @@ type ApplicationConfig struct {
 	ReadOnlyStorage bool
 
 	// InitialHeight is the height of the initial block.
-	InitialHeight uint64
+	InitialHeight int64
 
 	// ChainContext is the chain context for the network.
 	ChainContext string
@@ -273,7 +273,7 @@ func (mux *abciMux) registerHaltHook(hook api.HaltHook) {
 func (mux *abciMux) Info(types.RequestInfo) types.ResponseInfo {
 	return types.ResponseInfo{
 		AppVersion:       version.CometBFTAppVersion,
-		LastBlockHeight:  mux.state.BlockHeight(),
+		LastBlockHeight:  mux.state.LastHeight(),
 		LastBlockAppHash: mux.state.StateRootHash(),
 	}
 }
@@ -298,7 +298,7 @@ func (mux *abciMux) InitChain(req types.RequestInitChain) types.ResponseInitChai
 	})
 	mux.state.blockLock.Unlock()
 
-	if st.Height != req.InitialHeight || uint64(st.Height) != mux.state.initialHeight {
+	if st.Height != req.InitialHeight || st.Height != mux.state.initialHeight {
 		panic(fmt.Errorf("mux: inconsistent initial height (genesis: %d abci: %d state: %d)", st.Height, req.InitialHeight, mux.state.initialHeight))
 	}
 
@@ -570,12 +570,12 @@ func (mux *abciMux) BeginBlock(req types.RequestBeginBlock) types.ResponseBeginB
 		return *mux.state.proposal.resultsBeginBlock
 	}
 
-	blockHeight := mux.state.BlockHeight()
+	lastHeight := mux.state.LastHeight()
 
 	mux.logger.Debug("BeginBlock",
 		"req", req,
 		"hash", hex.EncodeToString(req.Hash),
-		"block_height", blockHeight,
+		"height", lastHeight,
 	)
 
 	params := mux.state.ConsensusParameters()
@@ -607,20 +607,20 @@ func (mux *abciMux) BeginBlock(req types.RequestBeginBlock) types.ResponseBeginB
 	// checks must run on each block to make sure that any pending upgrade descriptors are cleared
 	// after consensus upgrade is performed.
 	if upgrader := mux.state.Upgrader(); upgrader != nil {
-		switch err := upgrader.ConsensusUpgrade(ctx, currentEpoch, blockHeight); err {
+		switch err := upgrader.ConsensusUpgrade(ctx, currentEpoch, lastHeight); err {
 		case nil:
 			// Everything ok.
 		case upgrade.ErrStopForUpgrade:
 			// Signal graceful stop for upgrade.
-			mux.haltForUpgrade(blockHeight, currentEpoch, true)
+			mux.haltForUpgrade(lastHeight, currentEpoch, true)
 		default:
 			panic(fmt.Errorf("mux: error while trying to perform consensus upgrade: %w", err))
 		}
 	}
 
 	// Check if we need to halt based on local configuration.
-	if mux.state.shouldLocalHalt(blockHeight+1, currentEpoch) {
-		mux.haltForUpgrade(blockHeight, currentEpoch, true)
+	if mux.state.shouldLocalHalt(lastHeight+1, currentEpoch) {
+		mux.haltForUpgrade(lastHeight, currentEpoch, true)
 	}
 
 	// Dispatch BeginBlock to all applications.
@@ -632,7 +632,7 @@ func (mux *abciMux) BeginBlock(req types.RequestBeginBlock) types.ResponseBeginB
 			)
 
 			if errors.Is(err, upgrade.ErrStopForUpgrade) {
-				mux.haltForUpgrade(blockHeight, currentEpoch, true)
+				mux.haltForUpgrade(lastHeight, currentEpoch, true)
 			}
 			panic(fmt.Errorf("mux: BeginBlock: fatal error in application: '%s': %w", app.Name(), err))
 		}
@@ -642,7 +642,7 @@ func (mux *abciMux) BeginBlock(req types.RequestBeginBlock) types.ResponseBeginB
 
 	// During the first block, also collect and prepend application events generated during
 	// InitChain to BeginBlock events.
-	if mux.state.BlockHeight() == 0 {
+	if mux.state.LastHeight() == 0 {
 		response.Events = append(response.Events, mux.state.initEvents...)
 	}
 
@@ -761,7 +761,7 @@ func (mux *abciMux) EndBlock(req types.RequestEndBlock) types.ResponseEndBlock {
 
 	mux.logger.Debug("EndBlock",
 		"req", req,
-		"block_height", mux.state.BlockHeight(),
+		"height", mux.state.LastHeight(),
 	)
 
 	ctx := mux.state.NewContext(api.ContextEndBlock)
@@ -790,7 +790,7 @@ func (mux *abciMux) EndBlock(req types.RequestEndBlock) types.ResponseEndBlock {
 			panic(fmt.Errorf("mux: can't get current epoch in BeginBlock: %w", err))
 		}
 
-		err = upgrader.ConsensusUpgrade(ctx, currentEpoch, ctx.BlockHeight())
+		err = upgrader.ConsensusUpgrade(ctx, currentEpoch, ctx.LastHeight())
 		// This should never fail as all the checks were already performed in BeginBlock.
 		if err != nil {
 			panic(fmt.Errorf("mux: error while trying to perform consensus upgrade: %w", err))
@@ -829,7 +829,7 @@ func (mux *abciMux) Commit() types.ResponseCommit {
 	}
 
 	mux.logger.Debug("Commit",
-		"block_height", mux.state.BlockHeight(),
+		"height", mux.state.LastHeight(),
 		"state_root_hash", hex.EncodeToString(mux.state.StateRootHash()),
 		"last_retained_version", lastRetainedVersion,
 	)
@@ -936,7 +936,7 @@ func (mux *abciMux) checkDependencies() error {
 }
 
 func (mux *abciMux) finishInitialization() error {
-	if mux.state.BlockHeight() >= mux.state.InitialHeight() {
+	if mux.state.LastHeight() >= mux.state.InitialHeight() {
 		mux.state.resetProposal()
 		defer mux.state.closeProposal()
 
@@ -996,7 +996,7 @@ func newABCIMux(ctx context.Context, upgrader upgrade.Backend, cfg *ApplicationC
 	mux.md.Subscribe(api.MessageExecuteSubcall, mux)
 
 	mux.logger.Debug("ABCI multiplexer initialized",
-		"block_height", state.BlockHeight(),
+		"height", state.LastHeight(),
 		"state_root_hash", hex.EncodeToString(state.StateRootHash()),
 	)
 

go/consensus/cometbft/abci/mux.go

@@ -129,7 +129,7 @@ type applicationState struct {
 	ctx       context.Context
 	cancelCtx context.CancelFunc
 
-	initialHeight uint64
+	initialHeight int64
 
 	stateRoot storage.Root
 	storage   storage.LocalBackend
@@ -183,7 +183,7 @@ func (s *applicationState) NewContext(mode api.ContextMode) *api.Context {
 		blockCtx *api.BlockContext
 		state    mkvs.OverlayTree
 	)
-	blockHeight := int64(s.stateRoot.Version)
+	lastHeight := int64(s.stateRoot.Version)
 	now := s.blockTime
 	switch mode {
 	case api.ContextInitChain:
@@ -193,7 +193,7 @@ func (s *applicationState) NewContext(mode api.ContextMode) *api.Context {
 		s.initState = mkvs.NewOverlay(mkvs.New(nil, nil, storage.RootTypeState, mkvs.WithoutWriteLog()))
 		state = s.initState
 		// Configure block height so that current height will be correctly computed.
-		blockHeight = int64(s.initialHeight) - 1
+		lastHeight = s.initialHeight - 1
 	case api.ContextCheckTx:
 		state = mkvs.NewOverlayWrapper(s.checkState)
 	case api.ContextDeliverTx, api.ContextBeginBlock, api.ContextEndBlock:
@@ -215,9 +215,9 @@ func (s *applicationState) NewContext(mode api.ContextMode) *api.Context {
 		api.NewNopGasAccountant(),
 		s,
 		state,
-		blockHeight,
 		blockCtx,
-		int64(s.initialHeight),
+		lastHeight,
+		s.initialHeight,
 	)
 }
 
@@ -234,25 +234,26 @@ func (s *applicationState) Checkpointer() checkpoint.Checkpointer {
 }
 
 func (s *applicationState) InitialHeight() int64 {
-	return int64(s.initialHeight)
+	return s.initialHeight
 }
 
-func (s *applicationState) BlockHeight() int64 {
+func (s *applicationState) LastHeight() int64 {
 	s.blockLock.RLock()
 	defer s.blockLock.RUnlock()
 
-	height := s.stateRoot.Version
+	height := int64(s.stateRoot.Version)
 	if height < s.initialHeight {
 		height = 0
 	}
-	return int64(height)
+	return height
 }
 
 func (s *applicationState) StateRootHash() []byte {
 	s.blockLock.RLock()
 	defer s.blockLock.RUnlock()
 
-	if s.stateRoot.Version < s.initialHeight {
+	height := int64(s.stateRoot.Version)
+	if height < s.initialHeight {
 		// CometBFT expects a nil hash when there is no state otherwise it will panic.
 		return nil
 	}
@@ -279,12 +280,12 @@ func (s *applicationState) GetEpoch(ctx context.Context, blockHeight int64) (bea
 }
 
 func (s *applicationState) GetCurrentEpoch(ctx context.Context) (beacon.EpochTime, error) {
-	blockHeight := s.BlockHeight()
-	if blockHeight == 0 {
+	lastHeight := s.LastHeight()
+	if lastHeight == 0 {
 		return beacon.EpochInvalid, nil
 	}
 
-	latestHeight := blockHeight
+	latestHeight := lastHeight
 	if abciCtx := api.FromCtx(ctx); abciCtx != nil {
 		// If request was made from an ABCI application context, then use blockHeight + 1, to fetch
 		// the epoch at current (future) height. See cometbft/api.NewImmutableState for details.
@@ -297,23 +298,23 @@ func (s *applicationState) GetCurrentEpoch(ctx context.Context) (beacon.EpochTim
 	if err != nil {
 		return beacon.EpochInvalid, fmt.Errorf("failed to get future epoch for height %d: %w", latestHeight, err)
 	}
-	if future != nil && future.Height == blockHeight+1 {
+	if future != nil && future.Height == lastHeight+1 {
 		return future.Epoch, nil
 	}
 
 	currentEpoch, err := s.timeSource.GetEpoch(ctx, latestHeight)
 	if err != nil {
-		return beacon.EpochInvalid, fmt.Errorf("failed to get epoch for height %d: %w", blockHeight+1, err)
+		return beacon.EpochInvalid, fmt.Errorf("failed to get epoch for height %d: %w", lastHeight+1, err)
 	}
 	return currentEpoch, nil
 }
 
 func (s *applicationState) EpochChanged(ctx *api.Context) (bool, beacon.EpochTime) {
-	blockHeight := s.BlockHeight()
-	if blockHeight == 0 {
+	lastHeight := s.LastHeight()
+	if lastHeight == 0 {
 		return false, beacon.EpochInvalid
 	}
-	latestHeight := blockHeight
+	latestHeight := lastHeight
 	if abciCtx := api.FromCtx(ctx); abciCtx != nil {
 		// If request was made from an ABCI application context, then use blockHeight + 1, to fetch
 		// the epoch at current (future) height. See cometbft/api.NewImmutableState for details.
@@ -328,7 +329,7 @@ func (s *applicationState) EpochChanged(ctx *api.Context) (bool, beacon.EpochTim
 		return false, beacon.EpochInvalid
 	}
 
-	if uint64(blockHeight) == s.initialHeight {
+	if lastHeight == s.initialHeight {
 		// There is no block before the first block. For historic reasons, this is defined as not
 		// having had a transition.
 		return false, currentEpoch
@@ -380,13 +381,14 @@ func (s *applicationState) doInitChain() error {
 
 	// We use the height before the initial height for the state before the first block. Note that
 	// this tree is not persisted, we only need it to compute the root hash.
-	_, stateRootHash, err := s.canonicalState.Commit(s.ctx, s.stateRoot.Namespace, s.initialHeight-1, mkvs.NoPersist())
+	version := uint64(s.initialHeight - 1)
+	_, stateRootHash, err := s.canonicalState.Commit(s.ctx, s.stateRoot.Namespace, version, mkvs.NoPersist())
 	if err != nil {
 		return fmt.Errorf("failed to commit: %w", err)
 	}
 
 	s.stateRoot.Hash = stateRootHash
-	s.stateRoot.Version = s.initialHeight - 1
+	s.stateRoot.Version = version
 
 	return s.doCommitOrInitChainLocked()
 }
@@ -622,7 +624,7 @@ func (s *applicationState) pruneWorker() {
 			if err := s.statePruner.Prune(version); err != nil {
 				s.logger.Warn("failed to prune state",
 					"err", err,
-					"block_height", version,
+					"height", version,
 				)
 			}
 		}
@@ -756,7 +758,8 @@ func newApplicationState(ctx context.Context, upgrader upgrade.Backend, cfg *App
 	}
 
 	// Refresh consensus parameters when loading state if we are past genesis.
-	if latestVersion >= s.initialHeight {
+	initialVersion := uint64(s.initialHeight)
+	if latestVersion >= initialVersion {
 		if err = s.doCommitOrInitChainLocked(); err != nil {
 			return nil, fmt.Errorf("state: failed to run initial state commit hook: %w", err)
 		}
@@ -774,7 +777,7 @@ func newApplicationState(ctx context.Context, upgrader upgrade.Backend, cfg *App
 					Interval:       params.StateCheckpointInterval,
 					NumKept:        params.StateCheckpointNumKept,
 					ChunkSize:      params.StateCheckpointChunkSize,
-					InitialVersion: cfg.InitialHeight,
+					InitialVersion: initialVersion,
 					ChunkerThreads: cfg.ChunkerThreads,
 				}, nil
 			},

go/consensus/cometbft/abci/state.go

@@ -137,7 +137,7 @@ func (mux *abciMux) executeTx(ctx *api.Context, rawTx []byte) error {
 	// If we are in CheckTx mode and there is a pending upgrade in this block, make sure to reject
 	// any transactions before processing as they may potentially query incompatible state.
 	if upgrader := mux.state.Upgrader(); upgrader != nil && ctx.IsCheckOnly() {
-		hasUpgrade, err := upgrader.HasPendingUpgradeAt(ctx.BlockHeight() + 1)
+		hasUpgrade, err := upgrader.HasPendingUpgradeAt(ctx.CurrentHeight())
 		if err != nil {
 			return fmt.Errorf("failed to check for pending upgrades: %w", err)
 		}
@@ -156,7 +156,7 @@ func (mux *abciMux) EstimateGas(caller signature.PublicKey, tx *transaction.Tran
 
 	// Certain modules, in particular the beacon require InitChain or BeginBlock
 	// to have completed before initialization is complete.
-	if mux.state.BlockHeight() == 0 {
+	if mux.state.LastHeight() == 0 {
 		return 0, consensus.ErrNoCommittedBlocks
 	}
 

go/consensus/cometbft/abci/transaction.go

@@ -24,7 +24,7 @@ func (mux *abciMux) maybeHaltForUpgrade() {
 		)
 		return
 	}
-	height := mux.state.BlockHeight()
+	height := mux.state.LastHeight()
 
 	// Check if we need to halt for an upgrade -- but do not actually run the upgrade.
 	switch err := upgrader.ConsensusUpgrade(nil, epoch, height); err {

go/consensus/cometbft/abci/upgrade.go

@@ -79,8 +79,8 @@ type Context struct {
 
 	appState      ApplicationState
 	state         mkvs.KeyValueTree
-	blockHeight   int64
 	blockCtx      *BlockContext
+	lastHeight    int64
 	initialHeight int64
 
 	logger *logging.Logger
@@ -101,8 +101,8 @@ func NewContext(
 	gasAccountant GasAccountant,
 	appState ApplicationState,
 	state mkvs.KeyValueTree,
-	blockHeight int64,
 	blockCtx *BlockContext,
+	lastHeight int64,
 	initialHeight int64,
 ) *Context {
 	c := &Context{
@@ -112,8 +112,8 @@ func NewContext(
 		priority:      0,
 		appState:      appState,
 		state:         state,
-		blockHeight:   blockHeight,
 		blockCtx:      blockCtx,
+		lastHeight:    lastHeight,
 		initialHeight: initialHeight,
 		logger:        logging.GetLogger("consensus/cometbft/abci").With("mode", mode),
 	}
@@ -229,7 +229,7 @@ func (c *Context) NewChild() *Context {
 		callerAddress:      c.callerAddress,
 		appState:           c.appState,
 		state:              c.state,
-		blockHeight:        c.blockHeight,
+		lastHeight:         c.lastHeight,
 		blockCtx:           c.blockCtx,
 		initialHeight:      c.initialHeight,
 		logger:             c.logger,
@@ -421,9 +421,14 @@ func (c *Context) InitialHeight() int64 {
 	return c.initialHeight
 }
 
-// BlockHeight returns the current block height.
-func (c *Context) BlockHeight() int64 {
-	return c.blockHeight
+// LastHeight returns the last committed block height.
+func (c *Context) LastHeight() int64 {
+	return c.lastHeight
+}
+
+// CurrentHeight returns the height of the block being built.
+func (c *Context) CurrentHeight() int64 {
+	return c.lastHeight + 1
 }
 
 // LastStateRootHash returns the last state root hash.

go/consensus/cometbft/api/context.go

@@ -63,7 +63,7 @@ func TestChildContext(t *testing.T) {
 	require.EqualValues(ctx.State(), child.State(), "child.State should correspond to parent.State")
 	require.EqualValues(ctx.AppState(), child.AppState(), "child.Mode should correspond to parent.Mode")
 	require.EqualValues(ctx.InitialHeight(), child.InitialHeight(), "child.InitialHeight should correspond to parent.InitialHeight")
-	require.EqualValues(ctx.BlockHeight(), child.BlockHeight(), "child.BlockHeight should correspond to parent.BlockHeight")
+	require.EqualValues(ctx.LastHeight(), child.LastHeight(), "child.LastHeight should correspond to parent.LastHeight")
 	require.EqualValues(ctx.BlockContext(), child.BlockContext(), "child.BlockContext should correspond to parent.BlockContext")
 	require.EqualValues(1, child.CallDepth(), "child.CallDepth should be correct")