From ab3c77adca660558d1f10adbac248cf5fc480754 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Sun, 25 Jun 2023 15:21:22 -0500 Subject: [PATCH 01/23] Initial commit for discarding first transaction on revert in bundle --- miner/algo_common.go | 31 ++++++++++++++++++++++--------- miner/algo_greedy.go | 2 +- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/miner/algo_common.go b/miner/algo_common.go index a7145d56a2..5448c8a1f7 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -142,7 +142,7 @@ 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) { +func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData, dropTx bool) (*types.Receipt, int, error) { header := envDiff.header coinbase := &envDiff.baseEnvironment.coinbase signer := envDiff.baseEnvironment.signer @@ -156,6 +156,11 @@ func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData receipt, newState, err := applyTransactionWithBlacklist(signer, chData.chainConfig, chData.chain, coinbase, envDiff.gasPool, envDiff.state, header, tx, &header.GasUsed, *chData.chain.GetVMConfig(), chData.blacklist) + + if (err != nil && dropTx) || (receipt != nil && receipt.Status == types.ReceiptStatusFailed && dropTx) { + return receipt, shiftTx, err + } + envDiff.state = newState if err != nil { switch { @@ -207,8 +212,8 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa profitBefore := new(big.Int).Set(tmpEnvDiff.newProfit) var gasUsed uint64 - for _, tx := range bundle.OriginalBundle.Txs { - if tmpEnvDiff.header.BaseFee != nil && tx.Type() == 2 { + for index, tx := range bundle.OriginalBundle.Txs { + if tmpEnvDiff.header.BaseFee != nil && tx.Type() == types.DynamicFeeTxType { // Sanity check for extremely large numbers if tx.GasFeeCap().BitLen() > 256 { return core.ErrFeeCapVeryHigh @@ -240,16 +245,24 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa return errInterrupt } - receipt, _, err := tmpEnvDiff.commitTx(tx, chData) + canRevert := bundle.OriginalBundle.RevertingHash(tx.Hash()) + receipt, _, err := tmpEnvDiff.commitTx(tx, chData, index == 0 && canRevert) if err != nil { log.Trace("Bundle tx error", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "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.Status != types.ReceiptStatusSuccessful { + if canRevert { + if index == 0 { + log.Trace("Bundle tx failed, but can revert", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err) + continue + } + } else { + log.Trace("Bundle tx failed", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err) + return errors.New("bundle tx revert") + } } gasUsed += receipt.GasUsed @@ -387,7 +400,7 @@ func (envDiff *environmentDiff) commitPayoutTx(amount *big.Int, sender, receiver return nil, errors.New("incorrect sender private key") } - receipt, _, err := envDiff.commitTx(tx, chData) + receipt, _, err := envDiff.commitTx(tx, chData, false) if err != nil { return nil, err } @@ -465,7 +478,7 @@ func (envDiff *environmentDiff) commitSBundleInner(b *types.SBundle, chData chai coinbaseBefore = envDiff.state.GetBalance(envDiff.header.Coinbase) if el.Tx != nil { - receipt, _, err := envDiff.commitTx(el.Tx, chData) + receipt, _, err := envDiff.commitTx(el.Tx, chData, false) if err != nil { return err } diff --git a/miner/algo_greedy.go b/miner/algo_greedy.go index 0b36e89c45..7307d5b552 100644 --- a/miner/algo_greedy.go +++ b/miner/algo_greedy.go @@ -42,7 +42,7 @@ func (b *greedyBuilder) mergeOrdersIntoEnvDiff(envDiff *environmentDiff, orders } if tx := order.Tx(); tx != nil { - receipt, skip, err := envDiff.commitTx(tx, b.chainData) + receipt, skip, err := envDiff.commitTx(tx, b.chainData, false) switch skip { case shiftTx: orders.Shift() From 16ce37214dd9cf4d1cd568030b941974a5a937d6 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Fri, 30 Jun 2023 06:17:41 -0500 Subject: [PATCH 02/23] Update and simplify logic on reverts --- miner/algo_common.go | 52 +++++++++++++++++++++++++++----------------- miner/worker.go | 5 ++++- 2 files changed, 36 insertions(+), 21 deletions(-) diff --git a/miner/algo_common.go b/miner/algo_common.go index 5448c8a1f7..ced8caa6a2 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -157,7 +157,11 @@ func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData receipt, newState, err := applyTransactionWithBlacklist(signer, chData.chainConfig, chData.chain, coinbase, envDiff.gasPool, envDiff.state, header, tx, &header.GasUsed, *chData.chain.GetVMConfig(), chData.blacklist) - if (err != nil && dropTx) || (receipt != nil && receipt.Status == types.ReceiptStatusFailed && dropTx) { + // when drop transaction is enabled: + // 1. if there was an error applying the transaction OR + // 2. if the transaction receipt status is failed + // we don't apply the transaction to the state and we don't add it to the newTxs, return early + if dropTx && (err != nil || (receipt != nil && receipt.Status == types.ReceiptStatusFailed)) { return receipt, shiftTx, err } @@ -204,15 +208,21 @@ 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) 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) - for index, tx := range bundle.OriginalBundle.Txs { + gasUsed uint64 + canRevert bool + hasErr bool + hasReceiptFailed bool + ) + + for _, tx := range bundle.OriginalBundle.Txs { if tmpEnvDiff.header.BaseFee != nil && tx.Type() == types.DynamicFeeTxType { // Sanity check for extremely large numbers if tx.GasFeeCap().BitLen() > 256 { @@ -245,24 +255,26 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa return errInterrupt } - canRevert := bundle.OriginalBundle.RevertingHash(tx.Hash()) - receipt, _, err := tmpEnvDiff.commitTx(tx, chData, index == 0 && canRevert) + canRevert = bundle.OriginalBundle.RevertingHash(tx.Hash()) + receipt, _, err := tmpEnvDiff.commitTx(tx, chData, canRevert) - if err != nil { + hasErr = err != nil + hasReceiptFailed = receipt != nil && receipt.Status == types.ReceiptStatusFailed + + if canRevert && (hasErr || hasReceiptFailed) { + log.Trace("Failed to commit bundle transaction, but can revert", + "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err, "receipt-failed", hasReceiptFailed) + continue + } + + if hasErr { log.Trace("Bundle tx error", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err) return err } - if receipt.Status != types.ReceiptStatusSuccessful { - if canRevert { - if index == 0 { - log.Trace("Bundle tx failed, but can revert", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err) - continue - } - } else { - log.Trace("Bundle tx failed", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err) - return errors.New("bundle tx revert") - } + if hasReceiptFailed { + log.Trace("Bundle tx failed", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err) + return errors.New("bundle tx revert") } gasUsed += receipt.GasUsed diff --git a/miner/worker.go b/miner/worker.go index 136f3ca717..818477c0ff 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -1913,7 +1913,10 @@ func containsHash(arr []common.Hash, match common.Hash) bool { // Compute the adjusted gas price for a whole bundle // Done by calculating all gas spent, adding transfers to the coinbase, and then dividing by gas used -func (w *worker) computeBundleGas(env *environment, bundle types.MevBundle, state *state.StateDB, gasPool *core.GasPool, pendingTxs map[common.Address]types.Transactions, currentTxCount int) (simulatedBundle, error) { +func (w *worker) computeBundleGas( + env *environment, bundle types.MevBundle, state *state.StateDB, gasPool *core.GasPool, + pendingTxs map[common.Address]types.Transactions, currentTxCount int, +) (simulatedBundle, error) { var totalGasUsed uint64 = 0 var tempGasUsed uint64 gasFees := new(big.Int) From 4f20bca6cf27fda5e58a1e4ce09bbcea354018bc Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Sat, 1 Jul 2023 22:14:03 -0500 Subject: [PATCH 03/23] Add feature flag for toggling discard for reverted transaction hashes. Add support for dropping revertible transaction hashes in sbundle. Update logic to prevent naked transactions from being discarded since only bundles and sbundles currently support specifying revertible transaction hashes --- builder/builder.go | 3 ++ builder/config.go | 1 + builder/service.go | 1 + cmd/geth/main.go | 1 + cmd/utils/flags.go | 8 ++++ core/state/statedb.go | 4 ++ miner/algo_common.go | 74 +++++++++++++++++++++++++++--------- miner/algo_greedy.go | 12 ++++-- miner/algo_greedy_buckets.go | 26 ++++++------- miner/algo_greedy_test.go | 2 +- miner/algo_test.go | 48 ++++++++++++++++++++--- miner/multi_worker.go | 11 +++--- miner/worker.go | 23 ++++++----- 13 files changed, 156 insertions(+), 58 deletions(-) diff --git a/builder/builder.go b/builder/builder.go index 28fb3cfeb2..97a068a9c0 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -72,6 +72,7 @@ type Builder struct { builderPublicKey phase0.BLSPubKey builderSigningDomain phase0.Domain builderResubmitInterval time.Duration + discardRevertedHashes bool limiter *rate.Limiter submissionOffsetFromEndOfSlot time.Duration @@ -91,6 +92,7 @@ type BuilderArgs struct { relay IRelay builderSigningDomain phase0.Domain builderBlockResubmitInterval time.Duration + discardRevertedHashes 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, + discardRevertedHashes: args.discardRevertedHashes, submissionOffsetFromEndOfSlot: args.submissionOffsetFromEndOfSlot, limiter: args.limiter, diff --git a/builder/config.go b/builder/config.go index c1fef3ae32..aae0d5172d 100644 --- a/builder/config.go +++ b/builder/config.go @@ -25,6 +25,7 @@ type Config struct { BuilderRateLimitMaxBurst int `toml:",omitempty"` BuilderRateLimitResubmitInterval string `toml:",omitempty"` BuilderSubmissionOffset time.Duration `toml:",omitempty"` + DiscardRevertedHashes bool `toml:",omitempty"` EnableCancellations bool `toml:",omitempty"` } diff --git a/builder/service.go b/builder/service.go index cb8685a559..77a67d48ef 100644 --- a/builder/service.go +++ b/builder/service.go @@ -289,6 +289,7 @@ func Register(stack *node.Node, backend *eth.Ethereum, cfg *Config) error { builderSigningDomain: builderSigningDomain, builderBlockResubmitInterval: builderRateLimitInterval, submissionOffsetFromEndOfSlot: submissionOffset, + discardRevertedHashes: cfg.DiscardRevertedHashes, ignoreLatePayloadAttributes: cfg.IgnoreLatePayloadAttributes, validator: validator, beaconClient: beaconClient, diff --git a/cmd/geth/main.go b/cmd/geth/main.go index b5d3dd3553..cfb211d68b 100644 --- a/cmd/geth/main.go +++ b/cmd/geth/main.go @@ -179,6 +179,7 @@ var ( utils.BuilderRateLimitMaxBurst, utils.BuilderBlockResubmitInterval, utils.BuilderSubmissionOffset, + utils.BuilderDiscardRevertedHashes, utils.BuilderEnableCancellations, } diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 66f3da7618..8dee617527 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -845,6 +845,13 @@ var ( Category: flags.BuilderCategory, } + BuilderDiscardRevertedHashes = &cli.BoolFlag{ + Name: "builder.discard_reverted_hashes", + Usage: "When enabled, if a transaction submitted as part of a bundle in a send bundle request is reverted, " + + "and its hash is specified as one that can revert in the request body, the builder will discard the hash of the reverted 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", + } + BuilderEnableCancellations = &cli.BoolFlag{ Name: "builder.cancellations", Usage: "Enable cancellations for the builder", @@ -1668,6 +1675,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.DiscardRevertedHashes = ctx.Bool(BuilderDiscardRevertedHashes.Name) cfg.EnableCancellations = ctx.IsSet(BuilderEnableCancellations.Name) } diff --git a/core/state/statedb.go b/core/state/statedb.go index 256bd3e95f..c6d5da7db5 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -129,6 +129,10 @@ type StateDB struct { StorageDeleted int } +func (s *StateDB) OriginalRoot() common.Hash { + return s.originalRoot +} + // New creates a new state from a given trie. func New(root common.Hash, db Database, snaps *snapshot.Tree) (*StateDB, error) { tr, err := db.OpenTrie(root) diff --git a/miner/algo_common.go b/miner/algo_common.go index 5b2512ac8e..49e567919e 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -27,12 +27,10 @@ const ( const defaultProfitPercentMinimum = 70 var ( - defaultProfitThreshold = big.NewInt(defaultProfitPercentMinimum) defaultAlgorithmConfig = algorithmConfig{ DropTransactionOnRevert: false, EnforceProfit: false, - ExpectedProfit: common.Big0, - ProfitThresholdPercent: defaultProfitThreshold, + ProfitThresholdPercent: defaultProfitPercentMinimum, } ) @@ -63,10 +61,8 @@ type algorithmConfig struct { // 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 } type chainData struct { @@ -194,6 +190,7 @@ func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData header = envDiff.header coinbase = &envDiff.baseEnvironment.coinbase signer = envDiff.baseEnvironment.signer + root = envDiff.state.OriginalRoot() ) gasPrice, err := tx.EffectiveGasTip(header.BaseFee) @@ -202,7 +199,6 @@ func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData } 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) @@ -211,6 +207,13 @@ func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData // 2. if the transaction receipt status is failed // we don't apply the transaction to the state and we don't add it to the newTxs, return early if algoConf.DropTransactionOnRevert && (err != nil || (receipt != nil && receipt.Status == types.ReceiptStatusFailed)) { + s, sdbErr := state.New(root, envDiff.state.Database(), chData.chain.Snapshots()) + // if we cannot create a new state from the existing database, snapshots, and intermediate root hash, + // we panic because this is a fatal error + if sdbErr != nil { + panic(sdbErr) + } + envDiff.state = s return receipt, shiftTx, err } @@ -259,6 +262,9 @@ 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 { var ( + // Store the initial value for DropTransactionOnRevert as it will be mutated in the loop depending on whether a given transaction is revertible or not. + // Only sbundles and bundles currently support specifying revertible transactions. + discard = algoConf.DropTransactionOnRevert coinbase = envDiff.baseEnvironment.coinbase tmpEnvDiff = envDiff.copy() @@ -266,9 +272,11 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa profitBefore = new(big.Int).Set(tmpEnvDiff.newProfit) - gasUsed uint64 + gasUsed uint64 + hasErr bool hasReceiptFailed bool + canDiscard bool ) for _, tx := range bundle.OriginalBundle.Txs { @@ -304,12 +312,23 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa return errInterrupt } + // if drop transaction on revert is enabled and the transaction is found in list of reverting transaction hashes, + // we can consider the transaction for discarding + canDiscard = discard && bundle.OriginalBundle.RevertingHash(tx.Hash()) + algoConf.DropTransactionOnRevert = canDiscard + receipt, _, err := tmpEnvDiff.commitTx(tx, chData, algoConf) + // reset the drop transaction on revert flag for subsequent transactions + algoConf.DropTransactionOnRevert = discard + hasErr = err != nil hasReceiptFailed = receipt != nil && receipt.Status == types.ReceiptStatusFailed - if algoConf.DropTransactionOnRevert && (hasErr || hasReceiptFailed) { + // if drop transaction on revert is enabled and the transaction is found in list of reverting transaction hashes, + // when there was an error applying the transaction OR + // the transaction failed, we skip the transaction and continue with the next one + if canDiscard && (hasErr || hasReceiptFailed) { log.Trace("Failed to commit bundle transaction, but drop transaction on revert is enabled, skipping", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err, "receipt-failed", hasReceiptFailed) continue @@ -333,7 +352,16 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa bundleProfit := coinbaseBalanceDelta - bundleActualEffGP := bundleProfit.Div(bundleProfit, big.NewInt(int64(gasUsed))) + var ( + gasUsedBigInt = new(big.Int).SetUint64(gasUsed) + bundleActualEffGP *big.Int + ) + if gasUsed == 0 { + bundleActualEffGP = common.Big0 + } else { + bundleActualEffGP = bundleProfit.Div(bundleProfit, gasUsedBigInt) + } + bundleSimEffGP := new(big.Int).Set(bundle.MevGasPrice) // allow >-1% divergence @@ -351,11 +379,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 { @@ -500,7 +528,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 } @@ -535,7 +563,7 @@ func (envDiff *environmentDiff) commitSBundle(b *types.SimSBundle, chData chainD // 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 { @@ -551,7 +579,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 @@ -568,8 +598,9 @@ func (envDiff *environmentDiff) commitSBundleInner(b *types.SBundle, chData chai } var ( - algoConf = defaultAlgorithmConfig - + // Store the initial value for DropTransactionOnRevert as it will be mutated in the loop depending on whether a given transaction is revertible or not. + // Only sbundles and bundles currently support specifying revertible transactions. + discard = algoConf.DropTransactionOnRevert totalProfit *big.Int = new(big.Int) refundableProfit *big.Int = new(big.Int) @@ -582,7 +613,14 @@ func (envDiff *environmentDiff) commitSBundleInner(b *types.SBundle, chData chai coinbaseBefore = envDiff.state.GetBalance(envDiff.header.Coinbase) if el.Tx != nil { + // We only want to drop reverted transactions if they are specified as ones that can revert + // when they are submitted to the builder. Only bundles and sbundles currently support specifying + // revertible transactions. + algoConf.DropTransactionOnRevert = discard && el.CanRevert receipt, _, err := envDiff.commitTx(el.Tx, chData, algoConf) + // reset value for subsequent transactions or bundles + algoConf.DropTransactionOnRevert = discard + if err != nil { return err } @@ -590,7 +628,7 @@ func (envDiff *environmentDiff) commitSBundleInner(b *types.SBundle, chData chai 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 } diff --git a/miner/algo_greedy.go b/miner/algo_greedy.go index f4a778328c..23c1e6f2cb 100644 --- a/miner/algo_greedy.go +++ b/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, } } @@ -38,7 +43,6 @@ func (b *greedyBuilder) mergeOrdersIntoEnvDiff( envDiff *environmentDiff, orders *types.TransactionsByPriceAndNonce) ([]types.SimulatedBundle, []types.UsedSBundle, ) { var ( - algoConf = defaultAlgorithmConfig usedBundles []types.SimulatedBundle usedSbundles []types.UsedSBundle ) @@ -49,7 +53,7 @@ func (b *greedyBuilder) mergeOrdersIntoEnvDiff( } if tx := order.Tx(); tx != nil { - receipt, skip, err := envDiff.commitTx(tx, b.chainData, algoConf) + receipt, skip, err := envDiff.commitTx(tx, b.chainData, b.algoConf) switch skip { case shiftTx: orders.Shift() @@ -67,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) diff --git a/miner/algo_greedy_buckets.go b/miner/algo_greedy_buckets.go index c669ace317..53c0eefbdf 100644 --- a/miner/algo_greedy_buckets.go +++ b/miner/algo_greedy_buckets.go @@ -34,8 +34,7 @@ func newGreedyBucketsBuilder( if algoConf == nil { algoConf = &algorithmConfig{ EnforceProfit: true, - ExpectedProfit: nil, - ProfitThresholdPercent: defaultProfitThreshold, + ProfitThresholdPercent: defaultProfitPercentMinimum, } } return &greedyBucketsBuilder{ @@ -50,14 +49,8 @@ func newGreedyBucketsBuilder( // CutoffPriceFromOrder returns the cutoff price for a given order based on the cutoff percent. // For example, if the cutoff percent is 0.9, the cutoff price will be 90% of the order price, rounded down to the nearest integer. -func CutoffPriceFromOrder(order *types.TxWithMinerFee, cutoffPercent *big.Float) *big.Int { - floorPrice := new(big.Float). - Mul( - new(big.Float).SetInt(order.Price()), - cutoffPercent, - ) - round, _ := floorPrice.Int64() - return big.NewInt(round) +func CutoffPriceFromOrder(order *types.TxWithMinerFee, cutoffPercent int) *big.Int { + return common.PercentOf(order.Price(), cutoffPercent) } // IsOrderInPriceRange returns true if the order price is greater than or equal to the minPrice. @@ -71,9 +64,10 @@ func (b *greedyBucketsBuilder) commit(envDiff *environmentDiff, gasUsedMap map[*types.TxWithMinerFee]uint64, retryMap map[*types.TxWithMinerFee]int, retryLimit int, ) ([]types.SimulatedBundle, []types.UsedSBundle) { var ( + algoConf = b.algoConf + usedBundles []types.SimulatedBundle usedSbundles []types.UsedSBundle - algoConf = b.algoConf CheckRetryOrderAndReinsert = func( order *types.TxWithMinerFee, orders *types.TransactionsByPriceAndNonce, @@ -100,6 +94,11 @@ func (b *greedyBucketsBuilder) commit(envDiff *environmentDiff, for _, order := range transactions { if tx := order.Tx(); tx != nil { + // We only want to drop reverted transactions if they are specified as ones that can revert + // when they are submitted to the builder. Only bundles and sbundles currently support specifying + // revertible transactions. + algoConf := b.algoConf + algoConf.DropTransactionOnRevert = false receipt, skip, err := envDiff.commitTx(tx, b.chainData, algoConf) if err != nil { log.Trace("could not apply tx", "hash", tx.Hash(), "err", err) @@ -201,10 +200,7 @@ func (b *greedyBucketsBuilder) mergeOrdersIntoEnvDiff( usedBundles []types.SimulatedBundle usedSbundles []types.UsedSBundle transactions []*types.TxWithMinerFee - percent = new(big.Float).Quo( - new(big.Float).SetInt(b.algoConf.ProfitThresholdPercent), - new(big.Float).SetInt(common.Big100), - ) + percent = b.algoConf.ProfitThresholdPercent SortInPlaceByProfit = func(baseFee *big.Int, transactions []*types.TxWithMinerFee, gasUsedMap map[*types.TxWithMinerFee]uint64) { sort.SliceStable(transactions, func(i, j int) bool { diff --git a/miner/algo_greedy_test.go b/miner/algo_greedy_test.go index 8b84cf05ee..3d658b11cc 100644 --- a/miner/algo_greedy_test.go +++ b/miner/algo_greedy_test.go @@ -30,7 +30,7 @@ func TestBuildBlockGasLimit(t *testing.T) { var result *environment switch algo { case ALGO_GREEDY_BUCKETS: - builder := newGreedyBuilder(chData.chain, chData.chainConfig, nil, env, nil, nil) + builder := newGreedyBuilder(chData.chain, chData.chainConfig, nil, nil, env, nil, nil) result, _, _ = builder.buildBlock([]types.SimulatedBundle{}, nil, txs) case ALGO_GREEDY: builder := newGreedyBucketsBuilder(chData.chain, chData.chainConfig, nil, nil, env, nil, nil) diff --git a/miner/algo_test.go b/miner/algo_test.go index 09ce0bef5f..e364f27682 100644 --- a/miner/algo_test.go +++ b/miner/algo_test.go @@ -39,6 +39,7 @@ var algoTests = []*algoTest{ }, WantProfit: big.NewInt(2 * 21_000), SupportedAlgorithms: []AlgoType{ALGO_GREEDY, ALGO_GREEDY_BUCKETS}, + AlgorithmConfig: defaultAlgorithmConfig, }, { // Trivial tx pool with 3 txs by two accounts and a block gas limit that only allows two txs @@ -65,6 +66,7 @@ var algoTests = []*algoTest{ }, WantProfit: big.NewInt(4 * 21_000), SupportedAlgorithms: []AlgoType{ALGO_GREEDY, ALGO_GREEDY_BUCKETS}, + AlgorithmConfig: defaultAlgorithmConfig, }, { // Trivial bundle with one tx that reverts but is not allowed to revert. @@ -83,6 +85,7 @@ var algoTests = []*algoTest{ }, WantProfit: big.NewInt(0), SupportedAlgorithms: []AlgoType{ALGO_GREEDY, ALGO_GREEDY_BUCKETS}, + AlgorithmConfig: defaultAlgorithmConfig, }, { // Trivial bundle with one tx that reverts and is allowed to revert. @@ -104,6 +107,33 @@ var algoTests = []*algoTest{ }, WantProfit: big.NewInt(50_000), SupportedAlgorithms: []AlgoType{ALGO_GREEDY, ALGO_GREEDY_BUCKETS}, + AlgorithmConfig: defaultAlgorithmConfig, + }, + { + // Trivial bundle with one tx that reverts and is allowed to revert. + // + // Bundle should NOT be included since DropTransactionOnRevert is enabled. + Name: "atomic-bundle-revert-and-discard", + Header: &types.Header{GasLimit: 50_000}, + Alloc: []core.GenesisAccount{ + {Balance: big.NewInt(50_000)}, + {Code: contractRevert}, + }, + Bundles: func(acc accByIndex, sign signByIndex, txs txByAccIndexAndNonce) []*bundle { + return []*bundle{ + { + Txs: types.Transactions{sign(0, &types.LegacyTx{Nonce: 0, Gas: 50_000, To: acc(1), GasPrice: big.NewInt(1)})}, + RevertingTxIndices: []int{0}, + }, + } + }, + WantProfit: common.Big0, + SupportedAlgorithms: []AlgoType{ALGO_GREEDY, ALGO_GREEDY_BUCKETS}, + AlgorithmConfig: algorithmConfig{ + DropTransactionOnRevert: true, + EnforceProfit: defaultAlgorithmConfig.EnforceProfit, + ProfitThresholdPercent: defaultAlgorithmConfig.ProfitThresholdPercent, + }, }, { // Single failing tx that is included in the tx pool and in a bundle that is not allowed to @@ -130,6 +160,7 @@ var algoTests = []*algoTest{ }, WantProfit: big.NewInt(50_000), SupportedAlgorithms: []AlgoType{ALGO_GREEDY, ALGO_GREEDY_BUCKETS}, + AlgorithmConfig: defaultAlgorithmConfig, }, } @@ -152,7 +183,7 @@ func TestAlgo(t *testing.T) { t.Fatalf("Simulate Bundles: %v", err) } - gotProfit, err := runAlgoTest(algo, config, alloc, txPool, simBundles, test.Header, 1) + gotProfit, err := runAlgoTest(algo, test.AlgorithmConfig, config, alloc, txPool, simBundles, test.Header, 1) if err != nil { t.Fatal(err) } @@ -203,7 +234,7 @@ func BenchmarkAlgo(b *testing.B) { } }() - gotProfit, err := runAlgoTest(algo, config, alloc, txPoolCopy, simBundles, test.Header, scale) + gotProfit, err := runAlgoTest(algo, test.AlgorithmConfig, config, alloc, txPoolCopy, simBundles, test.Header, scale) if err != nil { b.Fatal(err) } @@ -218,8 +249,11 @@ func BenchmarkAlgo(b *testing.B) { } // runAlgo executes a single algoTest case and returns the profit. -func runAlgoTest(algo AlgoType, config *params.ChainConfig, alloc core.GenesisAlloc, - txPool map[common.Address]types.Transactions, bundles []types.SimulatedBundle, header *types.Header, scale int) (gotProfit *big.Int, err error) { +func runAlgoTest( + algo AlgoType, algoConf algorithmConfig, + config *params.ChainConfig, alloc core.GenesisAlloc, + txPool map[common.Address]types.Transactions, bundles []types.SimulatedBundle, header *types.Header, scale int, +) (gotProfit *big.Int, err error) { var ( statedb, chData = genTestSetupWithAlloc(config, alloc) env = newEnvironment(chData, statedb, header.Coinbase, header.GasLimit*uint64(scale), header.BaseFee) @@ -229,10 +263,10 @@ func runAlgoTest(algo AlgoType, config *params.ChainConfig, alloc core.GenesisAl // build block switch algo { case ALGO_GREEDY_BUCKETS: - builder := newGreedyBucketsBuilder(chData.chain, chData.chainConfig, nil, nil, env, nil, nil) + builder := newGreedyBucketsBuilder(chData.chain, chData.chainConfig, &algoConf, nil, env, nil, nil) resultEnv, _, _ = builder.buildBlock(bundles, nil, txPool) case ALGO_GREEDY: - builder := newGreedyBuilder(chData.chain, chData.chainConfig, nil, env, nil, nil) + builder := newGreedyBuilder(chData.chain, chData.chainConfig, &algoConf, nil, env, nil, nil) resultEnv, _, _ = builder.buildBlock(bundles, nil, txPool) } return resultEnv.profit, nil @@ -280,6 +314,8 @@ type algoTest struct { WantProfit *big.Int // Expected block profit SupportedAlgorithms []AlgoType + + AlgorithmConfig algorithmConfig } // setDefaults sets default values for the algoTest. diff --git a/miner/multi_worker.go b/miner/multi_worker.go index 93cb8aadae..d0e8411388 100644 --- a/miner/multi_worker.go +++ b/miner/multi_worker.go @@ -201,9 +201,10 @@ func newMultiWorkerMevGeth(config *Config, chainConfig *params.ChainConfig, engi } type flashbotsData struct { - isFlashbots bool - queue chan *task - maxMergedBundles int - algoType AlgoType - bundleCache *BundleCache + isFlashbots bool + queue chan *task + maxMergedBundles int + algoType AlgoType + bundleCache *BundleCache + discardRevertedHashes bool } diff --git a/miner/worker.go b/miner/worker.go index a961d69094..ac96092a12 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -1399,19 +1399,17 @@ func (w *worker) fillTransactionsAlgoWorker(interrupt *int32, env *environment) newEnv *environment blockBundles []types.SimulatedBundle usedSbundle []types.UsedSBundle + start = time.Now() ) - - start := time.Now() - switch w.flashbots.algoType { case ALGO_GREEDY_BUCKETS: - validationConf := &algorithmConfig{ - EnforceProfit: true, - ExpectedProfit: nil, - ProfitThresholdPercent: defaultProfitThreshold, + algoConf := &algorithmConfig{ + DropTransactionOnRevert: w.flashbots.discardRevertedHashes, + EnforceProfit: true, + ProfitThresholdPercent: defaultProfitPercentMinimum, } builder := newGreedyBucketsBuilder( - w.chain, w.chainConfig, validationConf, w.blockList, env, + w.chain, w.chainConfig, algoConf, w.blockList, env, w.config.BuilderTxSigningKey, interrupt, ) @@ -1419,8 +1417,15 @@ func (w *worker) fillTransactionsAlgoWorker(interrupt *int32, env *environment) case ALGO_GREEDY: fallthrough default: + // For default greedy builder, set algorithm configuration to default values, + // except DropTransactionOnRevert which is passed in from worker config + algoConf := &algorithmConfig{ + DropTransactionOnRevert: w.flashbots.discardRevertedHashes, + EnforceProfit: defaultAlgorithmConfig.EnforceProfit, + ProfitThresholdPercent: defaultAlgorithmConfig.ProfitThresholdPercent, + } builder := newGreedyBuilder( - w.chain, w.chainConfig, w.blockList, env, + w.chain, w.chainConfig, algoConf, w.blockList, env, w.config.BuilderTxSigningKey, interrupt, ) From fc81b92f1e2488ebd4ab23207a53590fb534d57e Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Sat, 1 Jul 2023 22:17:25 -0500 Subject: [PATCH 04/23] Update comment --- miner/algo_common.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/miner/algo_common.go b/miner/algo_common.go index 49e567919e..77994076f5 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -207,6 +207,8 @@ func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData // 2. if the transaction receipt status is failed // we don't apply the transaction to the state and we don't add it to the newTxs, return early if algoConf.DropTransactionOnRevert && (err != nil || (receipt != nil && receipt.Status == types.ReceiptStatusFailed)) { + // the StateDB for environment diff is already modified at this point, since it gets mutated when passed in to + // applyTransactionWithBlacklist, so we need to revert it s, sdbErr := state.New(root, envDiff.state.Database(), chData.chain.Snapshots()) // if we cannot create a new state from the existing database, snapshots, and intermediate root hash, // we panic because this is a fatal error From fa2ff4e9dc71fe0e7a30794be431439c096cd848 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Sat, 1 Jul 2023 22:21:27 -0500 Subject: [PATCH 05/23] Update comment --- miner/algo_common.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/miner/algo_common.go b/miner/algo_common.go index 77994076f5..dd78976f4c 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -327,9 +327,11 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa hasErr = err != nil hasReceiptFailed = receipt != nil && receipt.Status == types.ReceiptStatusFailed - // if drop transaction on revert is enabled and the transaction is found in list of reverting transaction hashes, - // when there was an error applying the transaction OR - // the transaction failed, we skip the transaction and continue with the next one + // if drop transaction on revert is enabled and the transaction is found in list of reverting transaction hashes: + // 1. when there was an error applying the transaction OR + // 2. the transaction failed + // + // we skip the transaction and continue with the next one if canDiscard && (hasErr || hasReceiptFailed) { log.Trace("Failed to commit bundle transaction, but drop transaction on revert is enabled, skipping", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err, "receipt-failed", hasReceiptFailed) From 721603a347a3936a977850d3970b3d76afeaadd7 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Sat, 1 Jul 2023 22:30:45 -0500 Subject: [PATCH 06/23] Update CLI flag for builder.discard_reverted_hashes --- builder/builder.go | 2 ++ cmd/utils/flags.go | 3 +++ 2 files changed, 5 insertions(+) diff --git a/builder/builder.go b/builder/builder.go index 97a068a9c0..c5befbb277 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -30,6 +30,8 @@ import ( ) const ( + DiscardRevertedHashesDefault = false + RateLimitIntervalDefault = 500 * time.Millisecond RateLimitBurstDefault = 10 BlockResubmitIntervalDefault = 500 * time.Millisecond diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 8dee617527..cb1e8cd9b5 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -850,6 +850,9 @@ var ( Usage: "When enabled, if a transaction submitted as part of a bundle in a send bundle request is reverted, " + "and its hash is specified as one that can revert in the request body, the builder will discard the hash of the reverted 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_REVERTED_HASHES"}, + Value: builder.DiscardRevertedHashesDefault, + Category: flags.BuilderCategory, } BuilderEnableCancellations = &cli.BoolFlag{ From cd05406796a30225f3fb1daa417c08d363cbf598 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Sat, 1 Jul 2023 22:46:30 -0500 Subject: [PATCH 07/23] Add godoc --- core/state/statedb.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/state/statedb.go b/core/state/statedb.go index c6d5da7db5..c2025e3cc0 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -129,6 +129,8 @@ type StateDB struct { StorageDeleted int } +// OriginalRoot returns the pre-state root hash before any changes were made. +// NOTE: This method is not safe for concurrent use. func (s *StateDB) OriginalRoot() common.Hash { return s.originalRoot } From 4af9cb7c94db0397251542bd4225c828d1f0e741 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Mon, 3 Jul 2023 15:50:44 -0500 Subject: [PATCH 08/23] Update greedy algorithm to only drop for bundles and sbundles --- miner/algo_greedy.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/miner/algo_greedy.go b/miner/algo_greedy.go index 23c1e6f2cb..25262c290c 100644 --- a/miner/algo_greedy.go +++ b/miner/algo_greedy.go @@ -53,7 +53,12 @@ func (b *greedyBuilder) mergeOrdersIntoEnvDiff( } if tx := order.Tx(); tx != nil { - receipt, skip, err := envDiff.commitTx(tx, b.chainData, b.algoConf) + // We only want to drop reverted transactions if they are specified as ones that can revert + // when they are submitted to the builder. Only bundles and sbundles currently support specifying + // revertible transactions. + algoConf := b.algoConf + algoConf.DropTransactionOnRevert = false + receipt, skip, err := envDiff.commitTx(tx, b.chainData, algoConf) switch skip { case shiftTx: orders.Shift() From 5fd4ba096beb8bd23d31154875d69d805eb16b89 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Mon, 3 Jul 2023 16:24:53 -0500 Subject: [PATCH 09/23] Add canRevert to algoconf to make it clearer when we discard transactions --- miner/algo_common.go | 23 ++++++++++++++--------- miner/algo_greedy.go | 2 +- miner/algo_greedy_buckets.go | 2 +- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/miner/algo_common.go b/miner/algo_common.go index dd78976f4c..31fd2b6761 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -58,6 +58,8 @@ type algorithmConfig struct { // DropTransactionOnRevert is set when a transaction is allowed to revert and we wish to not only revert the transaction // but discard it entirely DropTransactionOnRevert bool + + canRevert bool // EnforceProfit is true if we want to enforce a minimum profit threshold // for committing a transaction based on ProfitThresholdPercent EnforceProfit bool @@ -202,11 +204,14 @@ func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData receipt, newState, err := applyTransactionWithBlacklist(signer, chData.chainConfig, chData.chain, coinbase, envDiff.gasPool, envDiff.state, header, tx, &header.GasUsed, *chData.chain.GetVMConfig(), chData.blacklist) - // when drop transaction is enabled: + // when drop transaction is enabled, and transaction revert is enabled through bundle or sbundle: // 1. if there was an error applying the transaction OR // 2. if the transaction receipt status is failed // we don't apply the transaction to the state and we don't add it to the newTxs, return early - if algoConf.DropTransactionOnRevert && (err != nil || (receipt != nil && receipt.Status == types.ReceiptStatusFailed)) { + // NOTE: We only drop transaction when its specified in reverting transaction hashes for bundles and sbundles. + // Calling commitTx for a single transaction should set the boolean to false and not drop the transaction. + if algoConf.DropTransactionOnRevert && algoConf.canRevert && + (err != nil || (receipt != nil && receipt.Status == types.ReceiptStatusFailed)) { // the StateDB for environment diff is already modified at this point, since it gets mutated when passed in to // applyTransactionWithBlacklist, so we need to revert it s, sdbErr := state.New(root, envDiff.state.Database(), chData.chain.Snapshots()) @@ -266,9 +271,9 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa var ( // Store the initial value for DropTransactionOnRevert as it will be mutated in the loop depending on whether a given transaction is revertible or not. // Only sbundles and bundles currently support specifying revertible transactions. - discard = algoConf.DropTransactionOnRevert - coinbase = envDiff.baseEnvironment.coinbase - tmpEnvDiff = envDiff.copy() + canRevertDefault = algoConf.canRevert + coinbase = envDiff.baseEnvironment.coinbase + tmpEnvDiff = envDiff.copy() coinbaseBalanceBefore = tmpEnvDiff.state.GetBalance(coinbase) @@ -316,13 +321,13 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa // if drop transaction on revert is enabled and the transaction is found in list of reverting transaction hashes, // we can consider the transaction for discarding - canDiscard = discard && bundle.OriginalBundle.RevertingHash(tx.Hash()) - algoConf.DropTransactionOnRevert = canDiscard + algoConf.canRevert = bundle.OriginalBundle.RevertingHash(tx.Hash()) + canDiscard = algoConf.DropTransactionOnRevert && algoConf.canRevert receipt, _, err := tmpEnvDiff.commitTx(tx, chData, algoConf) - // reset the drop transaction on revert flag for subsequent transactions - algoConf.DropTransactionOnRevert = discard + // reset the canRevert flag for subsequent transactions + algoConf.canRevert = canRevertDefault hasErr = err != nil hasReceiptFailed = receipt != nil && receipt.Status == types.ReceiptStatusFailed diff --git a/miner/algo_greedy.go b/miner/algo_greedy.go index 25262c290c..5f9f491c5f 100644 --- a/miner/algo_greedy.go +++ b/miner/algo_greedy.go @@ -57,7 +57,7 @@ func (b *greedyBuilder) mergeOrdersIntoEnvDiff( // when they are submitted to the builder. Only bundles and sbundles currently support specifying // revertible transactions. algoConf := b.algoConf - algoConf.DropTransactionOnRevert = false + algoConf.canRevert = false receipt, skip, err := envDiff.commitTx(tx, b.chainData, algoConf) switch skip { case shiftTx: diff --git a/miner/algo_greedy_buckets.go b/miner/algo_greedy_buckets.go index 53c0eefbdf..9c5401c538 100644 --- a/miner/algo_greedy_buckets.go +++ b/miner/algo_greedy_buckets.go @@ -98,7 +98,7 @@ func (b *greedyBucketsBuilder) commit(envDiff *environmentDiff, // when they are submitted to the builder. Only bundles and sbundles currently support specifying // revertible transactions. algoConf := b.algoConf - algoConf.DropTransactionOnRevert = false + algoConf.canRevert = false receipt, skip, err := envDiff.commitTx(tx, b.chainData, algoConf) if err != nil { log.Trace("could not apply tx", "hash", tx.Hash(), "err", err) From 09c021629eebf12bab8431e846bb65352f8f41ae Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Mon, 3 Jul 2023 19:58:12 -0500 Subject: [PATCH 10/23] Update comment --- miner/algo_common.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/miner/algo_common.go b/miner/algo_common.go index 31fd2b6761..5ce0265417 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -209,7 +209,7 @@ func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData // 2. if the transaction receipt status is failed // we don't apply the transaction to the state and we don't add it to the newTxs, return early // NOTE: We only drop transaction when its specified in reverting transaction hashes for bundles and sbundles. - // Calling commitTx for a single transaction should set the boolean to false and not drop the transaction. + // Calling commitTx for a single transaction should set canRevert to false and not drop the transaction. if algoConf.DropTransactionOnRevert && algoConf.canRevert && (err != nil || (receipt != nil && receipt.Status == types.ReceiptStatusFailed)) { // the StateDB for environment diff is already modified at this point, since it gets mutated when passed in to From 95e4c7cb8518e0fa26a7e581bd5455167f660a20 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Mon, 3 Jul 2023 20:02:03 -0500 Subject: [PATCH 11/23] Update Sbundle --- miner/algo_greedy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/miner/algo_greedy.go b/miner/algo_greedy.go index 5f9f491c5f..253797d828 100644 --- a/miner/algo_greedy.go +++ b/miner/algo_greedy.go @@ -89,7 +89,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) From 481b80120dec5c2311c80004502315f5f6bce809 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Mon, 3 Jul 2023 22:46:04 -0500 Subject: [PATCH 12/23] Update sbundle to use canRevert --- miner/algo_common.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/miner/algo_common.go b/miner/algo_common.go index 5ce0265417..41b0ed2ef8 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -607,9 +607,9 @@ func (envDiff *environmentDiff) commitSBundleInner( } var ( - // Store the initial value for DropTransactionOnRevert as it will be mutated in the loop depending on whether a given transaction is revertible or not. + // Store the initial value for canRevert as it will be mutated in the loop depending on whether a given transaction is revertible or not. // Only sbundles and bundles currently support specifying revertible transactions. - discard = algoConf.DropTransactionOnRevert + canRevertDefault = algoConf.canRevert totalProfit *big.Int = new(big.Int) refundableProfit *big.Int = new(big.Int) @@ -625,10 +625,10 @@ func (envDiff *environmentDiff) commitSBundleInner( // We only want to drop reverted transactions if they are specified as ones that can revert // when they are submitted to the builder. Only bundles and sbundles currently support specifying // revertible transactions. - algoConf.DropTransactionOnRevert = discard && el.CanRevert + algoConf.canRevert = el.CanRevert receipt, _, err := envDiff.commitTx(el.Tx, chData, algoConf) // reset value for subsequent transactions or bundles - algoConf.DropTransactionOnRevert = discard + algoConf.canRevert = canRevertDefault if err != nil { return err From dcc7507761d9670b97abb3d4b80a461d4143a72a Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Tue, 4 Jul 2023 14:27:50 -0500 Subject: [PATCH 13/23] Update logic based on PR feedback - only discard revertible transactions on error, remove unnecessary from commit tx function, add additional unit test that includes bundle when it contains revertible tx that we wish to discard --- builder/builder.go | 2 - builder/config.go | 1 + cmd/utils/flags.go | 1 - miner/algo_common.go | 102 ++++++++++++----------------------- miner/algo_common_test.go | 22 ++++---- miner/algo_greedy.go | 7 +-- miner/algo_greedy_buckets.go | 7 +-- miner/algo_test.go | 46 +++++++++++++--- miner/worker.go | 14 ++--- 9 files changed, 93 insertions(+), 109 deletions(-) diff --git a/builder/builder.go b/builder/builder.go index c5befbb277..97a068a9c0 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -30,8 +30,6 @@ import ( ) const ( - DiscardRevertedHashesDefault = false - RateLimitIntervalDefault = 500 * time.Millisecond RateLimitBurstDefault = 10 BlockResubmitIntervalDefault = 500 * time.Millisecond diff --git a/builder/config.go b/builder/config.go index aae0d5172d..9649ff8e0f 100644 --- a/builder/config.go +++ b/builder/config.go @@ -51,6 +51,7 @@ var DefaultConfig = Config{ ValidationBlocklist: "", BuilderRateLimitDuration: RateLimitIntervalDefault.String(), BuilderRateLimitMaxBurst: RateLimitBurstDefault, + DiscardRevertedHashes: false, EnableCancellations: false, } diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index cb1e8cd9b5..5ca867b762 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -851,7 +851,6 @@ var ( "and its hash is specified as one that can revert in the request body, the builder will discard the hash of the reverted 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_REVERTED_HASHES"}, - Value: builder.DiscardRevertedHashesDefault, Category: flags.BuilderCategory, } diff --git a/miner/algo_common.go b/miner/algo_common.go index 41b0ed2ef8..139e522f99 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -28,9 +28,9 @@ const defaultProfitPercentMinimum = 70 var ( defaultAlgorithmConfig = algorithmConfig{ - DropTransactionOnRevert: false, - EnforceProfit: false, - ProfitThresholdPercent: defaultProfitPercentMinimum, + DropRevertibleTxOnErr: false, + EnforceProfit: false, + ProfitThresholdPercent: defaultProfitPercentMinimum, } ) @@ -55,11 +55,11 @@ func (e *lowProfitError) Error() string { } type algorithmConfig struct { - // DropTransactionOnRevert is set when a transaction is allowed to revert and we wish to not only revert the transaction - // but discard it entirely - DropTransactionOnRevert bool + // 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 - canRevert bool // EnforceProfit is true if we want to enforce a minimum profit threshold // for committing a transaction based on ProfitThresholdPercent EnforceProfit bool @@ -187,12 +187,11 @@ func applyTransactionWithBlacklist( } // commit tx to envDiff -func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData, algoConf algorithmConfig) (*types.Receipt, int, error) { +func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData) (*types.Receipt, int, error) { var ( header = envDiff.header coinbase = &envDiff.baseEnvironment.coinbase signer = envDiff.baseEnvironment.signer - root = envDiff.state.OriginalRoot() ) gasPrice, err := tx.EffectiveGasTip(header.BaseFee) @@ -204,26 +203,6 @@ func (envDiff *environmentDiff) commitTx(tx *types.Transaction, chData chainData receipt, newState, err := applyTransactionWithBlacklist(signer, chData.chainConfig, chData.chain, coinbase, envDiff.gasPool, envDiff.state, header, tx, &header.GasUsed, *chData.chain.GetVMConfig(), chData.blacklist) - // when drop transaction is enabled, and transaction revert is enabled through bundle or sbundle: - // 1. if there was an error applying the transaction OR - // 2. if the transaction receipt status is failed - // we don't apply the transaction to the state and we don't add it to the newTxs, return early - // NOTE: We only drop transaction when its specified in reverting transaction hashes for bundles and sbundles. - // Calling commitTx for a single transaction should set canRevert to false and not drop the transaction. - if algoConf.DropTransactionOnRevert && algoConf.canRevert && - (err != nil || (receipt != nil && receipt.Status == types.ReceiptStatusFailed)) { - // the StateDB for environment diff is already modified at this point, since it gets mutated when passed in to - // applyTransactionWithBlacklist, so we need to revert it - s, sdbErr := state.New(root, envDiff.state.Database(), chData.chain.Snapshots()) - // if we cannot create a new state from the existing database, snapshots, and intermediate root hash, - // we panic because this is a fatal error - if sdbErr != nil { - panic(sdbErr) - } - envDiff.state = s - return receipt, shiftTx, err - } - envDiff.state = newState if err != nil { switch { @@ -269,11 +248,8 @@ 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 { var ( - // Store the initial value for DropTransactionOnRevert as it will be mutated in the loop depending on whether a given transaction is revertible or not. - // Only sbundles and bundles currently support specifying revertible transactions. - canRevertDefault = algoConf.canRevert - coinbase = envDiff.baseEnvironment.coinbase - tmpEnvDiff = envDiff.copy() + coinbase = envDiff.baseEnvironment.coinbase + tmpEnvDiff = envDiff.copy() coinbaseBalanceBefore = tmpEnvDiff.state.GetBalance(coinbase) @@ -281,12 +257,13 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa gasUsed uint64 - hasErr bool + hasCommitErr bool hasReceiptFailed bool - canDiscard bool + isRevertibleTx bool ) 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 { @@ -319,37 +296,28 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa return errInterrupt } - // if drop transaction on revert is enabled and the transaction is found in list of reverting transaction hashes, - // we can consider the transaction for discarding - algoConf.canRevert = bundle.OriginalBundle.RevertingHash(tx.Hash()) - canDiscard = algoConf.DropTransactionOnRevert && algoConf.canRevert - - receipt, _, err := tmpEnvDiff.commitTx(tx, chData, algoConf) + // update isRevertibleTx if transaction is found in list of reverting transaction hashes + isRevertibleTx = bundle.OriginalBundle.RevertingHash(txHash) - // reset the canRevert flag for subsequent transactions - algoConf.canRevert = canRevertDefault + receipt, _, err := tmpEnvDiff.commitTx(tx, chData) - hasErr = err != nil + hasCommitErr = err != nil hasReceiptFailed = receipt != nil && receipt.Status == types.ReceiptStatusFailed - // if drop transaction on revert is enabled and the transaction is found in list of reverting transaction hashes: - // 1. when there was an error applying the transaction OR - // 2. the transaction failed - // - // we skip the transaction and continue with the next one - if canDiscard && (hasErr || hasReceiptFailed) { - log.Trace("Failed to commit bundle transaction, but drop transaction on revert is enabled, skipping", - "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err, "receipt-failed", hasReceiptFailed) + // if drop enabled, and revertible tx has error on commit, we skip the transaction and continue with next one + if algoConf.DropRevertibleTxOnErr && isRevertibleTx && hasCommitErr { + log.Warn("Found error on commit for revertible tx, but discard on err is enabled so skipping.", + "tx", txHash, "err", err) continue } - if hasErr { - log.Trace("Bundle tx error", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err) + if hasCommitErr { + log.Trace("Bundle tx error", "bundle", bundle.OriginalBundle.Hash, "tx", txHash, "err", err) return err } - if hasReceiptFailed && !bundle.OriginalBundle.RevertingHash(tx.Hash()) { - log.Trace("Bundle tx failed", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err) + if hasReceiptFailed && !isRevertibleTx { + log.Trace("Bundle tx failed", "bundle", bundle.OriginalBundle.Hash, "tx", txHash, "err", err) return errors.New("bundle tx revert") } @@ -519,7 +487,7 @@ func (envDiff *environmentDiff) commitPayoutTx(amount *big.Int, sender, receiver return nil, errors.New("incorrect sender private key") } - receipt, _, err := envDiff.commitTx(tx, chData, defaultAlgorithmConfig) + receipt, _, err := envDiff.commitTx(tx, chData) if err != nil { return nil, err } @@ -607,9 +575,6 @@ func (envDiff *environmentDiff) commitSBundleInner( } var ( - // Store the initial value for canRevert as it will be mutated in the loop depending on whether a given transaction is revertible or not. - // Only sbundles and bundles currently support specifying revertible transactions. - canRevertDefault = algoConf.canRevert totalProfit *big.Int = new(big.Int) refundableProfit *big.Int = new(big.Int) @@ -622,15 +587,16 @@ func (envDiff *environmentDiff) commitSBundleInner( coinbaseBefore = envDiff.state.GetBalance(envDiff.header.Coinbase) if el.Tx != nil { - // We only want to drop reverted transactions if they are specified as ones that can revert - // when they are submitted to the builder. Only bundles and sbundles currently support specifying - // revertible transactions. - algoConf.canRevert = el.CanRevert - receipt, _, err := envDiff.commitTx(el.Tx, chData, algoConf) - // reset value for subsequent transactions or bundles - algoConf.canRevert = canRevertDefault + 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.Warn("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 { diff --git a/miner/algo_common_test.go b/miner/algo_common_test.go index abd589d0ee..c845b5e256 100644 --- a/miner/algo_common_test.go +++ b/miner/algo_common_test.go @@ -234,7 +234,6 @@ func newEnvironment(data chainData, state *state.StateDB, coinbase common.Addres } func TestTxCommit(t *testing.T) { - algoConf := defaultAlgorithmConfig statedb, chData, signers := genTestSetup() env := newEnvironment(chData, statedb, signers.addresses[0], GasLimit, big.NewInt(1)) @@ -242,7 +241,7 @@ func TestTxCommit(t *testing.T) { tx := signers.signTx(1, 21000, big.NewInt(0), big.NewInt(1), signers.addresses[2], big.NewInt(0), []byte{}) - receipt, i, err := envDiff.commitTx(tx, chData, algoConf) + receipt, i, err := envDiff.commitTx(tx, chData) if err != nil { t.Fatal("can't commit transaction:", err) } @@ -325,7 +324,7 @@ func TestErrorTxCommit(t *testing.T) { signers.nonces[1] = 10 tx := signers.signTx(1, 21000, big.NewInt(0), big.NewInt(1), signers.addresses[2], big.NewInt(0), []byte{}) - _, i, err := envDiff.commitTx(tx, chData, defaultAlgorithmConfig) + _, i, err := envDiff.commitTx(tx, chData) if err == nil { t.Fatal("committed incorrect transaction:", err) } @@ -359,7 +358,7 @@ func TestCommitTxOverGasLimit(t *testing.T) { tx1 := signers.signTx(1, 21000, big.NewInt(0), big.NewInt(1), signers.addresses[2], big.NewInt(0), []byte{}) tx2 := signers.signTx(1, 21000, big.NewInt(0), big.NewInt(1), signers.addresses[2], big.NewInt(0), []byte{}) - receipt, i, err := envDiff.commitTx(tx1, chData, defaultAlgorithmConfig) + receipt, i, err := envDiff.commitTx(tx1, chData) if err != nil { t.Fatal("can't commit transaction:", err) } @@ -374,7 +373,7 @@ func TestCommitTxOverGasLimit(t *testing.T) { t.Fatal("Env diff gas pool is not drained") } - _, _, err = envDiff.commitTx(tx2, chData, defaultAlgorithmConfig) + _, _, err = envDiff.commitTx(tx2, chData) require.Error(t, err, "committed tx over gas limit") } @@ -400,7 +399,7 @@ func TestErrorBundleCommit(t *testing.T) { t.Fatal("Failed to simulate bundle", err) } - _, _, err = envDiff.commitTx(tx0, chData, defaultAlgorithmConfig) + _, _, err = envDiff.commitTx(tx0, chData) if err != nil { t.Fatal("Failed to commit tx0", err) } @@ -441,7 +440,6 @@ func TestErrorBundleCommit(t *testing.T) { } func TestBlacklist(t *testing.T) { - algoConf := defaultAlgorithmConfig statedb, chData, signers := genTestSetup() env := newEnvironment(chData, statedb, signers.addresses[0], GasLimit, big.NewInt(1)) @@ -459,13 +457,13 @@ func TestBlacklist(t *testing.T) { balanceBefore := envDiff.state.GetBalance(signers.addresses[3]) tx := signers.signTx(1, 21000, big.NewInt(0), big.NewInt(1), signers.addresses[3], big.NewInt(77), []byte{}) - _, _, err := envDiff.commitTx(tx, chData, algoConf) + _, _, err := envDiff.commitTx(tx, chData) if err == nil { t.Fatal("committed blacklisted transaction: to") } tx = signers.signTx(3, 21000, big.NewInt(0), big.NewInt(1), signers.addresses[1], big.NewInt(88), []byte{}) - _, _, err = envDiff.commitTx(tx, chData, algoConf) + _, _, err = envDiff.commitTx(tx, chData) if err == nil { t.Fatal("committed blacklisted transaction: sender") } @@ -474,19 +472,19 @@ func TestBlacklist(t *testing.T) { calldata = append(calldata, signers.addresses[3].Bytes()...) tx = signers.signTx(4, 40000, big.NewInt(0), big.NewInt(1), payProxyAddress, big.NewInt(99), calldata) - _, _, err = envDiff.commitTx(tx, chData, algoConf) + _, _, err = envDiff.commitTx(tx, chData) if err == nil { t.Fatal("committed blacklisted transaction: trace") } tx = signers.signTx(5, 40000, big.NewInt(0), big.NewInt(1), payProxyAddress, big.NewInt(0), calldata) - _, _, err = envDiff.commitTx(tx, chData, algoConf) + _, _, err = envDiff.commitTx(tx, chData) if err == nil { t.Fatal("committed blacklisted transaction: trace, zero value") } tx = signers.signTx(6, 30000, big.NewInt(0), big.NewInt(1), payProxyAddress, big.NewInt(99), calldata) - _, _, err = envDiff.commitTx(tx, chData, algoConf) + _, _, err = envDiff.commitTx(tx, chData) if err == nil { t.Fatal("committed blacklisted transaction: trace, failed tx") } diff --git a/miner/algo_greedy.go b/miner/algo_greedy.go index 253797d828..ae90d0a883 100644 --- a/miner/algo_greedy.go +++ b/miner/algo_greedy.go @@ -53,12 +53,7 @@ func (b *greedyBuilder) mergeOrdersIntoEnvDiff( } if tx := order.Tx(); tx != nil { - // We only want to drop reverted transactions if they are specified as ones that can revert - // when they are submitted to the builder. Only bundles and sbundles currently support specifying - // revertible transactions. - algoConf := b.algoConf - algoConf.canRevert = false - receipt, skip, err := envDiff.commitTx(tx, b.chainData, algoConf) + receipt, skip, err := envDiff.commitTx(tx, b.chainData) switch skip { case shiftTx: orders.Shift() diff --git a/miner/algo_greedy_buckets.go b/miner/algo_greedy_buckets.go index 9c5401c538..54fb03a5b1 100644 --- a/miner/algo_greedy_buckets.go +++ b/miner/algo_greedy_buckets.go @@ -94,12 +94,7 @@ func (b *greedyBucketsBuilder) commit(envDiff *environmentDiff, for _, order := range transactions { if tx := order.Tx(); tx != nil { - // We only want to drop reverted transactions if they are specified as ones that can revert - // when they are submitted to the builder. Only bundles and sbundles currently support specifying - // revertible transactions. - algoConf := b.algoConf - algoConf.canRevert = false - receipt, skip, err := envDiff.commitTx(tx, b.chainData, algoConf) + receipt, skip, err := envDiff.commitTx(tx, b.chainData) if err != nil { log.Trace("could not apply tx", "hash", tx.Hash(), "err", err) diff --git a/miner/algo_test.go b/miner/algo_test.go index e364f27682..c67ad89edd 100644 --- a/miner/algo_test.go +++ b/miner/algo_test.go @@ -110,10 +110,10 @@ var algoTests = []*algoTest{ AlgorithmConfig: defaultAlgorithmConfig, }, { - // Trivial bundle with one tx that reverts and is allowed to revert. + // Trivial bundle with one tx that has nonce error and fails. // - // Bundle should NOT be included since DropTransactionOnRevert is enabled. - Name: "atomic-bundle-revert-and-discard", + // Bundle should NOT be included since DropRevertibleTxOnErr is enabled. + Name: "atomic-bundle-nonce-error-and-discard", Header: &types.Header{GasLimit: 50_000}, Alloc: []core.GenesisAccount{ {Balance: big.NewInt(50_000)}, @@ -122,7 +122,7 @@ var algoTests = []*algoTest{ Bundles: func(acc accByIndex, sign signByIndex, txs txByAccIndexAndNonce) []*bundle { return []*bundle{ { - Txs: types.Transactions{sign(0, &types.LegacyTx{Nonce: 0, Gas: 50_000, To: acc(1), GasPrice: big.NewInt(1)})}, + Txs: types.Transactions{sign(0, &types.LegacyTx{Nonce: 1, Gas: 50_000, To: acc(1), GasPrice: big.NewInt(1)})}, RevertingTxIndices: []int{0}, }, } @@ -130,9 +130,41 @@ var algoTests = []*algoTest{ WantProfit: common.Big0, SupportedAlgorithms: []AlgoType{ALGO_GREEDY, ALGO_GREEDY_BUCKETS}, AlgorithmConfig: algorithmConfig{ - DropTransactionOnRevert: true, - EnforceProfit: defaultAlgorithmConfig.EnforceProfit, - ProfitThresholdPercent: defaultAlgorithmConfig.ProfitThresholdPercent, + DropRevertibleTxOnErr: true, + EnforceProfit: defaultAlgorithmConfig.EnforceProfit, + ProfitThresholdPercent: defaultAlgorithmConfig.ProfitThresholdPercent, + }, + }, + { + // Bundle with two transactions - first tx will revert and second has nonce error + // + // Bundle SHOULD be included ONLY with first tx since DropRevertibleTxOnErr is enabled. + Name: "bundle-with-revert-tx-and-invalid-nonce-discard", + Header: &types.Header{ + GasLimit: 3 * 21_000, + }, + Alloc: []core.GenesisAccount{ + {Balance: big.NewInt(3 * 21_000)}, + {Code: contractRevert}, + }, + Bundles: func(acc accByIndex, sign signByIndex, txs txByAccIndexAndNonce) []*bundle { + return []*bundle{ + { + Txs: types.Transactions{sign(0, &types.LegacyTx{Nonce: 0, Gas: 21_000, To: acc(1), GasPrice: big.NewInt(1)})}, + RevertingTxIndices: []int{0}, + }, + { + Txs: types.Transactions{sign(0, &types.LegacyTx{Nonce: 2, Gas: 42_000, To: acc(1), GasPrice: big.NewInt(1)})}, + RevertingTxIndices: []int{0}, + }, + } + }, + WantProfit: big.NewInt(21_000), + SupportedAlgorithms: []AlgoType{ALGO_GREEDY, ALGO_GREEDY_BUCKETS}, + AlgorithmConfig: algorithmConfig{ + DropRevertibleTxOnErr: true, + EnforceProfit: defaultAlgorithmConfig.EnforceProfit, + ProfitThresholdPercent: defaultAlgorithmConfig.ProfitThresholdPercent, }, }, { diff --git a/miner/worker.go b/miner/worker.go index ac96092a12..40f0bb3754 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -1404,9 +1404,9 @@ func (w *worker) fillTransactionsAlgoWorker(interrupt *int32, env *environment) switch w.flashbots.algoType { case ALGO_GREEDY_BUCKETS: algoConf := &algorithmConfig{ - DropTransactionOnRevert: w.flashbots.discardRevertedHashes, - EnforceProfit: true, - ProfitThresholdPercent: defaultProfitPercentMinimum, + DropRevertibleTxOnErr: w.flashbots.discardRevertedHashes, + EnforceProfit: true, + ProfitThresholdPercent: defaultProfitPercentMinimum, } builder := newGreedyBucketsBuilder( w.chain, w.chainConfig, algoConf, w.blockList, env, @@ -1418,11 +1418,11 @@ func (w *worker) fillTransactionsAlgoWorker(interrupt *int32, env *environment) fallthrough default: // For default greedy builder, set algorithm configuration to default values, - // except DropTransactionOnRevert which is passed in from worker config + // except DropRevertibleTxOnErr which is passed in from worker config algoConf := &algorithmConfig{ - DropTransactionOnRevert: w.flashbots.discardRevertedHashes, - EnforceProfit: defaultAlgorithmConfig.EnforceProfit, - ProfitThresholdPercent: defaultAlgorithmConfig.ProfitThresholdPercent, + DropRevertibleTxOnErr: w.flashbots.discardRevertedHashes, + EnforceProfit: defaultAlgorithmConfig.EnforceProfit, + ProfitThresholdPercent: defaultAlgorithmConfig.ProfitThresholdPercent, } builder := newGreedyBuilder( w.chain, w.chainConfig, algoConf, w.blockList, env, From 3d31190355204da644b40094455466fcdb96e80c Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Tue, 4 Jul 2023 14:33:46 -0500 Subject: [PATCH 14/23] Update parameter names to be clearer when revertible transaction has error --- builder/builder.go | 6 +++--- builder/config.go | 4 ++-- builder/service.go | 2 +- cmd/geth/main.go | 2 +- cmd/utils/flags.go | 10 +++++----- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/builder/builder.go b/builder/builder.go index 97a068a9c0..44201bcdeb 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -72,7 +72,7 @@ type Builder struct { builderPublicKey phase0.BLSPubKey builderSigningDomain phase0.Domain builderResubmitInterval time.Duration - discardRevertedHashes bool + discardRevertibleTxOnErr bool limiter *rate.Limiter submissionOffsetFromEndOfSlot time.Duration @@ -92,7 +92,7 @@ type BuilderArgs struct { relay IRelay builderSigningDomain phase0.Domain builderBlockResubmitInterval time.Duration - discardRevertedHashes bool + discardRevertibleTxOnErr bool eth IEthereumService dryRun bool ignoreLatePayloadAttributes bool @@ -138,7 +138,7 @@ func NewBuilder(args BuilderArgs) (*Builder, error) { builderPublicKey: pk, builderSigningDomain: args.builderSigningDomain, builderResubmitInterval: args.builderBlockResubmitInterval, - discardRevertedHashes: args.discardRevertedHashes, + discardRevertibleTxOnErr: args.discardRevertibleTxOnErr, submissionOffsetFromEndOfSlot: args.submissionOffsetFromEndOfSlot, limiter: args.limiter, diff --git a/builder/config.go b/builder/config.go index 9649ff8e0f..efadb78074 100644 --- a/builder/config.go +++ b/builder/config.go @@ -25,7 +25,7 @@ type Config struct { BuilderRateLimitMaxBurst int `toml:",omitempty"` BuilderRateLimitResubmitInterval string `toml:",omitempty"` BuilderSubmissionOffset time.Duration `toml:",omitempty"` - DiscardRevertedHashes bool `toml:",omitempty"` + DiscardRevertibleTxOnErr bool `toml:",omitempty"` EnableCancellations bool `toml:",omitempty"` } @@ -51,7 +51,7 @@ var DefaultConfig = Config{ ValidationBlocklist: "", BuilderRateLimitDuration: RateLimitIntervalDefault.String(), BuilderRateLimitMaxBurst: RateLimitBurstDefault, - DiscardRevertedHashes: false, + DiscardRevertibleTxOnErr: false, EnableCancellations: false, } diff --git a/builder/service.go b/builder/service.go index 77a67d48ef..55f4300c67 100644 --- a/builder/service.go +++ b/builder/service.go @@ -289,7 +289,7 @@ func Register(stack *node.Node, backend *eth.Ethereum, cfg *Config) error { builderSigningDomain: builderSigningDomain, builderBlockResubmitInterval: builderRateLimitInterval, submissionOffsetFromEndOfSlot: submissionOffset, - discardRevertedHashes: cfg.DiscardRevertedHashes, + discardRevertibleTxOnErr: cfg.DiscardRevertibleTxOnErr, ignoreLatePayloadAttributes: cfg.IgnoreLatePayloadAttributes, validator: validator, beaconClient: beaconClient, diff --git a/cmd/geth/main.go b/cmd/geth/main.go index cfb211d68b..265e3f4e81 100644 --- a/cmd/geth/main.go +++ b/cmd/geth/main.go @@ -179,7 +179,7 @@ var ( utils.BuilderRateLimitMaxBurst, utils.BuilderBlockResubmitInterval, utils.BuilderSubmissionOffset, - utils.BuilderDiscardRevertedHashes, + utils.BuilderDiscardRevertibleTxOnErr, utils.BuilderEnableCancellations, } diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 5ca867b762..f653016a28 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -845,10 +845,10 @@ var ( Category: flags.BuilderCategory, } - BuilderDiscardRevertedHashes = &cli.BoolFlag{ - Name: "builder.discard_reverted_hashes", - Usage: "When enabled, if a transaction submitted as part of a bundle in a send bundle request is reverted, " + - "and its hash is specified as one that can revert in the request body, the builder will discard the hash of the reverted transaction from the submitted bundle." + + 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_REVERTED_HASHES"}, Category: flags.BuilderCategory, @@ -1677,7 +1677,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.DiscardRevertedHashes = ctx.Bool(BuilderDiscardRevertedHashes.Name) + cfg.DiscardRevertibleTxOnErr = ctx.Bool(BuilderDiscardRevertibleTxOnErr.Name) cfg.EnableCancellations = ctx.IsSet(BuilderEnableCancellations.Name) } From a552260aa021f2918062df4effbc718dc8d7d9f3 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Tue, 4 Jul 2023 14:34:52 -0500 Subject: [PATCH 15/23] Remove unused function --- core/state/statedb.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/core/state/statedb.go b/core/state/statedb.go index c2025e3cc0..256bd3e95f 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -129,12 +129,6 @@ type StateDB struct { StorageDeleted int } -// OriginalRoot returns the pre-state root hash before any changes were made. -// NOTE: This method is not safe for concurrent use. -func (s *StateDB) OriginalRoot() common.Hash { - return s.originalRoot -} - // New creates a new state from a given trie. func New(root common.Hash, db Database, snaps *snapshot.Tree) (*StateDB, error) { tr, err := db.OpenTrie(root) From 2e5a5b3c001161f4c0be8b2c5a9388bcc0303c9e Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Tue, 4 Jul 2023 14:56:17 -0500 Subject: [PATCH 16/23] Update env var --- cmd/utils/flags.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index f653016a28..236f7cc56d 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -850,7 +850,7 @@ var ( 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_REVERTED_HASHES"}, + EnvVars: []string{"FLASHBOTS_BUILDER_DISCARD_REVERTIBLE_TX_ON_ERROR"}, Category: flags.BuilderCategory, } From 70f49142f04f3efac742c42b261ec2c4d5bb25a7 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Wed, 5 Jul 2023 09:25:46 -0500 Subject: [PATCH 17/23] Address PR feedback --- miner/algo_common.go | 49 +++++++++++++++----------------------------- 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/miner/algo_common.go b/miner/algo_common.go index 139e522f99..c0b94597a3 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -256,14 +256,14 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa profitBefore = new(big.Int).Set(tmpEnvDiff.newProfit) gasUsed uint64 - - hasCommitErr bool - hasReceiptFailed bool - isRevertibleTx bool ) for _, tx := range bundle.OriginalBundle.Txs { - txHash := tx.Hash() + var ( + txHash = tx.Hash() + // update isRevertibleTx if transaction is found in list of reverting transaction hashes + isRevertibleTx = bundle.OriginalBundle.RevertingHash(txHash) + ) if tmpEnvDiff.header.BaseFee != nil && tx.Type() == types.DynamicFeeTxType { // Sanity check for extremely large numbers if tx.GasFeeCap().BitLen() > 256 { @@ -296,27 +296,20 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa return errInterrupt } - // update isRevertibleTx if transaction is found in list of reverting transaction hashes - isRevertibleTx = bundle.OriginalBundle.RevertingHash(txHash) - receipt, _, err := tmpEnvDiff.commitTx(tx, chData) - hasCommitErr = err != nil - hasReceiptFailed = receipt != nil && receipt.Status == types.ReceiptStatusFailed - - // if drop enabled, and revertible tx has error on commit, we skip the transaction and continue with next one - if algoConf.DropRevertibleTxOnErr && isRevertibleTx && hasCommitErr { - log.Warn("Found error on commit for revertible tx, but discard on err is enabled so skipping.", - "tx", txHash, "err", err) - continue - } + 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 && isRevertibleTx { + log.Info("Found error on commit for revertible tx, but discard on err is enabled so skipping.", + "tx", txHash, "err", err) + continue + } - if hasCommitErr { log.Trace("Bundle tx error", "bundle", bundle.OriginalBundle.Hash, "tx", txHash, "err", err) return err - } - - if hasReceiptFailed && !isRevertibleTx { + } else if receipt != nil && receipt.Status == types.ReceiptStatusFailed && !isRevertibleTx { + // 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") } @@ -329,16 +322,8 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa bundleProfit := coinbaseBalanceDelta - var ( - gasUsedBigInt = new(big.Int).SetUint64(gasUsed) - bundleActualEffGP *big.Int - ) - if gasUsed == 0 { - bundleActualEffGP = common.Big0 - } else { - bundleActualEffGP = bundleProfit.Div(bundleProfit, gasUsedBigInt) - } - + gasUsedBigInt := new(big.Int).SetUint64(gasUsed) + bundleActualEffGP := bundleProfit.Div(bundleProfit, gasUsedBigInt) bundleSimEffGP := new(big.Int).Set(bundle.MevGasPrice) // allow >-1% divergence @@ -593,7 +578,7 @@ func (envDiff *environmentDiff) commitSBundleInner( // 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.Warn("Found error on commit for revertible tx, but discard on err is enabled so skipping.", + log.Info("Found error on commit for revertible tx, but discard on err is enabled so skipping.", "tx", el.Tx.Hash(), "err", err) continue } From 81c5a3427fd5ae0a355abb7f28090f7b89cdab7f Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Wed, 5 Jul 2023 09:44:54 -0500 Subject: [PATCH 18/23] Update to only instantiate isRevertibleTx in conditional paths --- miner/algo_common.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/miner/algo_common.go b/miner/algo_common.go index c0b94597a3..01629d119f 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -259,11 +259,7 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa ) for _, tx := range bundle.OriginalBundle.Txs { - var ( - txHash = tx.Hash() - // update isRevertibleTx if transaction is found in list of reverting transaction hashes - isRevertibleTx = bundle.OriginalBundle.RevertingHash(txHash) - ) + txHash := tx.Hash() if tmpEnvDiff.header.BaseFee != nil && tx.Type() == types.DynamicFeeTxType { // Sanity check for extremely large numbers if tx.GasFeeCap().BitLen() > 256 { @@ -299,6 +295,7 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa receipt, _, err := tmpEnvDiff.commitTx(tx, chData) if err != nil { + 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.Info("Found error on commit for revertible tx, but discard on err is enabled so skipping.", @@ -308,10 +305,13 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa log.Trace("Bundle tx error", "bundle", bundle.OriginalBundle.Hash, "tx", txHash, "err", err) return err - } else if receipt != nil && receipt.Status == types.ReceiptStatusFailed && !isRevertibleTx { - // 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 if receipt != nil && receipt.Status == types.ReceiptStatusFailed { + isRevertibleTx := bundle.OriginalBundle.RevertingHash(txHash) + if !isRevertibleTx { + // 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") + } } gasUsed += receipt.GasUsed From e8239529da88f5d03b24c93d38c35f3703d90725 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Wed, 5 Jul 2023 12:15:10 -0500 Subject: [PATCH 19/23] Update README --- README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/README.md b/README.md index 86dfd72537..25ffd0c821 100644 --- a/README.md +++ b/README.md @@ -52,6 +52,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 From 40ec427fb5117042834f0397603c7be2e837e93d Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Wed, 5 Jul 2023 12:25:00 -0500 Subject: [PATCH 20/23] Update profit percent comment --- miner/algo_common.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/miner/algo_common.go b/miner/algo_common.go index 01629d119f..2929a1d614 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -64,7 +64,7 @@ type algorithmConfig struct { // for committing a transaction based on ProfitThresholdPercent EnforceProfit bool // ProfitThresholdPercent is the minimum profit threshold for committing a transaction - ProfitThresholdPercent int + ProfitThresholdPercent int // 0-100, e.g. 70 means 70% } type chainData struct { From a1b5e6a2de62bc636cc9d2357e30158d52945e06 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Wed, 2 Aug 2023 14:14:17 -0500 Subject: [PATCH 21/23] Update log to debug, add assertion for nil receipt on commit bundle --- miner/algo_common.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/miner/algo_common.go b/miner/algo_common.go index b73ee08ce0..0ac1fabe01 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -311,20 +311,25 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa 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.Info("Found error on commit for revertible tx, but discard on err is enabled so skipping.", + log.Debug("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 - } else if receipt != nil && receipt.Status == types.ReceiptStatusFailed { - isRevertibleTx := bundle.OriginalBundle.RevertingHash(txHash) - if !isRevertibleTx { + } + + 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 @@ -591,7 +596,7 @@ func (envDiff *environmentDiff) commitSBundleInner( // 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.Info("Found error on commit for revertible tx, but discard on err is enabled so skipping.", + log.Debug("Found error on commit for revertible tx, but discard on err is enabled so skipping.", "tx", el.Tx.Hash(), "err", err) continue } From 0c4af4754bca37b7cc4b6c01b29a5f094a234ddd Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Thu, 3 Aug 2023 10:23:32 -0500 Subject: [PATCH 22/23] Add default value to CLI flag --- cmd/utils/flags.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 61d895f18c..4e9448d42d 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -877,6 +877,7 @@ var ( "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, } From 9e848abbafa820e2c71f08046d1c2b1a4867f220 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Thu, 3 Aug 2023 10:44:18 -0500 Subject: [PATCH 23/23] Update discard tx log to trace - debug is too noisy and less needed after testing --- miner/algo_common.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/miner/algo_common.go b/miner/algo_common.go index 0ac1fabe01..a24c5adf57 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -311,7 +311,7 @@ func (envDiff *environmentDiff) commitBundle(bundle *types.SimulatedBundle, chDa 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.Debug("Found error on commit for revertible tx, but discard on err is enabled so skipping.", + log.Trace("Found error on commit for revertible tx, but discard on err is enabled so skipping.", "tx", txHash, "err", err) continue } @@ -596,7 +596,7 @@ func (envDiff *environmentDiff) commitSBundleInner( // 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.Debug("Found error on commit for revertible tx, but discard on err is enabled so skipping.", + log.Trace("Found error on commit for revertible tx, but discard on err is enabled so skipping.", "tx", el.Tx.Hash(), "err", err) continue }