fix(manager): move update height (#1782)

## Overview

Update height only after block responses are saved.

Partially fixes #477 

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit


- **New Features**
- Enhanced testing capabilities for the block publishing process,
including validation for successful executions and error handling.

- **Bug Fixes**
- Improved state management logic during block publication to enhance
consistency and error handling.

- **Tests**
- Added new test cases to ensure the correct behavior of the block
publishing method under various scenarios.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Author: tuxcanfly

block/manager.go

@@ -917,8 +917,6 @@ func (m *Manager) publishBlock(ctx context.Context) error {
 	}
 
 	blockHeight := block.Height()
-	// Update the store height before submitting to the DA layer and committing to the DB
-	m.store.SetHeight(ctx, blockHeight)
 
 	blockHash := block.Hash().String()
 	m.blockCache.setSeen(blockHash)
@@ -943,6 +941,9 @@ func (m *Manager) publishBlock(ctx context.Context) error {
 		return err
 	}
 
+	// Update the store height before submitting to the DA layer but after committing to the DB
+	m.store.SetHeight(ctx, blockHeight)
+
 	newState.DAHeight = atomic.LoadUint64(&m.daHeight)
 	// After this call m.lastState is the NEW state returned from ApplyBlock
 	// updateState also commits the DB tx

block/manager_test.go

@@ -3,20 +3,28 @@ package block
 import (
 	"bytes"
 	"context"
+	"crypto/rand"
 	"errors"
 	"fmt"
 	"os"
 	"strconv"
+	"sync"
 	"testing"
 	"time"
 
+	abci "github.com/cometbft/cometbft/abci/types"
+	cfg "github.com/cometbft/cometbft/config"
 	cmcrypto "github.com/cometbft/cometbft/crypto"
 	"github.com/cometbft/cometbft/crypto/ed25519"
 	"github.com/cometbft/cometbft/crypto/secp256k1"
+	"github.com/cometbft/cometbft/libs/log"
+	cmproto "github.com/cometbft/cometbft/proto/tendermint/types"
+	"github.com/cometbft/cometbft/proxy"
 	cmtypes "github.com/cometbft/cometbft/types"
 	ds "github.com/ipfs/go-datastore"
 	"github.com/libp2p/go-libp2p/core/crypto"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 
 	goDA "github.com/rollkit/go-da"
@@ -25,6 +33,8 @@ import (
 	"github.com/rollkit/rollkit/config"
 	"github.com/rollkit/rollkit/da"
 	mockda "github.com/rollkit/rollkit/da/mock"
+	"github.com/rollkit/rollkit/mempool"
+	"github.com/rollkit/rollkit/state"
 	"github.com/rollkit/rollkit/store"
 	test "github.com/rollkit/rollkit/test/log"
 	"github.com/rollkit/rollkit/test/mocks"
@@ -549,6 +559,121 @@ func Test_publishBlock_ManagerNotProposer(t *testing.T) {
 	require.ErrorIs(err, ErrNotProposer)
 }
 
+func TestManager_publishBlock(t *testing.T) {
+	mockStore := new(mocks.Store)
+	mockLogger := new(test.MockLogger)
+	assert := assert.New(t)
+	require := require.New(t)
+
+	logger := log.TestingLogger()
+
+	var mockAppHash []byte
+	_, err := rand.Read(mockAppHash[:])
+	require.NoError(err)
+
+	app := &mocks.Application{}
+	app.On("CheckTx", mock.Anything, mock.Anything).Return(&abci.ResponseCheckTx{}, nil)
+	app.On("Commit", mock.Anything, mock.Anything).Return(&abci.ResponseCommit{}, nil)
+	app.On("PrepareProposal", mock.Anything, mock.Anything).Return(func(_ context.Context, req *abci.RequestPrepareProposal) (*abci.ResponsePrepareProposal, error) {
+		return &abci.ResponsePrepareProposal{
+			Txs: req.Txs,
+		}, nil
+	})
+	app.On("ProcessProposal", mock.Anything, mock.Anything).Return(&abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT}, nil)
+	app.On("FinalizeBlock", mock.Anything, mock.Anything).Return(
+		func(_ context.Context, req *abci.RequestFinalizeBlock) (*abci.ResponseFinalizeBlock, error) {
+			txResults := make([]*abci.ExecTxResult, len(req.Txs))
+			for idx := range req.Txs {
+				txResults[idx] = &abci.ExecTxResult{
+					Code: abci.CodeTypeOK,
+				}
+			}
+
+			return &abci.ResponseFinalizeBlock{
+				TxResults: txResults,
+				AppHash:   mockAppHash,
+			}, nil
+		},
+	)
+
+	client, err := proxy.NewLocalClientCreator(app).NewABCIClient()
+	require.NoError(err)
+	require.NotNil(client)
+
+	vKey := ed25519.GenPrivKey()
+	validators := []*cmtypes.Validator{
+		{
+			Address:          vKey.PubKey().Address(),
+			PubKey:           vKey.PubKey(),
+			VotingPower:      int64(100),
+			ProposerPriority: int64(1),
+		},
+	}
+
+	lastState := types.State{}
+	lastState.ConsensusParams.Block = &cmproto.BlockParams{}
+	lastState.ConsensusParams.Block.MaxBytes = 100
+	lastState.ConsensusParams.Block.MaxGas = 100000
+	lastState.ConsensusParams.Abci = &cmproto.ABCIParams{VoteExtensionsEnableHeight: 0}
+	lastState.Validators = cmtypes.NewValidatorSet(validators)
+	lastState.NextValidators = cmtypes.NewValidatorSet(validators)
+	lastState.LastValidators = cmtypes.NewValidatorSet(validators)
+
+	mpool := mempool.NewCListMempool(cfg.DefaultMempoolConfig(), proxy.NewAppConnMempool(client, proxy.NopMetrics()), 0)
+	executor := state.NewBlockExecutor(vKey.PubKey().Address(), "test", mpool, proxy.NewAppConnConsensus(client, proxy.NopMetrics()), nil, 100, logger, state.NopMetrics())
+
+	signingKey, err := types.PrivKeyToSigningKey(vKey)
+	require.NoError(err)
+	m := &Manager{
+		lastState:    lastState,
+		lastStateMtx: new(sync.RWMutex),
+		blockCache:   NewBlockCache(),
+		executor:     executor,
+		store:        mockStore,
+		logger:       mockLogger,
+		genesis: &cmtypes.GenesisDoc{
+			ChainID:       "myChain",
+			InitialHeight: 1,
+			AppHash:       []byte("app hash"),
+		},
+		conf: config.BlockManagerConfig{
+			BlockTime:      time.Second,
+			LazyAggregator: false,
+		},
+		isProposer:  true,
+		proposerKey: signingKey,
+		metrics:     NopMetrics(),
+	}
+
+	t.Run("height should not be updated if saving block responses fails", func(t *testing.T) {
+		mockStore.On("Height").Return(uint64(0))
+		signature := types.Signature([]byte{1, 1, 1})
+		block, err := executor.CreateBlock(0, &signature, abci.ExtendedCommitInfo{}, []byte{}, lastState)
+		require.NoError(err)
+		require.NotNil(block)
+		assert.Equal(uint64(0), block.Height())
+		dataHash, err := block.Data.Hash()
+		assert.NoError(err)
+		block.SignedHeader.DataHash = dataHash
+
+		// Update the signature on the block to current from last
+		voteBytes := block.SignedHeader.Header.MakeCometBFTVote()
+		signature, _ = vKey.Sign(voteBytes)
+		block.SignedHeader.Signature = signature
+		block.SignedHeader.Validators = lastState.Validators
+
+		mockStore.On("GetBlock", mock.Anything, uint64(1)).Return(block, nil).Once()
+		mockStore.On("SaveBlock", mock.Anything, block, mock.Anything).Return(nil).Once()
+		mockStore.On("SaveBlockResponses", mock.Anything, uint64(0), mock.Anything).Return(errors.New("failed to save block responses")).Once()
+
+		ctx := context.Background()
+		err = m.publishBlock(ctx)
+		assert.ErrorContains(err, "failed to save block responses")
+
+		mockStore.AssertExpectations(t)
+	})
+}
+
 func TestManager_getRemainingSleep(t *testing.T) {
 	tests := []struct {
 		name          string