remove isProposer as it use to be always true

tac0turtle · Apr 28, 2025 · Apr 24, 2025 · Apr 24, 2025 · Apr 24, 2025 · Apr 24, 2025
commit c9e1a5d5338dcd1aaa1210f3c0e8ca109b6457f3
diff --git a/block/aggregation_test.go b/block/aggregation_test.go
@@ -20,89 +20,6 @@ import (
 	"github.com/rollkit/rollkit/types"
 )
 
-// TestAggregationLoop_Normal_ContextCancellation verifies the loop terminates promptly on context cancellation.
-func TestAggregationLoop_Normal_ContextCancellation(t *testing.T) {
-	assert := assert.New(t)
-
-	blockTime := 1 * time.Second
-	shortTimeout := 100 * time.Millisecond
-
-	mockStore := mocks.NewStore(t)
-	mockStore.On("Height", mock.Anything).Return(uint64(1), nil).Maybe()
-	mockStore.On("GetState", mock.Anything).Return(types.State{LastBlockTime: time.Now().Add(-blockTime)}, nil).Maybe()
-
-	mockExec := mocks.NewExecutor(t)
-	mockSeq := mocks.NewSequencer(t)
-	mockDAC := mocks.NewClient(t)
-	logger := log.NewTestLogger(t)
-
-	m := &Manager{
-		store:      mockStore,
-		exec:       mockExec,
-		sequencer:  mockSeq,
-		dalc:       mockDAC,
-		logger:     logger,
-		isProposer: true,
-		config: config.Config{
-			Node: config.NodeConfig{
-				BlockTime:      config.DurationWrapper{Duration: blockTime},
-				LazyAggregator: false,
-			},
-			DA: config.DAConfig{
-				BlockTime: config.DurationWrapper{Duration: 1 * time.Second},
-			},
-		},
-		genesis: genesispkg.Genesis{
-			InitialHeight: 1,
-		},
-		lastState: types.State{
-			LastBlockTime: time.Now().Add(-blockTime),
-		},
-		lastStateMtx: &sync.RWMutex{},
-		metrics:      NopMetrics(),
-		headerCache:  cache.NewCache[types.SignedHeader](),
-		dataCache:    cache.NewCache[types.Data](),
-	}
-
-	var publishCalls atomic.Int64
-	mockPublishBlock := func(ctx context.Context) error {
-		publishCalls.Add(1)
-		m.logger.Debug("Mock publishBlock called")
-		time.Sleep(10 * time.Millisecond)
-		return nil
-	}
-	m.publishBlock = mockPublishBlock
-
-	ctx, cancel := context.WithCancel(context.Background())
-	var wg sync.WaitGroup
-	wg.Add(1)
-	go func() {
-		defer wg.Done()
-		m.AggregationLoop(ctx)
-		m.logger.Info("AggregationLoop exited")
-	}()
-
-	m.logger.Info("Cancelling context immediately")
-	cancel()
-
-	waitChan := make(chan struct{})
-	go func() {
-		wg.Wait()
-		close(waitChan)
-	}()
-
-	select {
-	case <-waitChan:
-		m.logger.Info("WaitGroup finished within timeout")
-	case <-time.After(shortTimeout):
-		assert.Fail("AggregationLoop did not exit within the expected short timeout after cancellation")
-	}
-
-	calls := publishCalls.Load()
-	assert.LessOrEqualf(calls, int64(1), "publishBlock should have been called at most once, but was called %d times", calls)
-	m.logger.Info("Publish block calls", "count", calls)
-}
-
 // TestAggregationLoop_Normal_BasicInterval verifies the basic time interval logic of the normal aggregation loop.
 func TestAggregationLoop_Normal_BasicInterval(t *testing.T) {
 	assert := assert.New(t)
@@ -121,12 +38,11 @@ func TestAggregationLoop_Normal_BasicInterval(t *testing.T) {
 	logger := log.NewTestLogger(t)
 
 	m := &Manager{
-		store:      mockStore,
-		exec:       mockExec,
-		sequencer:  mockSeq,
-		dalc:       mockDAC,
-		logger:     logger,
-		isProposer: true,
+		store:     mockStore,
+		exec:      mockExec,
+		sequencer: mockSeq,
+		dalc:      mockDAC,
+		logger:    logger,
 		config: config.Config{
 			Node: config.NodeConfig{
 				BlockTime:      config.DurationWrapper{Duration: blockTime},
@@ -219,12 +135,11 @@ func TestAggregationLoop_Normal_PublishBlockError(t *testing.T) {
 
 	// Create a basic Manager instance
 	m := &Manager{
-		store:      mockStore,
-		exec:       mockExec,
-		sequencer:  mockSeq,
-		dalc:       mockDAC,
-		logger:     mockLogger,
-		isProposer: true,
+		store:     mockStore,
+		exec:      mockExec,
+		sequencer: mockSeq,
+		dalc:      mockDAC,
+		logger:    mockLogger,
 		config: config.Config{
 			Node: config.NodeConfig{
 				BlockTime:      config.DurationWrapper{Duration: blockTime},
@@ -293,64 +208,3 @@ func TestAggregationLoop_Normal_PublishBlockError(t *testing.T) {
 		WithinDuration(t, blockTime, interval, tolerance)
 	}
 }
-
-func TestAggregationLoop(t *testing.T) {
-	mockStore := new(mocks.Store)
-	mockLogger := log.NewTestLogger(t)
-
-	m := &Manager{
-		store:  mockStore,
-		logger: mockLogger,
-		genesis: genesispkg.NewGenesis(
-			"myChain",
-			1,
-			time.Now(),
-			[]byte{},
-		),
-		config: config.Config{
-			Node: config.NodeConfig{
-				BlockTime:      config.DurationWrapper{Duration: time.Second},
-				LazyAggregator: false,
-			},
-		},
-	}
-
-	m.publishBlock = m.publishBlockInternal
-
-	mockStore.On("Height", mock.Anything).Return(uint64(0), nil)
-
-	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
-	defer cancel()
-
-	go m.AggregationLoop(ctx)
-
-	<-ctx.Done()
-
-	mockStore.AssertExpectations(t)
-}
-
-func TestNormalAggregationLoop(t *testing.T) {
-	mockLogger := log.NewTestLogger(t)
-
-	m := &Manager{
-		logger: mockLogger,
-		config: config.Config{
-			Node: config.NodeConfig{
-				BlockTime:      config.DurationWrapper{Duration: 1 * time.Second},
-				LazyAggregator: false,
-			},
-		},
-	}
-
-	m.publishBlock = m.publishBlockInternal
-
-	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
-	defer cancel()
-
-	blockTimer := time.NewTimer(m.config.Node.BlockTime.Duration)
-	defer blockTimer.Stop()
-
-	go m.normalAggregationLoop(ctx, blockTimer)
-
-	<-ctx.Done()
-}
diff --git a/block/manager.go b/block/manager.go
@@ -141,9 +141,6 @@ type Manager struct {
 	// for reporting metrics
 	metrics *Metrics
 
-	// true if the manager is a proposer
-	isProposer bool
-
 	exec coreexecutor.Executor
 
 	// daIncludedHeight is rollup height at which all blocks have been included
@@ -162,12 +159,10 @@ type Manager struct {
 }
 
 // getInitialState tries to load lastState from Store, and if it's not available it reads genesis.
-func getInitialState(ctx context.Context, genesis genesis.Genesis, signer signer.Signer, store store.Store, exec coreexecutor.Executor, logger log.Logger) (types.State, crypto.PubKey, error) {
+func getInitialState(ctx context.Context, genesis genesis.Genesis, signer signer.Signer, store store.Store, exec coreexecutor.Executor, logger log.Logger) (types.State, error) {
 	// Load the state from store.
 	s, err := store.GetState(ctx)
 
-	var pubKey crypto.PubKey
-
 	if errors.Is(err, ds.ErrNotFound) {
 		logger.Info("No state found in store, initializing new state")
 
@@ -183,22 +178,23 @@ func getInitialState(ctx context.Context, genesis genesis.Genesis, signer signer
 
 		var signature types.Signature
 
+		var pubKey crypto.PubKey
 		// The signer is only provided in aggregator nodes. This enables the creation of a signed genesis header,
 		// which includes a public key and a cryptographic signature for the header.
 		// In a full node (non-aggregator), the signer will be nil, and only an unsigned genesis header will be initialized locally.
 		if signer != nil {
 			pubKey, err = signer.GetPublic()
 			if err != nil {
-				return types.State{}, pubKey, fmt.Errorf("failed to get public key: %w", err)
+				return types.State{}, fmt.Errorf("failed to get public key: %w", err)
 			}
 
 			b, err := header.MarshalBinary()
 			if err != nil {
-				return types.State{}, pubKey, err
+				return types.State{}, err
 			}
 			signature, err = signer.Sign(b)
 			if err != nil {
-				return types.State{}, pubKey, fmt.Errorf("failed to get header signature: %w", err)
+				return types.State{}, fmt.Errorf("failed to get header signature: %w", err)
 			}
 		}
 
@@ -215,15 +211,15 @@ func getInitialState(ctx context.Context, genesis genesis.Genesis, signer signer
 			&signature,
 		)
 		if err != nil {
-			return types.State{}, pubKey, fmt.Errorf("failed to save genesis block: %w", err)
+			return types.State{}, fmt.Errorf("failed to save genesis block: %w", err)
 		}
 
 		// If the user is starting a fresh chain (or hard-forking), we assume the stored state is empty.
 		// TODO(tzdybal): handle max bytes
 		stateRoot, _, err := exec.InitChain(ctx, genesis.GenesisDAStartHeight, genesis.InitialHeight, genesis.ChainID)
 		if err != nil {
 			logger.Error("error while initializing chain", "error", err)
-			return types.State{}, pubKey, err
+			return types.State{}, err
 		}
 
 		s := types.State{
@@ -235,20 +231,20 @@ func getInitialState(ctx context.Context, genesis genesis.Genesis, signer signer
 			AppHash:         stateRoot,
 			DAHeight:        0,
 		}
-		return s, pubKey, nil
+		return s, nil
 	} else if err != nil {
 		logger.Error("error while getting state", "error", err)
-		return types.State{}, pubKey, err
+		return types.State{}, err
 	} else {
 		// Perform a sanity-check to stop the user from
 		// using a higher genesis than the last stored state.
 		// if they meant to hard-fork, they should have cleared the stored State
 		if uint64(genesis.InitialHeight) > s.LastBlockHeight { //nolint:unconvert
-			return types.State{}, pubKey, fmt.Errorf("genesis.InitialHeight (%d) is greater than last stored state's LastBlockHeight (%d)", genesis.InitialHeight, s.LastBlockHeight)
+			return types.State{}, fmt.Errorf("genesis.InitialHeight (%d) is greater than last stored state's LastBlockHeight (%d)", genesis.InitialHeight, s.LastBlockHeight)
 		}
 	}
 
-	return s, pubKey, nil
+	return s, nil
 }
 
 // NewManager creates new block Manager.
@@ -268,7 +264,7 @@ func NewManager(
 	gasPrice float64,
 	gasMultiplier float64,
 ) (*Manager, error) {
-	s, pubkey, err := getInitialState(ctx, genesis, signer, store, exec, logger)
+	s, err := getInitialState(ctx, genesis, signer, store, exec, logger)
 	if err != nil {
 		logger.Error("error while getting initial state", "error", err)
 		return nil, err
@@ -307,15 +303,11 @@ func NewManager(
 	if err != nil {
 		return nil, err
 	}
-
+	// allow buffer for the block header and protocol encoding
+	// TODO: why is this needed?
+	//nolint:ineffassign // This assignment is needed
 	maxBlobSize -= blockProtocolOverhead
 
-	isProposer, err := isProposer(signer, pubkey)
-	if err != nil {
-		logger.Error("error while checking if proposer", "error", err)
-		return nil, err
-	}
-
 	pendingHeaders, err := NewPendingHeaders(store, logger)
 	if err != nil {
 		return nil, err
@@ -358,7 +350,6 @@ func NewManager(
 		buildingBlock:  false,
 		pendingHeaders: pendingHeaders,
 		metrics:        seqMetrics,
-		isProposer:     isProposer,
 		sequencer:      sequencer,
 		exec:           exec,
 		gasPrice:       gasPrice,
@@ -380,11 +371,6 @@ func (m *Manager) PendingHeaders() *PendingHeaders {
 	return m.pendingHeaders
 }
 
-// IsProposer returns true if the manager is acting as proposer.
-func (m *Manager) IsProposer() bool {
-	return m.isProposer
-}
-
 // SeqClient returns the grpc sequencing client.
 func (m *Manager) SeqClient() coresequencer.Sequencer {
 	return m.sequencer
@@ -524,10 +510,6 @@ func (m *Manager) publishBlockInternal(ctx context.Context) error {
 	default:
 	}
 
-	if !m.isProposer {
-		return ErrNotProposer
-	}
-
 	if m.config.Node.MaxPendingBlocks != 0 && m.pendingHeaders.numPendingHeaders() >= m.config.Node.MaxPendingBlocks {
 		return fmt.Errorf("refusing to create block: pending blocks [%d] reached limit [%d]",
 			m.pendingHeaders.numPendingHeaders(), m.config.Node.MaxPendingBlocks)
@@ -583,7 +565,6 @@ func (m *Manager) publishBlockInternal(ctx context.Context) error {
 			// Continue but log the state
 			m.logger.Info("Current state",
 				"height", height,
-				"isProposer", m.isProposer,
 				"pendingHeaders", m.pendingHeaders.numPendingHeaders())
 		}
 

diff --git a/block/manager_test.go b/block/manager_test.go
@@ -70,7 +70,7 @@ func TestInitialStateClean(t *testing.T) {
 	mockExecutor.On("InitChain", ctx, genesisData.GenesisDAStartHeight, genesisData.InitialHeight, genesisData.ChainID).
 		Return([]byte("mockAppHash"), uint64(1000), nil).Once()
 
-	s, _, err := getInitialState(ctx, genesisData, nil, emptyStore, mockExecutor, logger)
+	s, err := getInitialState(ctx, genesisData, nil, emptyStore, mockExecutor, logger)
 	require.NoError(err)
 	initialHeight := genesisData.InitialHeight
 	require.Equal(initialHeight-1, s.LastBlockHeight)
@@ -100,7 +100,7 @@ func TestInitialStateStored(t *testing.T) {
 	mockExecutor := mocks.NewExecutor(t)
 
 	// getInitialState should not call InitChain if state exists
-	s, _, err := getInitialState(ctx, genesisData, nil, store, mockExecutor, logger)
+	s, err := getInitialState(ctx, genesisData, nil, store, mockExecutor, logger)
 	require.NoError(err)
 	require.Equal(s.LastBlockHeight, uint64(100))
 	require.Equal(s.InitialHeight, uint64(1))
@@ -134,7 +134,7 @@ func TestInitialStateUnexpectedHigherGenesis(t *testing.T) {
 	require.NoError(err)
 	mockExecutor := mocks.NewExecutor(t)
 
-	_, _, err = getInitialState(ctx, genesis, nil, store, mockExecutor, logger)
+	_, err = getInitialState(ctx, genesis, nil, store, mockExecutor, logger)
 	require.EqualError(err, "genesis.InitialHeight (2) is greater than last stored state's LastBlockHeight (0)")
 
 	// Assert mock expectations (InitChain should not have been called)