Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toggleable discard for failed revertible transaction hashes #81

Merged
merged 26 commits into from
Aug 3, 2023
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
ab3c77a
Initial commit for discarding first transaction on revert in bundle
Wazzymandias Jun 25, 2023
16ce372
Update and simplify logic on reverts
Wazzymandias Jun 30, 2023
6b6e815
Merge remote-tracking branch 'origin/main' into bundle-tx-discard
Wazzymandias Jul 1, 2023
4f20bca
Add feature flag for toggling discard for reverted transaction hashes…
Wazzymandias Jul 2, 2023
fc81b92
Update comment
Wazzymandias Jul 2, 2023
fa2ff4e
Update comment
Wazzymandias Jul 2, 2023
721603a
Update CLI flag for builder.discard_reverted_hashes
Wazzymandias Jul 2, 2023
cd05406
Add godoc
Wazzymandias Jul 2, 2023
4af9cb7
Update greedy algorithm to only drop for bundles and sbundles
Wazzymandias Jul 3, 2023
5fd4ba0
Add canRevert to algoconf to make it clearer when we discard transact…
Wazzymandias Jul 3, 2023
09c0216
Update comment
Wazzymandias Jul 4, 2023
95e4c7c
Update Sbundle
Wazzymandias Jul 4, 2023
481b801
Update sbundle to use canRevert
Wazzymandias Jul 4, 2023
dcc7507
Update logic based on PR feedback - only discard revertible transacti…
Wazzymandias Jul 4, 2023
3d31190
Update parameter names to be clearer when revertible transaction has …
Wazzymandias Jul 4, 2023
a552260
Remove unused function
Wazzymandias Jul 4, 2023
2e5a5b3
Update env var
Wazzymandias Jul 4, 2023
70f4914
Address PR feedback
Wazzymandias Jul 5, 2023
81c5a34
Update to only instantiate isRevertibleTx in conditional paths
Wazzymandias Jul 5, 2023
e823952
Update README
Wazzymandias Jul 5, 2023
40ec427
Update profit percent comment
Wazzymandias Jul 5, 2023
4ca2566
Merge remote-tracking branch 'origin/main' into bundle-tx-discard
Wazzymandias Jul 7, 2023
e82869c
Merge remote-tracking branch 'origin/main' into bundle-tx-discard
Wazzymandias Aug 2, 2023
a1b5e6a
Update log to debug, add assertion for nil receipt on commit bundle
Wazzymandias Aug 2, 2023
0c4af47
Add default value to CLI flag
Wazzymandias Aug 3, 2023
9e848ab
Update discard tx log to trace - debug is too noisy and less needed a…
Wazzymandias Aug 3, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions builder/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ type Builder struct {
builderPublicKey phase0.BLSPubKey
builderSigningDomain phase0.Domain
builderResubmitInterval time.Duration
discardRevertibleTxOnErr bool

limiter *rate.Limiter
submissionOffsetFromEndOfSlot time.Duration
Expand All @@ -91,6 +92,7 @@ type BuilderArgs struct {
relay IRelay
builderSigningDomain phase0.Domain
builderBlockResubmitInterval time.Duration
discardRevertibleTxOnErr bool
eth IEthereumService
dryRun bool
ignoreLatePayloadAttributes bool
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions builder/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}

Expand All @@ -50,6 +51,7 @@ var DefaultConfig = Config{
ValidationBlocklist: "",
BuilderRateLimitDuration: RateLimitIntervalDefault.String(),
BuilderRateLimitMaxBurst: RateLimitBurstDefault,
DiscardRevertibleTxOnErr: false,
EnableCancellations: false,
}

Expand Down
1 change: 1 addition & 0 deletions builder/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions cmd/geth/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ var (
utils.BuilderRateLimitMaxBurst,
utils.BuilderBlockResubmitInterval,
utils.BuilderSubmissionOffset,
utils.BuilderDiscardRevertibleTxOnErr,
utils.BuilderEnableCancellations,
}

Expand Down
10 changes: 10 additions & 0 deletions cmd/utils/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -845,6 +845,15 @@ var (
Category: flags.BuilderCategory,
}

BuilderDiscardRevertibleTxOnErr = &cli.BoolFlag{
Name: "builder.discard_revertible_tx_on_error",
Wazzymandias marked this conversation as resolved.
Show resolved Hide resolved
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"},
Category: flags.BuilderCategory,
}

BuilderEnableCancellations = &cli.BoolFlag{
Name: "builder.cancellations",
Usage: "Enable cancellations for the builder",
Expand Down Expand Up @@ -1668,6 +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.DiscardRevertibleTxOnErr = ctx.Bool(BuilderDiscardRevertibleTxOnErr.Name)
cfg.EnableCancellations = ctx.IsSet(BuilderEnableCancellations.Name)
}

Expand Down
92 changes: 61 additions & 31 deletions miner/algo_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,10 @@ const (
const defaultProfitPercentMinimum = 70

var (
defaultProfitThreshold = big.NewInt(defaultProfitPercentMinimum)
defaultAlgorithmConfig = algorithmConfig{
DropRevertibleTxOnErr: false,
EnforceProfit: false,
ExpectedProfit: common.Big0,
ProfitThresholdPercent: defaultProfitThreshold,
ProfitThresholdPercent: defaultProfitPercentMinimum,
}
)

Expand All @@ -56,13 +55,16 @@ 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
Wazzymandias marked this conversation as resolved.
Show resolved Hide resolved
}

type chainData struct {
Expand Down Expand Up @@ -124,7 +126,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()
Expand Down Expand Up @@ -182,17 +188,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 (
Wazzymandias marked this conversation as resolved.
Show resolved Hide resolved
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)

Expand Down Expand Up @@ -240,15 +247,23 @@ 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)

profitBefore := new(big.Int).Set(tmpEnvDiff.newProfit)
var gasUsed uint64
gasUsed uint64
)

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)
Wazzymandias marked this conversation as resolved.
Show resolved Hide resolved
)
if tmpEnvDiff.header.BaseFee != nil && tx.Type() == types.DynamicFeeTxType {
// Sanity check for extremely large numbers
if tx.GasFeeCap().BitLen() > 256 {
Expand Down Expand Up @@ -284,12 +299,18 @@ 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)
return err
}
// 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.",
Wazzymandias marked this conversation as resolved.
Show resolved Hide resolved
"tx", txHash, "err", err)
continue
}

