feedback

cmd/geth/main.go

@@ -152,6 +152,7 @@ var (
 		utils.GpoMinSuggestedPriorityFeeFlag,
 		utils.RollupSequencerHTTPFlag,
 		utils.RollupSequencerEnableTxConditionalFlag,
+		utils.RollupSequencerTxConditionalRateLimitFlag,
 		utils.RollupHistoricalRPCFlag,
 		utils.RollupHistoricalRPCTimeoutFlag,
 		utils.RollupDisableTxPoolGossipFlag,

cmd/utils/flags.go

@@ -914,6 +914,12 @@ var (
 		Category: flags.RollupCategory,
 		Value:    false,
 	}
+	RollupSequencerTxConditionalRateLimitFlag = &cli.Uint64Flag{
+		Name:     "rollup.sequencertxconditionalratelimit",
+		Usage:    "Maximum cost -- storage lookups -- allowed for conditional transactions in a given second",
+		Category: flags.RollupCategory,
+		Value:    5000,
+	}
 
 	// Metrics flags
 	MetricsEnabledFlag = &cli.BoolFlag{
@@ -1641,6 +1647,9 @@ func setMiner(ctx *cli.Context, cfg *miner.Config) {
 	if ctx.IsSet(RollupComputePendingBlock.Name) {
 		cfg.RollupComputePendingBlock = ctx.Bool(RollupComputePendingBlock.Name)
 	}
+	if ctx.IsSet(RollupSequencerTxConditionalRateLimitFlag.Name) {
+		cfg.RollupTransactionConditionalBurstRate = int(ctx.Uint64(RollupSequencerTxConditionalRateLimitFlag.Name))
+	}
 }
 
 func setRequiredBlocks(ctx *cli.Context, cfg *ethconfig.Config) {
@@ -1872,10 +1881,7 @@ func SetEthConfig(ctx *cli.Context, stack *node.Node, cfg *ethconfig.Config) {
 	cfg.RollupDisableTxPoolAdmission = cfg.RollupSequencerHTTP != "" && !ctx.Bool(RollupEnableTxPoolAdmissionFlag.Name)
 	cfg.RollupHaltOnIncompatibleProtocolVersion = ctx.String(RollupHaltOnIncompatibleProtocolVersionFlag.Name)
 	cfg.ApplySuperchainUpgrades = ctx.Bool(RollupSuperchainUpgradesFlag.Name)
-
-	if ctx.IsSet(RollupSequencerEnableTxConditionalFlag.Name) {
-		cfg.RollupSequencerEnableTxConditional = ctx.Bool(RollupSequencerEnableTxConditionalFlag.Name)
-	}
+	cfg.RollupSequencerEnableTxConditional = ctx.Bool(RollupSequencerEnableTxConditionalFlag.Name)
 
 	// Override any default configs for hard coded networks.
 	switch {

core/state/statedb.go

@@ -369,7 +369,15 @@ func (s *StateDB) HasSelfDestructed(addr common.Address) bool {
 }
 
 // CheckTransactionConditional validates the account preconditions against the statedb.
+//
+// NOTE: A lock is not held on the db while the conditional is checked. The caller must
+// ensure no state changes occur while this check is executed.
 func (s *StateDB) CheckTransactionConditional(cond *types.TransactionConditional) error {
+	cost := cond.Cost()
+	if cost > types.TransactionConditionalMaxCost {
+		return fmt.Errorf("conditional cost, %d, exceeded max: %d", cost, types.TransactionConditionalMaxCost)
+	}
+
 	for addr, acct := range cond.KnownAccounts {
 		if root, isRoot := acct.Root(); isRoot {
 			storageRoot := s.GetStorageRoot(addr)

core/txpool/legacypool/legacypool.go

@@ -101,9 +101,6 @@ var (
 	slotsGauge   = metrics.NewRegisteredGauge("txpool/slots", nil)
 
 	reheapTimer = metrics.NewRegisteredTimer("txpool/reheap", nil)
-
-	txConditionalDemotedCounter = metrics.NewRegisteredCounter("txpool/transactionConditional/demoted", nil)
-	txConditionalDemotedTimer   = metrics.NewRegisteredTimer("txpool/transactionConditional/demoted/elapsedtime", nil)
 )
 
 // BlockChain defines the minimal set of methods needed to back a tx pool with
@@ -1743,25 +1740,9 @@ func (pool *LegacyPool) demoteUnexecutables() {
 			pool.enqueueTx(hash, tx, false, false)
 		}
 
-		conditionalDrops, err := list.FilterTransactionConditionals(pool.currentState)
-		if err != nil {
-			log.Trace("%d stale transactions dropped with failed conditional: %w", len(conditionalDrops), err)
-		}
-
-		txConditionalDemotedCounter.Inc(int64(len(conditionalDrops)))
-		for _, tx := range conditionalDrops {
-			if conditional := tx.Conditional(); conditional != nil { // not nil but just in case
-				txConditionalDemotedTimer.UpdateSince(conditional.SubmissionTime)
-			}
-
-			hash := tx.Hash()
-			pool.all.Remove(tx.Hash())
-			log.Trace("Removed stale transaction with invalidated conditional", "hash", hash)
-		}
-
-		pendingGauge.Dec(int64(len(olds) + len(drops) + len(invalids) + len(conditionalDrops)))
+		pendingGauge.Dec(int64(len(olds) + len(drops) + len(invalids)))
 		if pool.locals.contains(addr) {
-			localGauge.Dec(int64(len(olds) + len(drops) + len(invalids) + len(conditionalDrops)))
+			localGauge.Dec(int64(len(olds) + len(drops) + len(invalids)))
 		}
 		// If there's a gap in front, alert (should never happen) and postpone all transactions
 		if list.Len() > 0 && list.txs.Get(nonce) == nil {

core/txpool/legacypool/list.go

@@ -18,8 +18,6 @@ package legacypool
 
 import (
 	"container/heap"
-	"errors"
-	"fmt"
 	"math"
 	"math/big"
 	"sort"
@@ -28,7 +26,6 @@ import (
 	"time"
 
 	"github.com/ethereum/go-ethereum/common"
-	"github.com/ethereum/go-ethereum/core/state"
 	"github.com/ethereum/go-ethereum/core/txpool"
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/holiman/uint256"
@@ -413,30 +410,6 @@ func (l *list) Filter(costLimit *uint256.Int, gasLimit uint64) (types.Transactio
 	return removed, invalids
 }
 
-// FilterTransactionConditionals removes transactions with an attached conditional against the provided state. Every
-// removed transaction is returned for post-removal maintenance. It is more costly than the operations in `Filter`
-// so it is split into its own method and is called less often. The error returned contains the contextual info
-// about the invalidated conditional for each transaction.
-func (l *list) FilterTransactionConditionals(statedb *state.StateDB) (types.Transactions, error) {
-	var errs []error
-	removed := l.txs.filter(func(tx *types.Transaction) bool {
-		if conditional := tx.Conditional(); conditional != nil {
-			if err := statedb.CheckTransactionConditional(conditional); err != nil {
-				errs = append(errs, fmt.Errorf("%s is a transaction with invalidated conditional: %w", tx.Hash(), err))
-				return true
-			}
-		}
-		return false
-	})
-
-	if len(removed) == 0 {
-		return nil, nil
-	}
-
-	l.txs.reheap()
-	return removed, errors.Join(errs...)
-}
-
 // Cap places a hard limit on the number of items, returning all transactions
 // exceeding that limit.
 func (l *list) Cap(threshold int) types.Transactions {

core/txpool/legacypool/list_test.go

@@ -22,8 +22,6 @@ import (
 	"testing"
 
 	"github.com/ethereum/go-ethereum/common"
-	"github.com/ethereum/go-ethereum/core/rawdb"
-	"github.com/ethereum/go-ethereum/core/state"
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/crypto"
 	"github.com/holiman/uint256"
@@ -70,67 +68,6 @@ func TestListAddVeryExpensive(t *testing.T) {
 	}
 }
 
-// TestFilterTransactionConditionals tests filtering by invalid TransactionConditionals.
-func TestFilterTransactionConditional(t *testing.T) {
-	// Create an in memory state db to test against.
-	state, _ := state.New(types.EmptyRootHash, state.NewDatabase(rawdb.NewMemoryDatabase()), nil)
-	state.IntermediateRoot(false)
-
-	// Create a private key to sign transactions.
-	key, _ := crypto.GenerateKey()
-
-	// Create a list.
-	list := newList(true)
-
-	// Create a transaction with no defined conditional and add to the list.
-	tx1 := transaction(0, 1000, key)
-	list.Add(tx1, DefaultConfig.PriceBump, nil)
-
-	// There should be no drops at this point, no conditional txs
-	drops, err := list.FilterTransactionConditionals(state)
-	if err != nil {
-		t.Fatalf("error filtering by TransactionConditionals: %s", err)
-	}
-	if count := len(drops); count != 0 {
-		t.Fatalf("got %d filtered by TransactionConditionals when there should not be any", count)
-	}
-
-	// Create another transaction with a conditional
-	tx2 := transaction(1, 1000, key)
-	tx2.SetConditional(&types.TransactionConditional{
-		KnownAccounts: map[common.Address]types.KnownAccount{{19: 1}: {StorageRoot: &types.EmptyRootHash}},
-	})
-	list.Add(tx2, DefaultConfig.PriceBump, nil)
-
-	// There should still be no drops as no state has been modified.
-	drops, err = list.FilterTransactionConditionals(state)
-	if err != nil {
-		t.Fatalf("error filtering by TransactionConditionals: %s", err)
-	}
-	if count := len(drops); count != 0 {
-		t.Fatalf("got %d filtered by TransactionConditionals when there should not be any", count)
-	}
-
-	// Set state that conflicts with tx2's conditional
-	state.SetState(common.Address{19: 1}, common.Hash{}, common.Hash{31: 1})
-	state.IntermediateRoot(false)
-
-	// tx2 should be the single transaction filtered out
-	drops, err = list.FilterTransactionConditionals(state)
-	if err == nil {
-		t.Fatalf("expected tx filtered by TransactionConditionals")
-	}
-	if count := len(drops); count != 1 {
-		t.Fatalf("got %d filtered by TransactionConditionals when there should be a single one", count)
-	}
-	if drops[0] != tx2 {
-		t.Fatalf("Got %x, expected %x", drops[0].Hash(), tx2.Hash())
-	}
-	if list.Len() != 1 {
-		t.Fatal("expected only 1 transaction remaining in the list")
-	}
-}
-
 func BenchmarkListAdd(b *testing.B) {
 	// Generate a list of transactions to insert
 	key, _ := crypto.GenerateKey()

core/types/transaction.go

@@ -66,7 +66,7 @@ type Transaction struct {
 	rollupCostData atomic.Value
 
 	// optional preconditions for inclusion
-	conditional *TransactionConditional
+	conditional atomic.Value
 }
 
 // NewTx creates a new transaction.
@@ -392,12 +392,16 @@ func (tx *Transaction) RollupCostData() RollupCostData {
 
 // Conditional returns the conditional attached to the transaction
 func (tx *Transaction) Conditional() *TransactionConditional {
-	return tx.conditional
+	v := tx.conditional.Load()
+	if v == nil {
+		return nil
+	}
+	return v.(*TransactionConditional)
 }
 
 // SetConditional attaches a conditional to the transaction
 func (tx *Transaction) SetConditional(cond *TransactionConditional) {
-	tx.conditional = cond
+	tx.conditional.Store(cond)
 }
 
 // RawSignatureValues returns the V, R, S signature values of the transaction.

core/types/transaction_conditional.go

@@ -12,6 +12,8 @@ import (
 	"github.com/ethereum/go-ethereum/rlp"
 )
 
+const TransactionConditionalMaxCost = 1000
+
 // KnownAccounts represents a set of KnownAccounts
 type KnownAccounts map[common.Address]KnownAccount
 
@@ -150,9 +152,8 @@ func (ka *KnownAccount) Slots() (map[common.Hash]common.Hash, bool) {
 
 //go:generate go run github.com/fjl/gencodec -type TransactionConditional -field-override transactionConditionalMarshalling -out gen_transaction_conditional_json.go
 
-// TransactionConditional represents the preconditions that determine
-// the inclusion of the transaction, enforced out-of-protocol by the
-// sequencer.
+// TransactionConditional represents the preconditions that determine the
+// inclusion of the transaction, enforced out-of-protocol by the sequencer.
 type TransactionConditional struct {
 	// KnownAccounts represents account prestate conditions
 	KnownAccounts KnownAccounts `json:"knownAccounts"`
@@ -220,18 +221,18 @@ func (cond *TransactionConditional) DecodeRLP(s *rlp.Stream) error {
 	return nil
 }
 
-// Validate will perform sanity checks on the specified parameters
+// Validate will perform sanity checks on the specified preconditions
 func (cond *TransactionConditional) Validate() error {
 	if cond.BlockNumberMin != nil && cond.BlockNumberMax != nil && cond.BlockNumberMin.Cmp(cond.BlockNumberMax) > 0 {
-		return fmt.Errorf("block number minimum constraint must be less than the max")
+		return fmt.Errorf("block number minimum constraint must be less than the maximum")
 	}
 	if cond.TimestampMin != nil && cond.TimestampMax != nil && *cond.TimestampMin > *cond.TimestampMax {
-		return fmt.Errorf("timestamp minimum constraint must be less than the max")
+		return fmt.Errorf("timestamp minimum constraint must be less than the maximum")
 	}
 	return nil
 }
 
-// Cost computes the cost of validating the TxOptions. It will return the number of storage lookups required
+// Cost computes the cost of checking the conditional -- total number of storage lookups required
 func (opts *TransactionConditional) Cost() int {
 	cost := 0
 	for _, account := range opts.KnownAccounts {

internal/sequencerapi/api.go

@@ -13,13 +13,11 @@ import (
 	"github.com/ethereum/go-ethereum/rpc"
 )
 
-const (
-	txConditionalMaxCost = 1000
-)
-
 var (
-	sendRawTxConditionalCostMeter = metrics.NewRegisteredMeter("sequencer/sendRawTransactionConditional/cost", nil)
+	conditionalTxRejectedErrCode        = -32003
+	conditionalTxCostExceededMaxErrCode = -32005
 
+	sendRawTxConditionalCostMeter       = metrics.NewRegisteredMeter("sequencer/sendRawTransactionConditional/cost", nil)
 	sendRawTxConditionalRequestsCounter = metrics.NewRegisteredCounter("sequencer/sendRawTransactionConditional/requests", nil)
 	sendRawTxConditionalAcceptedCounter = metrics.NewRegisteredCounter("sequencer/sendRawTransactionConditional/accepted", nil)
 )
@@ -40,24 +38,36 @@ func (s *sendRawTxCond) SendRawTransactionConditional(ctx context.Context, txByt
 
 	cost := cond.Cost()
 	sendRawTxConditionalCostMeter.Mark(int64(cost))
-	if cost > txConditionalMaxCost {
-		return common.Hash{}, fmt.Errorf("conditional cost, %d, exceeded 1000", cost)
+	if cost > types.TransactionConditionalMaxCost {
+		return common.Hash{}, &jsonRpcError{
+			message: fmt.Sprintf("conditional cost, %d, exceeded max: %d", cost, types.TransactionConditionalMaxCost),
+			code:    conditionalTxCostExceededMaxErrCode,
+		}
 	}
 
 	// Perform sanity validation prior to state lookups
 	if err := cond.Validate(); err != nil {
-		return common.Hash{}, fmt.Errorf("failed conditional validation: %s", err)
+		return common.Hash{}, &jsonRpcError{
+			message: fmt.Sprintf("failed conditional validation: %s", err),
+			code:    conditionalTxRejectedErrCode,
+		}
 	}
 
 	state, header, err := s.b.StateAndHeaderByNumber(context.Background(), rpc.LatestBlockNumber)
 	if err != nil {
 		return common.Hash{}, err
 	}
 	if err := header.CheckTransactionConditional(&cond); err != nil {
-		return common.Hash{}, fmt.Errorf("failed header check: %w", err)
+		return common.Hash{}, &jsonRpcError{
+			message: fmt.Sprintf("failed header check: %s", err),
+			code:    conditionalTxRejectedErrCode,
+		}
 	}
 	if err := state.CheckTransactionConditional(&cond); err != nil {
-		return common.Hash{}, fmt.Errorf("failed state check: %w", err)
+		return common.Hash{}, &jsonRpcError{
+			message: fmt.Sprintf("failed state check: %s", err),
+			code:    conditionalTxRejectedErrCode,
+		}
 	}
 
 	// We also check against the parent block to eliminate the MEV incentive in comparison with sendRawTransaction
@@ -67,7 +77,10 @@ func (s *sendRawTxCond) SendRawTransactionConditional(ctx context.Context, txByt
 		return common.Hash{}, err
 	}
 	if err := parentState.CheckTransactionConditional(&cond); err != nil {
-		return common.Hash{}, fmt.Errorf("failed parent header state check: %w", err)
+		return common.Hash{}, &jsonRpcError{
+			message: fmt.Sprintf("failed parent block %s state check: %s", header.ParentHash, err),
+			code:    conditionalTxRejectedErrCode,
+		}
 	}
 
 	tx := new(types.Transaction)

internal/sequencerapi/errors.go

@@ -0,0 +1,20 @@
+package sequencerapi
+
+import (
+	"github.com/ethereum/go-ethereum/rpc"
+)
+
+var _ rpc.Error = new(jsonRpcError)
+
+type jsonRpcError struct {
+	message string
+	code    int
+}
+
+func (j *jsonRpcError) Error() string {
+	return j.message
+}
+
+func (j *jsonRpcError) ErrorCode() int {
+	return j.code
+}

miner/miner.go

@@ -60,8 +60,10 @@ type Config struct {
 
 	NewPayloadTimeout time.Duration // The maximum time allowance for creating a new payload
 
-	RollupComputePendingBlock bool   // Compute the pending block from tx-pool, instead of copying the latest-block
-	EffectiveGasCeil          uint64 // if non-zero, a gas ceiling to apply independent of the header's gaslimit value
+	RollupComputePendingBlock             bool // Compute the pending block from tx-pool, instead of copying the latest-block
+	RollupTransactionConditionalBurstRate int  // Total number of conditional cost units allowed in a second
+
+	EffectiveGasCeil uint64 // if non-zero, a gas ceiling to apply independent of the header's gaslimit value
 }
 
 // DefaultConfig contains default settings for miner.