diff --git a/core/celo_multi_gaspool.go b/core/celo_multi_gaspool.go index c5bd7beed1..556108b667 100644 --- a/core/celo_multi_gaspool.go +++ b/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 { diff --git a/core/celo_multi_gaspool_test.go b/core/celo_multi_gaspool_test.go index 8c658ad4ae..9d8dd2b7d5 100644 --- a/core/celo_multi_gaspool_test.go +++ b/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, ) } diff --git a/miner/celo_miner_test.go b/miner/celo_miner_test.go index f85a11917e..472e324141 100644 --- a/miner/celo_miner_test.go +++ b/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") +} diff --git a/miner/miner.go b/miner/miner.go index cc04a7de07..4d80e6861d 100644 --- a/miner/miner.go +++ b/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 diff --git a/miner/worker.go b/miner/worker.go index da607529f2..4d57f1d0b8 100644 --- a/miner/worker.go +++ b/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()) + } + } } }