if receipt.Status != types.ReceiptStatusSuccessful && !bundle.OriginalBundle.RevertingHash(tx.Hash()) {
log.Trace("Bundle tx failed", "bundle", bundle.OriginalBundle.Hash, "tx", tx.Hash(), "err", err)
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")
}

Expand All @@ -301,7 +322,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
Expand All @@ -319,11 +341,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 {
Expand Down Expand Up @@ -468,7 +490,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
}

Expand Down Expand Up @@ -497,13 +519,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 {
Expand All @@ -519,7 +541,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(
Wazzymandias marked this conversation as resolved.
Show resolved Hide resolved
b *types.SBundle, chData chainData, interrupt *int32, key *ecdsa.PrivateKey, algoConf algorithmConfig,
) error {
// check inclusion
minBlock := b.Inclusion.BlockNumber
maxBlock := b.Inclusion.MaxBlockNumber
Expand All @@ -538,9 +562,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
)
Expand All @@ -551,14 +573,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.Info("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
}
Expand Down
3 changes: 2 additions & 1 deletion miner/algo_common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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)
}
Expand Down
11 changes: 8 additions & 3 deletions miner/algo_greedy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Wazzymandias marked this conversation as resolved.
Show resolved Hide resolved
algoConf = &defaultAlgorithmConfig
}
return &greedyBuilder{
inputEnvironment: env,
chainData: chainData{chainConfig, chain, blacklist},
builderKey: key,
interrupt: interrupt,
algoConf: *algoConf,
}
}

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down