Use blocklist and multi-gas-pools only while sequencing

core/celo_multi_gaspool.go

@@ -23,37 +23,44 @@ func NewMultiGasPool(
 	allowlist common.AddressSet,
 	defaultLimit float64,
 	limitsMapping FeeCurrencyLimitMapping,
+	isDeriving bool,
 ) *MultiGasPool {
-	pools := make(map[FeeCurrency]*GasPool, len(allowlist))
+	multiGasPool := &MultiGasPool{
+		defaultPool: new(GasPool).AddGas(blockGasLimit),
+	}
+
+	// we want to deactivate the separate pools for
+	// fee-currencies when we are deriving from l1.
+	// The parameters are locally configurable,
+	// and we don't want them to differ
+	// from the "consensus", i.e. the ones
+	// that the sequencer used for building.
+	if isDeriving {
+		return multiGasPool
+	}
+	multiGasPool.pools = make(map[FeeCurrency]*GasPool, len(allowlist))
 
 	for currency := range allowlist {
 		fraction, ok := limitsMapping[currency]
 		if !ok {
 			fraction = defaultLimit
 		}
-
-		pools[currency] = new(GasPool).AddGas(
+		multiGasPool.pools[currency] = new(GasPool).AddGas(
 			uint64(float64(blockGasLimit) * fraction),
 		)
 	}
 
-	// A special case for CELO which doesn't have a limit
-	celoPool := new(GasPool).AddGas(blockGasLimit)
-
-	return &MultiGasPool{
-		pools:       pools,
-		defaultPool: celoPool,
-	}
+	return multiGasPool
 }
 
 // PoolFor returns a configured pool for the given fee currency or the default
-// one otherwise
-func (mgp MultiGasPool) PoolFor(feeCurrency *FeeCurrency) *GasPool {
+// one otherwise, the returned boolean is true when a custom fee currency gas pool is returned.
+func (mgp MultiGasPool) PoolFor(feeCurrency *FeeCurrency) (*GasPool, bool) {
 	if feeCurrency == nil || mgp.pools[*feeCurrency] == nil {
-		return mgp.defaultPool
+		return mgp.defaultPool, false
 	}
 
-	return mgp.pools[*feeCurrency]
+	return mgp.pools[*feeCurrency], true
 }
 
 func (mgp MultiGasPool) Copy() *MultiGasPool {

core/celo_multi_gaspool_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/ethereum/go-ethereum/common"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestMultiCurrencyGasPool(t *testing.T) {
@@ -102,22 +103,27 @@ func TestMultiCurrencyGasPool(t *testing.T) {
 				c.allowlist,
 				c.defaultLimit,
 				c.limits,
+				false,
 			)
 
-			pool := mgp.PoolFor(c.feeCurrency)
+			pool, hasMultiPool := mgp.PoolFor(c.feeCurrency)
 			pool.SubGas(uint64(subGasAmount))
+			result := pool.Gas()
+
+			defaultPool, defaultHasMultipool := mgp.PoolFor(nil)
+			assert.False(t, defaultHasMultipool)
 
 			if c.defaultPoolExpected {
-				result := mgp.PoolFor(nil).Gas()
+				assert.False(t, hasMultiPool)
 				if result != c.expectedValue {
 					t.Error("Default pool expected", c.expectedValue, "got", result)
 				}
 			} else {
-				result := mgp.PoolFor(c.feeCurrency).Gas()
-
+				assert.True(t, hasMultiPool)
+				assert.Equal(t, defaultPool.Gas(), blockGasLimit)
 				if result != c.expectedValue {
 					t.Error(
-						"Expected pool", c.feeCurrency, "value", c.expectedValue,
+						"Expected multi-gas-pool for currency", c.feeCurrency, "value", c.expectedValue,
 						"got", result,
 					)
 				}

miner/celo_miner_test.go

@@ -53,6 +53,7 @@ func createCeloMiner(t *testing.T) *Miner {
 			GasLimit:   11500000,
 			Difficulty: big.NewInt(1048576),
 			Alloc:      core.CeloGenesisAccounts(address),
+			Timestamp:  uint64(time.Now().Unix()),
 		}
 		minerConfig = Config{
 			PendingFeeRecipient:                   common.HexToAddress("123456789"),
@@ -101,7 +102,6 @@ func createCeloMiner(t *testing.T) *Miner {
 // for miners correctly considers the currency of the transaction fee
 func TestMinerFeeCalculationWithCurrencyConversion(t *testing.T) {
 	miner := createCeloMiner(t)
-	timestamp := uint64(time.Now().Unix())
 	rates := common.ExchangeRates{
 		core.DevFeeCurrencyAddr: big.NewRat(2, 1),
 	}
@@ -127,11 +127,12 @@ func TestMinerFeeCalculationWithCurrencyConversion(t *testing.T) {
 	})
 	txs := types.Transactions{tx1, tx2}
 
+	parentBlock := miner.chain.CurrentBlock()
 	// Add transactions & request block
 	miner.txpool.Add(txs, true, false)
 	r := miner.generateWork(&generateParams{
-		parentHash: miner.chain.CurrentBlock().Hash(),
-		timestamp:  timestamp,
+		parentHash: parentBlock.Hash(),
+		timestamp:  parentBlock.Time + 1,
 		random:     common.HexToHash("0xcafebabe"),
 		noTxs:      false,
 		forceTime:  true,
@@ -162,3 +163,85 @@ func TestMinerFeeCalculationWithCurrencyConversion(t *testing.T) {
 	)
 	assert.Equal(t, expectedMinerFee, r.fees)
 }
+
+func TestBlocklistOnlyForSequencing(t *testing.T) {
+	miner := createCeloMiner(t)
+
+	signer := types.LatestSigner(miner.chainConfig)
+	parentBlock := miner.chain.CurrentBlock()
+	miner.feeCurrencyBlocklist.Add(core.DevFeeCurrencyAddr, *parentBlock)
+
+	var nonce uint64 = 0
+	makeTx := func(addToPool bool) *types.Transaction {
+		tx := types.MustSignNewTx(key, signer, &types.CeloDynamicFeeTxV2{
+			Nonce:       nonce,
+			To:          &testUserAddress,
+			Value:       big.NewInt(1),
+			Gas:         71000,
+			FeeCurrency: &core.DevFeeCurrencyAddr,
+			GasFeeCap:   big.NewInt(100 * params.GWei),
+			GasTipCap:   big.NewInt(10 * params.GWei),
+		})
+		assert.NotNil(t, parentBlock)
+		if addToPool {
+			miner.txpool.Add(types.Transactions{tx}, true, false)
+		}
+		return tx
+	}
+
+	tx := makeTx(true)
+	require.True(t, miner.feeCurrencyBlocklist.IsBlocked(*tx.FeeCurrency(), parentBlock))
+
+	// pending block building, blocklist disabled
+	r := miner.generateWork(&generateParams{
+		parentHash: parentBlock.Hash(),
+		timestamp:  parentBlock.Time + 1,
+		random:     common.HexToHash("0xcafebabe"),
+		noTxs:      false,
+		forceTime:  true,
+		isPending:  true,
+	}, false)
+
+	// Make sure the transactions are finalized
+	require.Equal(t, 1, len(r.block.Transactions()), "block should have 1 transactions")
+	require.False(t, tx.Rejected(), "tx should not be rejected")
+
+	// make a transaction that's not in the pool
+	tx2 := makeTx(false)
+
+	// add to pool once more to the pool
+	makeTx(true)
+
+	// l1 derivation block building, blocklist disabled
+	r2 := miner.generateWork(&generateParams{
+		parentHash: parentBlock.Hash(),
+		timestamp:  parentBlock.Time + 1,
+		random:     common.HexToHash("0xcafebabe"),
+		noTxs:      true,
+		forceTime:  true,
+		isPending:  false,
+		txs:        types.Transactions{tx2},
+	}, false)
+
+	require.NoError(t, r2.err)
+
+	// the mempool tx should not be included since we are omitting the mempool
+	require.Equal(t, 1, len(r2.block.Transactions()), "block should have 1 transaction")
+	require.Equal(t, 1, len(r2.receipts), "block should have 1 transaction receipts")
+	require.False(t, tx2.Rejected(), "tx should not be rejected")
+
+	// mempool block building, blocklist enabled
+	// and the tx has a blocked fee-currency
+	r3 := miner.generateWork(&generateParams{
+		parentHash: parentBlock.Hash(),
+		timestamp:  parentBlock.Time + 1,
+		random:     common.HexToHash("0xcafebabe"),
+		noTxs:      false,
+		forceTime:  true,
+		isPending:  false,
+	}, false)
+	require.NoError(t, r3.err)
+
+	// third tx should not be included since it's using a blocked fee-currency
+	require.Equal(t, 0, len(r3.block.Transactions()), "block should have 0 transactions")
+}

miner/miner.go

@@ -223,6 +223,7 @@ func (miner *Miner) getPending() *newPayloadResult {
 		withdrawals: withdrawal,
 		beaconRoot:  nil,
 		noTxs:       false,
+		isPending:   true,
 	}, false) // we will never make a witness for a pending block
 	if ret.err != nil {
 		return nil

miner/worker.go

@@ -90,6 +90,9 @@ type environment struct {
 	multiGasPool         *core.MultiGasPool // available per-fee-currency gas used to pack transactions
 	feeCurrencyAllowlist common.AddressSet
 	feeCurrencyContext   *common.FeeCurrencyContext
+	// used for derivation / pending block building - in that case we don't
+	// want to use any blocklist features
+	blocklistsDisabled bool
 }
 
 const (
@@ -128,6 +131,7 @@ type generateParams struct {
 	eip1559Params []byte             // Optional EIP-1559 parameters
 	interrupt     *atomic.Int32      // Optional interruption signal to pass down to worker.generateWork
 	isUpdate      bool               // Optional flag indicating that this is building a discardable update
+	isPending     bool               // Optional flag indicating that this is building a pending block
 
 	rpcCtx context.Context // context to control block-building RPC work. No RPC allowed if nil.
 }
@@ -158,6 +162,7 @@ func (miner *Miner) generateWork(params *generateParams, witness bool) *newPaylo
 			work.feeCurrencyAllowlist,
 			miner.config.FeeCurrencyDefault,
 			miner.config.FeeCurrencyLimits,
+			work.noTxs,
 		)
 	}
 
@@ -173,7 +178,8 @@ func (miner *Miner) generateWork(params *generateParams, witness bool) *newPaylo
 		// the non-fee currency pool in the multipool is not used, but for consistency
 		// subtract the gas. Don't check the error either, this has been checked already
 		// with the work.gasPool.
-		work.multiGasPool.PoolFor(nil).SubGas(tx.Gas())
+		pool, _ := work.multiGasPool.PoolFor(nil)
+		pool.SubGas(tx.Gas())
 	}
 	if !params.noTxs {
 		// use shared interrupt if present
@@ -336,18 +342,28 @@ func (miner *Miner) prepareWork(genParams *generateParams, witness bool) (*envir
 		return nil, err
 	}
 	env.noTxs = genParams.noTxs
-	if evicted := miner.feeCurrencyBlocklist.Evict(parent); len(evicted) > 0 {
-		log.Warn(
-			"Evicted temporarily blocked fee-currencies from local block-list",
-			"evicted-fee-currencies", evicted,
-			"eviction-timeout-seconds", EvictionTimeoutSeconds,
+	// don't do anything blocklist related when we are:
+	// - deriving from L1
+	// - building the pending block
+	// because this could poison the blocklist or prevent
+	// derivation from executing consensus blocks when the blocklist
+	// is poisoned.
+	env.blocklistsDisabled = env.noTxs || genParams.isPending
+	env.feeCurrencyAllowlist = common.CurrencyAllowlist(env.feeCurrencyContext.ExchangeRates)
+	if !env.blocklistsDisabled {
+		if evicted := miner.feeCurrencyBlocklist.Evict(parent); len(evicted) > 0 {
+			log.Warn(
+				"Evicted temporarily blocked fee-currencies from local block-list",
+				"evicted-fee-currencies", evicted,
+				"eviction-timeout-seconds", EvictionTimeoutSeconds,
+			)
+			feeCurrenciesInBlocklistCounter.Dec(int64(len(evicted)))
+		}
+		env.feeCurrencyAllowlist = miner.feeCurrencyBlocklist.FilterAllowlist(
+			env.feeCurrencyAllowlist,
+			header,
 		)
-		feeCurrenciesInBlocklistCounter.Dec(int64(len(evicted)))
 	}
-	env.feeCurrencyAllowlist = miner.feeCurrencyBlocklist.FilterAllowlist(
-		common.CurrencyAllowlist(env.feeCurrencyContext.ExchangeRates),
-		header,
-	)
 
 	if header.ParentBeaconRoot != nil {
 		core.ProcessBeaconBlockRoot(*header.ParentBeaconRoot, env.evm)
@@ -421,8 +437,8 @@ func (miner *Miner) commitTransaction(env *environment, tx *types.Transaction) e
 	receipt, err := miner.applyTransaction(env, tx)
 	if err != nil {
 		if errors.Is(err, contracts.ErrFeeCurrencyEVMCall) {
-			log.Warn(
-				"fee-currency EVM execution error, temporarily blocking fee-currency in local txpools",
+			log.Error(
+				"fee-currency EVM execution error",
 				"tx-hash", tx.Hash(),
 				"fee-currency", tx.FeeCurrency(),
 				"error", err.Error(),
@@ -548,6 +564,7 @@ func (miner *Miner) commitTransactions(env *environment, plainTxs, blobTxs *tran
 			env.feeCurrencyAllowlist,
 			miner.config.FeeCurrencyDefault,
 			miner.config.FeeCurrencyLimits,
+			env.noTxs,
 		)
 	}
 	for {
@@ -626,7 +643,8 @@ func (miner *Miner) commitTransactions(env *environment, plainTxs, blobTxs *tran
 				continue
 			}
 		}
-		if left := env.multiGasPool.PoolFor(ltx.FeeCurrency).Gas(); left < ltx.Gas {
+		pool, _ := env.multiGasPool.PoolFor(ltx.FeeCurrency)
+		if left := pool.Gas(); left < ltx.Gas {
 			log.Trace(
 				"Not enough specific fee-currency gas left for transaction",
 				"currency", ltx.FeeCurrency, "hash", ltx.Hash,
@@ -683,12 +701,13 @@ func (miner *Miner) commitTransactions(env *environment, plainTxs, blobTxs *tran
 			txs.Pop()
 
 		case errors.Is(err, nil):
-			err := env.multiGasPool.PoolFor(tx.FeeCurrency()).SubGas(gasUsed)
+			pool, _ := env.multiGasPool.PoolFor(tx.FeeCurrency())
+			err := pool.SubGas(gasUsed)
 			if err != nil {
 				// Should never happen as we check it above
 				log.Warn(
 					"Unexpectedly reached limit for fee currency, but tx will not be skipped",
-					"hash", tx.Hash(), "gas", env.multiGasPool.PoolFor(tx.FeeCurrency()).Gas(),
+					"hash", tx.Hash(), "gas", pool.Gas(),
 					"tx gas used", gasUsed,
 				)
 				// If we reach this codepath, we want to still include the transaction,
@@ -837,18 +856,33 @@ func (miner *Miner) validateParams(genParams *generateParams) (time.Duration, er
 	return time.Duration(blockTime) * time.Second, nil
 }
 
-func (miner *Miner) registerFeeCurrencyTxFailure(env *environment, tx *types.Transaction, err error) {
-	// the fee-currency is still in the allowlist of this environment,
-	// so set the fee-currency block gas limit to 0 to prevent other
-	// transactions.
-	if miner.feeCurrencyBlocklist.BlockingEnabled(*tx.FeeCurrency()) {
-		pool := env.multiGasPool.PoolFor(tx.FeeCurrency())
-		pool.SetGas(0)
-	}
-	// also add the fee-currency to a worker-wide blocklist,
-	// so that they are not allowlisted in the following blocks
-	// (only locally in the txpool, not consensus-critical)
-	if miner.feeCurrencyBlocklist.Add(*tx.FeeCurrency(), *env.header) {
-		feeCurrenciesInBlocklistCounter.Inc(1)
+func (miner *Miner) registerFeeCurrencyTxFailure(env *environment, tx *types.Transaction, _ error) {
+	if !env.blocklistsDisabled {
+		// add the fee-currency to a worker-wide blocklist,
+		// so that they are not allowlisted in the following blocks
+		// (only locally in the txpool, not consensus-critical)
+		if miner.feeCurrencyBlocklist.Add(*tx.FeeCurrency(), *env.header) {
+			log.Warn(
+				"added fee-currency to local blocklist",
+				"fee-currency", tx.FeeCurrency(),
+			)
+			feeCurrenciesInBlocklistCounter.Inc(1)
+		}
+
+		if miner.feeCurrencyBlocklist.BlockingEnabled(*tx.FeeCurrency()) {
+			// the fee-currency is still in the allowlist of this environment,
+			// so set the fee-currency block gas limit to 0 to prevent other
+			// transactions.
+			pool, hasSeparateMultiPool := env.multiGasPool.PoolFor(tx.FeeCurrency())
+			// if for whatever reason we didn't set a separate multipool
+			// for this fee-currency, we don't want to completely
+			// block all other gas usage
+			if hasSeparateMultiPool {
+				pool.SetGas(0)
+			} else {
+				log.Warn("blocked fee-currency does not have separate multi-gas-pool"+
+					", using default gas-pool for further block building", "fee-currency", tx.FeeCurrency())
+			}
+		}
 	}
 }