Toggleable discard for failed revertible transaction hashes (ethereum#81

)

* Add toggleable support for discarding reverted transaction hashes on error

* When a transaction that's part of a bundle fails, and the request sent to the builder sets field to allow revert for the transaction hash, the builder will discard the hash of the failed transaction from the submitted bundle.

README.md

@@ -59,6 +59,15 @@ $ geth --help
     --builder.cancellations        (default: false)
           Enable cancellations for the builder
 
+    --builder.discard_revertible_tx_on_error (default: false)
+          When enabled, if a transaction submitted as part of a bundle in a send bundle
+          request has error on commit, and its hash is specified as one that can revert in
+          the request body, the builder will discard the hash of the failed transaction
+          from the submitted bundle.For additional details on the structure of the request
+          body, see
+          https://docs.flashbots.net/flashbots-mev-share/searchers/understanding-bundles#bundle-definition
+          [$FLASHBOTS_BUILDER_DISCARD_REVERTIBLE_TX_ON_ERROR]
+
     --builder.dry-run              (default: false)
           Builder only validates blocks without submission to the relay
 

builder/builder.go

@@ -72,6 +72,7 @@ type Builder struct {
 	builderPublicKey            phase0.BLSPubKey
 	builderSigningDomain        phase0.Domain
 	builderResubmitInterval     time.Duration
+	discardRevertibleTxOnErr    bool
 
 	limiter                       *rate.Limiter
 	submissionOffsetFromEndOfSlot time.Duration
@@ -91,6 +92,7 @@ type BuilderArgs struct {
 	relay                         IRelay
 	builderSigningDomain          phase0.Domain
 	builderBlockResubmitInterval  time.Duration
+	discardRevertibleTxOnErr      bool
 	eth                           IEthereumService
 	dryRun                        bool
 	ignoreLatePayloadAttributes   bool
@@ -136,6 +138,7 @@ func NewBuilder(args BuilderArgs) (*Builder, error) {
 		builderPublicKey:              pk,
 		builderSigningDomain:          args.builderSigningDomain,
 		builderResubmitInterval:       args.builderBlockResubmitInterval,
+		discardRevertibleTxOnErr:      args.discardRevertibleTxOnErr,
 		submissionOffsetFromEndOfSlot: args.submissionOffsetFromEndOfSlot,
 
 		limiter:       args.limiter,

builder/config.go

@@ -25,6 +25,7 @@ type Config struct {
 	BuilderRateLimitMaxBurst         int           `toml:",omitempty"`
 	BuilderRateLimitResubmitInterval string        `toml:",omitempty"`
 	BuilderSubmissionOffset          time.Duration `toml:",omitempty"`
+	DiscardRevertibleTxOnErr         bool          `toml:",omitempty"`
 	EnableCancellations              bool          `toml:",omitempty"`
 }
 
@@ -50,6 +51,7 @@ var DefaultConfig = Config{
 	ValidationBlocklist:           "",
 	BuilderRateLimitDuration:      RateLimitIntervalDefault.String(),
 	BuilderRateLimitMaxBurst:      RateLimitBurstDefault,
+	DiscardRevertibleTxOnErr:      false,
 	EnableCancellations:           false,
 }
 

builder/service.go

@@ -289,6 +289,7 @@ func Register(stack *node.Node, backend *eth.Ethereum, cfg *Config) error {
 		builderSigningDomain:          builderSigningDomain,
 		builderBlockResubmitInterval:  builderRateLimitInterval,
 		submissionOffsetFromEndOfSlot: submissionOffset,
+		discardRevertibleTxOnErr:      cfg.DiscardRevertibleTxOnErr,
 		ignoreLatePayloadAttributes:   cfg.IgnoreLatePayloadAttributes,
 		validator:                     validator,
 		beaconClient:                  beaconClient,

cmd/geth/main.go

@@ -181,6 +181,7 @@ var (
 		utils.BuilderRateLimitMaxBurst,
 		utils.BuilderBlockResubmitInterval,
 		utils.BuilderSubmissionOffset,
+		utils.BuilderDiscardRevertibleTxOnErr,
 		utils.BuilderEnableCancellations,
 	}
 

cmd/utils/flags.go

@@ -871,6 +871,16 @@ var (
 		Category: flags.BuilderCategory,
 	}
 
+	BuilderDiscardRevertibleTxOnErr = &cli.BoolFlag{
+		Name: "builder.discard_revertible_tx_on_error",
+		Usage: "When enabled, if a transaction submitted as part of a bundle in a send bundle request has error on commit, " +
+			"and its hash is specified as one that can revert in the request body, the builder will discard the hash of the failed transaction from the submitted bundle." +
+			"For additional details on the structure of the request body, see https://docs.flashbots.net/flashbots-mev-share/searchers/understanding-bundles#bundle-definition",
+		EnvVars:  []string{"FLASHBOTS_BUILDER_DISCARD_REVERTIBLE_TX_ON_ERROR"},
+		Value:    builder.DefaultConfig.DiscardRevertibleTxOnErr,
+		Category: flags.BuilderCategory,
+	}
+
 	BuilderEnableCancellations = &cli.BoolFlag{
 		Name:     "builder.cancellations",
 		Usage:    "Enable cancellations for the builder",
@@ -1704,6 +1714,7 @@ func SetBuilderConfig(ctx *cli.Context, cfg *builder.Config) {
 	cfg.BuilderRateLimitDuration = ctx.String(BuilderRateLimitDuration.Name)
 	cfg.BuilderRateLimitMaxBurst = ctx.Int(BuilderRateLimitMaxBurst.Name)
 	cfg.BuilderSubmissionOffset = ctx.Duration(BuilderSubmissionOffset.Name)
+	cfg.DiscardRevertibleTxOnErr = ctx.Bool(BuilderDiscardRevertibleTxOnErr.Name)
 	cfg.EnableCancellations = ctx.IsSet(BuilderEnableCancellations.Name)
 	cfg.BuilderRateLimitResubmitInterval = ctx.String(BuilderBlockResubmitInterval.Name)
 }

miner/algo_common.go

@@ -23,20 +23,20 @@ const (
 )
 
 const (
-	// defaultProfitPercentMinimum is to ensure committed transactions, bundles, sbundles don't fall below this threshold
+	// defaultProfitThresholdPercent is to ensure committed transactions, bundles, sbundles don't fall below this threshold
 	// when profit is enforced
-	defaultProfitPercentMinimum = 70
+	defaultProfitThresholdPercent = 70
 
 	// defaultPriceCutoffPercent is for bucketing transactions by price, used for greedy buckets algorithm
 	defaultPriceCutoffPercent = 50
 )
 
 var (
-	defaultProfitThreshold = big.NewInt(defaultProfitPercentMinimum)
 	defaultAlgorithmConfig = algorithmConfig{
+		DropRevertibleTxOnErr:  false,
 		EnforceProfit:          false,
-		ExpectedProfit:         common.Big0,
-		ProfitThresholdPercent: defaultProfitThreshold,
+		ProfitThresholdPercent: defaultProfitThresholdPercent,
+		PriceCutoffPercent:     defaultPriceCutoffPercent,
 	}
 )
 
@@ -61,13 +61,18 @@ func (e *lowProfitError) Error() string {
 }
 
 type algorithmConfig struct {
+	// DropRevertibleTxOnErr is used when a revertible transaction has error on commit, and we wish to discard
+	// the transaction and continue processing the rest of a bundle or sbundle.
+	// Revertible transactions are specified as hashes that can revert in a bundle or sbundle.
+	DropRevertibleTxOnErr bool
+
 	// EnforceProfit is true if we want to enforce a minimum profit threshold
 	// for committing a transaction based on ProfitThresholdPercent
 	EnforceProfit bool
-	// ExpectedProfit should be set on a per transaction basis when profit is enforced
-	ExpectedProfit *big.Int
+
 	// ProfitThresholdPercent is the minimum profit threshold for committing a transaction
-	ProfitThresholdPercent *big.Int
+	ProfitThresholdPercent int // 0-100, e.g. 70 means 70%
+
 	// PriceCutoffPercent is the minimum effective gas price threshold used for bucketing transactions by price.
 	// For example if the top transaction in a list has an effective gas price of 1000 wei and PriceCutoffPercent
 	// is 10 (i.e. 10%), then the minimum effective gas price included in the same bucket as the top transaction
@@ -134,7 +139,11 @@ func checkInterrupt(i *int32) bool {
 
 // Simulate bundle on top of current state without modifying it
 // pending txs used to track if bundle tx is part of the mempool
-func applyTransactionWithBlacklist(signer types.Signer, config *params.ChainConfig, bc core.ChainContext, author *common.Address, gp *core.GasPool, statedb *state.StateDB, header *types.Header, tx *types.Transaction, usedGas *uint64, cfg vm.Config, blacklist map[common.Address]struct{}) (*types.Receipt, *state.StateDB, error) {
+func applyTransactionWithBlacklist(
+	signer types.Signer, config *params.ChainConfig, bc core.ChainContext, author *common.Address, gp *core.GasPool,
+	statedb *state.StateDB, header *types.Header, tx *types.Transaction, usedGas *uint64,
+	cfg vm.Config, blacklist map[common.Address]struct{},
+) (*types.Receipt, *state.StateDB, error) {
 	// short circuit if blacklist is empty
 	if len(blacklist) == 0 {
 		snap := statedb.Snapshot()
@@ -192,17 +201,18 @@ func applyTransactionWithBlacklist(signer types.Signer, config *params.ChainConf
 
 // commit tx to envDiff
 func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData) (*types.Receipt, int, error) {
-	header := envDiff.header
-	coinbase := &envDiff.baseEnvironment.coinbase
-	signer := envDiff.baseEnvironment.signer
+	var (
+		header   = envDiff.header
+		coinbase = &envDiff.baseEnvironment.coinbase
+		signer   = envDiff.baseEnvironment.signer
+	)
 
 	gasPrice, err := tx.EffectiveGasTip(header.BaseFee)
 	if err != nil {
 		return nil, shiftTx, err
 	}
 
 	envDiff.state.SetTxContext(tx.Hash(), envDiff.baseEnvironment.tcount+len(envDiff.newTxs))
-
 	receipt, newState, err := applyTransactionWithBlacklist(signer, chData.chainConfig, chData.chain, coinbase,
 		envDiff.gasPool, envDiff.state, header, tx, &header.GasUsed, *chData.chain.GetVMConfig(), chData.blacklist)
 
@@ -250,15 +260,19 @@ func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData
 
 // Commit Bundle to env diff
 func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chData chainData, interrupt *int32, algoConf algorithmConfig) error {
-	coinbase := envDiff.baseEnvironment.coinbase
-	tmpEnvDiff := envDiff.copy()
+	var (
+		coinbase   = envDiff.baseEnvironment.coinbase
+		tmpEnvDiff = envDiff.copy()
 
-	coinbaseBalanceBefore := tmpEnvDiff.state.GetBalance(coinbase)
+		coinbaseBalanceBefore = tmpEnvDiff.state.GetBalance(coinbase)
 
-	profitBefore := new(big.Int).Set(tmpEnvDiff.newProfit)
-	var gasUsed uint64
+		profitBefore = new(big.Int).Set(tmpEnvDiff.newProfit)
+
+		gasUsed uint64
+	)
 
 	for _, tx := range bundle.OriginalBundle.Txs {
+		txHash := tx.Hash()
 		if tmpEnvDiff.header.BaseFee != nil && tx.Type() == types.DynamicFeeTxType {
 			// Sanity check for extremely large numbers
 			if tx.GasFeeCap().BitLen() > 256 {
@@ -294,13 +308,28 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa
 		receipt, _, err := tmpEnvDiff.commitTx(tx, chData)
 
 		if err != nil {
-			log.Trace("Bundle tx error", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err)
+			isRevertibleTx := bundle.OriginalBundle.RevertingHash(txHash)
+			// if drop enabled, and revertible tx has error on commit, we skip the transaction and continue with next one
+			if algoConf.DropRevertibleTxOnErr && isRevertibleTx {
+				log.Trace("Found error on commit for revertible tx, but discard on err is enabled so skipping.",
+					"tx", txHash, "err", err)
+				continue
+			}
+
+			log.Trace("Bundle tx error", "bundle", bundle.OriginalBundle.Hash, "tx", txHash, "err", err)
 			return err
 		}
 
-		if receipt.Status != types.ReceiptStatusSuccessful && !bundle.OriginalBundle.RevertingHash(tx.Hash()) {
-			log.Trace("Bundle tx failed", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err)
-			return errors.New("bundle tx revert")
+		if receipt != nil {
+			if receipt.Status == types.ReceiptStatusFailed && !bundle.OriginalBundle.RevertingHash(txHash) {
+				// if transaction reverted and isn't specified as reverting hash, return error
+				log.Trace("Bundle tx failed", "bundle", bundle.OriginalBundle.Hash, "tx", txHash, "err", err)
+				return errors.New("bundle tx revert")
+			}
+		} else {
+			// NOTE: The expectation is that a receipt is only nil if an error occurred.
+			//  If there is no error but receipt is nil, there is likely a programming error.
+			return errors.New("invalid receipt when no error occurred")
 		}
 
 		gasUsed += receipt.GasUsed
@@ -311,7 +340,8 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa
 
 	bundleProfit := coinbaseBalanceDelta
 
-	bundleActualEffGP := bundleProfit.Div(bundleProfit, big.NewInt(int64(gasUsed)))
+	gasUsedBigInt := new(big.Int).SetUint64(gasUsed)
+	bundleActualEffGP := bundleProfit.Div(bundleProfit, gasUsedBigInt)
 	bundleSimEffGP := new(big.Int).Set(bundle.MevGasPrice)
 
 	// allow >-1% divergence
@@ -329,11 +359,11 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa
 	if algoConf.EnforceProfit {
 		// if profit is enforced between simulation and actual commit, only allow ProfitThresholdPercent divergence
 		simulatedBundleProfit := new(big.Int).Set(bundle.TotalEth)
-		actualBundleProfit := new(big.Int).Mul(bundleActualEffGP, big.NewInt(int64(gasUsed)))
+		actualBundleProfit := new(big.Int).Mul(bundleActualEffGP, gasUsedBigInt)
 
 		// We want to make simulated profit smaller to allow for some leeway in cases where the actual profit is
 		// lower due to transaction ordering
-		simulatedProfitMultiple := new(big.Int).Mul(simulatedBundleProfit, algoConf.ProfitThresholdPercent)
+		simulatedProfitMultiple := common.PercentOf(simulatedBundleProfit, algoConf.ProfitThresholdPercent)
 		actualProfitMultiple := new(big.Int).Mul(actualBundleProfit, common.Big100)
 
 		if simulatedProfitMultiple.Cmp(actualProfitMultiple) > 0 {
@@ -478,7 +508,7 @@ func (envDiff *environmentDiff) commitSBundle(b *types.SimSBundle, chData chainD
 	coinbaseBefore := tmpEnvDiff.state.GetBalance(tmpEnvDiff.header.Coinbase)
 	gasBefore := tmpEnvDiff.gasPool.Gas()
 
-	if err := tmpEnvDiff.commitSBundleInner(b.Bundle, chData, interrupt, key); err != nil {
+	if err := tmpEnvDiff.commitSBundleInner(b.Bundle, chData, interrupt, key, algoConf); err != nil {
 		return err
 	}
 
@@ -507,13 +537,13 @@ func (envDiff *environmentDiff) commitSBundle(b *types.SimSBundle, chData chainD
 	}
 
 	if algoConf.EnforceProfit {
-		// if profit is enforced between simulation and actual commit, only allow >-1% divergence
+		// if profit is enforced between simulation and actual commit, only allow ProfitThresholdPercent divergence
 		simulatedSbundleProfit := new(big.Int).Set(b.Profit)
 		actualSbundleProfit := new(big.Int).Set(coinbaseDelta)
 
 		// We want to make simulated profit smaller to allow for some leeway in cases where the actual profit is
 		// lower due to transaction ordering
-		simulatedProfitMultiple := new(big.Int).Mul(simulatedSbundleProfit, algoConf.ProfitThresholdPercent)
+		simulatedProfitMultiple := common.PercentOf(simulatedSbundleProfit, algoConf.ProfitThresholdPercent)
 		actualProfitMultiple := new(big.Int).Mul(actualSbundleProfit, common.Big100)
 
 		if simulatedProfitMultiple.Cmp(actualProfitMultiple) > 0 {
@@ -529,7 +559,9 @@ func (envDiff *environmentDiff) commitSBundle(b *types.SimSBundle, chData chainD
 	return nil
 }
 
-func (envDiff *environmentDiff) commitSBundleInner(b *types.SBundle, chData chainData, interrupt *int32, key *ecdsa.PrivateKey) error {
+func (envDiff *environmentDiff) commitSBundleInner(
+	b *types.SBundle, chData chainData, interrupt *int32, key *ecdsa.PrivateKey, algoConf algorithmConfig,
+) error {
 	// check inclusion
 	minBlock := b.Inclusion.BlockNumber
 	maxBlock := b.Inclusion.MaxBlockNumber
@@ -548,9 +580,7 @@ func (envDiff *environmentDiff) commitSBundleInner(b *types.SBundle, chData chai
 	var (
 		totalProfit      *big.Int = new(big.Int)
 		refundableProfit *big.Int = new(big.Int)
-	)
 
-	var (
 		coinbaseDelta  = new(big.Int)
 		coinbaseBefore *big.Int
 	)
@@ -561,14 +591,22 @@ func (envDiff *environmentDiff) commitSBundleInner(b *types.SBundle, chData chai
 
 		if el.Tx != nil {
 			receipt, _, err := envDiff.commitTx(el.Tx, chData)
+
 			if err != nil {
+				// if drop enabled, and revertible tx has error on commit,
+				// we skip the transaction and continue with next one
+				if algoConf.DropRevertibleTxOnErr && el.CanRevert {
+					log.Trace("Found error on commit for revertible tx, but discard on err is enabled so skipping.",
+						"tx", el.Tx.Hash(), "err", err)
+					continue
+				}
 				return err
 			}
 			if receipt.Status != types.ReceiptStatusSuccessful && !el.CanRevert {
 				return errors.New("tx failed")
 			}
 		} else if el.Bundle != nil {
-			err := envDiff.commitSBundleInner(el.Bundle, chData, interrupt, key)
+			err := envDiff.commitSBundleInner(el.Bundle, chData, interrupt, key, algoConf)
 			if err != nil {
 				return err
 			}

miner/algo_common_test.go

@@ -280,6 +280,7 @@ func TestTxCommit(t *testing.T) {
 }
 
 func TestBundleCommit(t *testing.T) {
+	algoConf := defaultAlgorithmConfig
 	statedb, chData, signers := genTestSetup()
 
 	env := newEnvironment(chData, statedb, signers.addresses[0], GasLimit, big.NewInt(1))
@@ -298,7 +299,7 @@ func TestBundleCommit(t *testing.T) {
 		t.Fatal("Failed to simulate bundle", err)
 	}
 
-	err = envDiff.commitBundle(&simBundle, chData, nil, defaultAlgorithmConfig)
+	err = envDiff.commitBundle(&simBundle, chData, nil, algoConf)
 	if err != nil {
 		t.Fatal("Failed to commit bundle", err)
 	}

miner/algo_greedy.go

@@ -20,17 +20,22 @@ type greedyBuilder struct {
 	chainData        chainData
 	builderKey       *ecdsa.PrivateKey
 	interrupt        *int32
+	algoConf         algorithmConfig
 }
 
 func newGreedyBuilder(
-	chain *core.BlockChain, chainConfig *params.ChainConfig,
+	chain *core.BlockChain, chainConfig *params.ChainConfig, algoConf *algorithmConfig,
 	blacklist map[common.Address]struct{}, env *environment, key *ecdsa.PrivateKey, interrupt *int32,
 ) *greedyBuilder {
+	if algoConf == nil {
+		algoConf = &defaultAlgorithmConfig
+	}
 	return &greedyBuilder{
 		inputEnvironment: env,
 		chainData:        chainData{chainConfig, chain, blacklist},
 		builderKey:       key,
 		interrupt:        interrupt,
+		algoConf:         *algoConf,
 	}
 }
 
@@ -66,7 +71,7 @@ func (b *greedyBuilder) mergeOrdersIntoEnvDiff(
 			}
 		} else if bundle := order.Bundle(); bundle != nil {
 			//log.Debug("buildBlock considering bundle", "egp", bundle.MevGasPrice.String(), "hash", bundle.OriginalBundle.Hash)
-			err := envDiff.commitBundle(bundle, b.chainData, b.interrupt, defaultAlgorithmConfig)
+			err := envDiff.commitBundle(bundle, b.chainData, b.interrupt, b.algoConf)
 			orders.Pop()
 			if err != nil {
 				log.Trace("Could not apply bundle", "bundle", bundle.OriginalBundle.Hash, "err", err)
@@ -79,7 +84,7 @@ func (b *greedyBuilder) mergeOrdersIntoEnvDiff(
 			usedEntry := types.UsedSBundle{
 				Bundle: sbundle.Bundle,
 			}
-			err := envDiff.commitSBundle(sbundle, b.chainData, b.interrupt, b.builderKey, defaultAlgorithmConfig)
+			err := envDiff.commitSBundle(sbundle, b.chainData, b.interrupt, b.builderKey, b.algoConf)
 			orders.Pop()
 			if err != nil {
 				log.Trace("Could not apply sbundle", "bundle", sbundle.Bundle.Hash(), "err", err)