From 2ebef22b10e0a75cc460bdbe2b4861e387b73e44 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Mon, 7 Aug 2023 10:07:06 -0500 Subject: [PATCH] Revert some refactor changes Address PR feedback, separate block build function initializing to de-dupe logic, split types into separate definitions Add comment Add panic if copy is performed with non-empty stack of snapshots Update env changes method name Update comments Fix unit test and update log to trace Remove refund in case it causes bad state --- cmd/utils/flags.go | 1 - core/state/access_list.go | 8 +- core/state/multi_tx_snapshot.go | 21 ++-- core/state/multi_tx_snapshot_test.go | 4 + core/state/statedb.go | 27 ++--- miner/algo_common.go | 143 ++++++++++++++++++--------- miner/algo_greedy.go | 44 ++------- miner/algo_greedy_buckets.go | 34 +------ miner/env_changes.go | 10 +- miner/env_changes_test.go | 20 ++-- miner/environment_diff.go | 2 + 11 files changed, 156 insertions(+), 158 deletions(-) diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 9219617c9b..42c7b7ac1d 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -1972,7 +1972,6 @@ func setMiner(ctx *cli.Context, cfg *miner.Config) { } } - cfg.DiscardRevertibleTxOnErr = ctx.Bool(BuilderDiscardRevertibleTxOnErr.Name) cfg.EnableMultiTransactionSnapshot = ctx.Bool(BuilderEnableMultiTxSnapshot.Name) cfg.PriceCutoffPercent = ctx.Int(BuilderPriceCutoffPercentFlag.Name) } diff --git a/core/state/access_list.go b/core/state/access_list.go index 0ff5c3db6e..4194691345 100644 --- a/core/state/access_list.go +++ b/core/state/access_list.go @@ -55,13 +55,13 @@ func newAccessList() *accessList { } // Copy creates an independent copy of an accessList. -func (al *accessList) Copy() *accessList { +func (a *accessList) Copy() *accessList { cp := newAccessList() - for k, v := range al.addresses { + for k, v := range a.addresses { cp.addresses[k] = v } - cp.slots = make([]map[common.Hash]struct{}, len(al.slots)) - for i, slotMap := range al.slots { + cp.slots = make([]map[common.Hash]struct{}, len(a.slots)) + for i, slotMap := range a.slots { newSlotmap := make(map[common.Hash]struct{}, len(slotMap)) for k := range slotMap { newSlotmap[k] = struct{}{} diff --git a/core/state/multi_tx_snapshot.go b/core/state/multi_tx_snapshot.go index 0900ecc33c..022cb76220 100644 --- a/core/state/multi_tx_snapshot.go +++ b/core/state/multi_tx_snapshot.go @@ -29,17 +29,16 @@ type MultiTxSnapshot struct { accountNotPending map[common.Address]struct{} accountNotDirty map[common.Address]struct{} - previousRefund uint64 // TODO: snapdestructs, snapaccount storage } // NewMultiTxSnapshot creates a new MultiTxSnapshot -func NewMultiTxSnapshot(previousRefund uint64) *MultiTxSnapshot { - multiTxSnapshot := newMultiTxSnapshot(previousRefund) +func NewMultiTxSnapshot() *MultiTxSnapshot { + multiTxSnapshot := newMultiTxSnapshot() return &multiTxSnapshot } -func newMultiTxSnapshot(previousRefund uint64) MultiTxSnapshot { +func newMultiTxSnapshot() MultiTxSnapshot { return MultiTxSnapshot{ numLogsAdded: make(map[common.Hash]int), prevObjects: make(map[common.Address]*stateObject), @@ -52,7 +51,6 @@ func newMultiTxSnapshot(previousRefund uint64) MultiTxSnapshot { accountDeleted: make(map[common.Address]bool), accountNotPending: make(map[common.Address]struct{}), accountNotDirty: make(map[common.Address]struct{}), - previousRefund: previousRefund, } } @@ -135,7 +133,8 @@ func (s *MultiTxSnapshot) updateFromJournal(journal *journal) { } } -// objectChanged returns whether the object was changed (in the set of prevObjects). +// objectChanged returns whether the object was changed (in the set of prevObjects), which can happen +// because of self-destructs and deployments. func (s *MultiTxSnapshot) objectChanged(address common.Address) bool { _, ok := s.prevObjects[address] return ok @@ -364,11 +363,6 @@ func (s *MultiTxSnapshot) Merge(other *MultiTxSnapshot) error { // revertState reverts the state to the snapshot. func (s *MultiTxSnapshot) revertState(st *StateDB) { - // restore previous refund - if st.refund != s.previousRefund { - st.refund = s.previousRefund - } - // remove all the logs added for txhash, numLogs := range s.numLogsAdded { lens := len(st.logs[txhash]) @@ -463,7 +457,7 @@ func (stack *MultiTxSnapshotStack) NewSnapshot() (*MultiTxSnapshot, error) { return nil, errors.New("failed to create new multi-transaction snapshot - invalid snapshot found at head") } - snap := newMultiTxSnapshot(stack.state.refund) + snap := newMultiTxSnapshot() stack.snapshots = append(stack.snapshots, snap) return &snap, nil } @@ -549,8 +543,6 @@ func (stack *MultiTxSnapshotStack) Size() int { // Invalidate invalidates the latest snapshot. This is used when state changes are committed to trie. func (stack *MultiTxSnapshotStack) Invalidate() { - // TODO: if latest snapshot is invalid, then all previous snapshots - // would also be invalidated, need to update logic to reflect that size := len(stack.snapshots) if size == 0 { return @@ -560,7 +552,6 @@ func (stack *MultiTxSnapshotStack) Invalidate() { head.invalid = true stack.snapshots = stack.snapshots[:0] stack.snapshots = append(stack.snapshots, head) - //stack.snapshots[size-1].invalid = true } // UpdatePendingStatus updates the pending status for an address. diff --git a/core/state/multi_tx_snapshot_test.go b/core/state/multi_tx_snapshot_test.go index 607b458924..2848f3145e 100644 --- a/core/state/multi_tx_snapshot_test.go +++ b/core/state/multi_tx_snapshot_test.go @@ -577,6 +577,10 @@ func TestStackAgainstSingleSnap(t *testing.T) { // we generate a random seed ten times to fuzz test multiple stack snapshots against single layer snapshot for i := 0; i < 10; i++ { testMultiTxSnapshot(t, func(s *StateDB) { + // Need to drop initial snapshot since copy requires empty snapshot stack + if err := s.MultiTxSnapshotRevert(); err != nil { + t.Fatalf("error reverting snapshot: %v", err) + } original := s.Copy() baselineStateDB := s.Copy() diff --git a/core/state/statedb.go b/core/state/statedb.go index a062bd3793..040f0f6ede 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -718,6 +718,11 @@ func (s *StateDB) Copy() *StateDB { hasher: crypto.NewKeccakState(), } // Initialize new multi-transaction snapshot stack for the copied state + // NOTE(wazzymandias): We avoid copying the snapshot stack from the original state + // because it may contain snapshots that are not valid for the copied state. + if s.multiTxSnapshotStack.Size() > 0 { + panic("cannot copy state with active multi-transaction snapshot stack") + } state.multiTxSnapshotStack = NewMultiTxSnapshotStack(state) // Copy the dirty states, logs, and preimages for addr := range s.journal.dirties { @@ -865,7 +870,6 @@ func (s *StateDB) Finalise(deleteEmptyObjects bool) { } if obj.suicided || (deleteEmptyObjects && obj.empty()) { s.multiTxSnapshotStack.UpdateObjectDeleted(obj.address, obj.deleted) - //s.multiTxSnapshotStack.UpdateObjectDeleted(obj.address, obj.deleted) obj.deleted = true @@ -1204,20 +1208,17 @@ func (s *StateDB) convertAccountSet(set map[common.Address]struct{}) map[common. return ret } -func (s *StateDB) NewMultiTxSnapshot() error { - _, err := s.multiTxSnapshotStack.NewSnapshot() - if err != nil { - return err - } - return nil +func (s *StateDB) NewMultiTxSnapshot() (err error) { + _, err = s.multiTxSnapshotStack.NewSnapshot() + return } -func (s *StateDB) MultiTxSnapshotRevert() error { - _, err := s.multiTxSnapshotStack.Revert() - return err +func (s *StateDB) MultiTxSnapshotRevert() (err error) { + _, err = s.multiTxSnapshotStack.Revert() + return } -func (s *StateDB) MultiTxSnapshotCommit() error { - _, err := s.multiTxSnapshotStack.Commit() - return err +func (s *StateDB) MultiTxSnapshotCommit() (err error) { + _, err = s.multiTxSnapshotStack.Commit() + return } diff --git a/miner/algo_common.go b/miner/algo_common.go index db4dbcbc51..5c7ce0f8d2 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -35,7 +35,7 @@ var ( defaultAlgorithmConfig = algorithmConfig{ DropRevertibleTxOnErr: false, EnforceProfit: false, - ExpectedProfit: common.Big0, + ExpectedProfit: nil, ProfitThresholdPercent: defaultProfitThresholdPercent, PriceCutoffPercent: defaultPriceCutoffPercent, EnableMultiTxSnap: false, @@ -66,35 +66,50 @@ 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 int // 0-100, e.g. 70 means 70% - // PriceCutoffPercent is the minimum effective gas price threshold used for bucketing transactions by price. - // For example if the top transaction in a list has an effective gas price of 1000 wei and PriceCutoffPercent - // is 10 (i.e. 10%), then the minimum effective gas price included in the same bucket as the top transaction - // is (1000 * 10%) = 100 wei. - PriceCutoffPercent int - // EnableMultiTxSnap is true if we want to use multi-transaction snapshot for committing transactions, - // which reduce state copies when reverting failed bundles (note: experimental) - EnableMultiTxSnap bool - } +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 int // 0-100, e.g. 70 means 70% + // PriceCutoffPercent is the minimum effective gas price threshold used for bucketing transactions by price. + // For example if the top transaction in a list has an effective gas price of 1000 wei and PriceCutoffPercent + // is 10 (i.e. 10%), then the minimum effective gas price included in the same bucket as the top transaction + // is (1000 * 10%) = 100 wei. + PriceCutoffPercent int + // EnableMultiTxSnap is true if we want to use multi-transaction snapshot for committing transactions, + // which reduce state copies when reverting failed bundles (note: experimental) + EnableMultiTxSnap bool +} - chainData struct { - chainConfig *params.ChainConfig - chain *core.BlockChain - blacklist map[common.Address]struct{} - } +type chainData struct { + chainConfig *params.ChainConfig + chain *core.BlockChain + blacklist map[common.Address]struct{} +} +// PayoutTransactionParams holds parameters for committing a payout transaction, used in commitPayoutTx +type PayoutTransactionParams struct { + Amount *big.Int + BaseFee *big.Int + ChainData chainData + Gas uint64 + CommitFn CommitTxFunc + Receiver common.Address + Sender common.Address + SenderBalance *big.Int + SenderNonce uint64 + Signer types.Signer + PrivateKey *ecdsa.PrivateKey +} + +type ( // BuildBlockFunc is the function signature for building a block BuildBlockFunc func( simBundles []types.SimulatedBundle, @@ -103,22 +118,57 @@ type ( // CommitTxFunc is the function signature for committing a transaction CommitTxFunc func(*types.Transaction, chainData) (*types.Receipt, int, error) +) - // PayoutTransactionParams holds parameters for committing a payout transaction, used in commitPayoutTx - PayoutTransactionParams struct { - Amount *big.Int - BaseFee *big.Int - ChainData chainData - Gas uint64 - CommitFn CommitTxFunc - Receiver common.Address - Sender common.Address - SenderBalance *big.Int - SenderNonce uint64 - Signer types.Signer - PrivateKey *ecdsa.PrivateKey +func NewBuildBlockFunc( + inputEnvironment *environment, + builderKey *ecdsa.PrivateKey, + chData chainData, + algoConf algorithmConfig, + greedyBuckets *greedyBucketsBuilder, + greedy *greedyBuilder, +) BuildBlockFunc { + if algoConf.EnableMultiTxSnap { + return func(simBundles []types.SimulatedBundle, simSBundles []*types.SimSBundle, transactions map[common.Address]types.Transactions) (*environment, []types.SimulatedBundle, []types.UsedSBundle) { + orders := types.NewTransactionsByPriceAndNonce(inputEnvironment.signer, transactions, + simBundles, simSBundles, inputEnvironment.header.BaseFee) + + usedBundles, usedSbundles, err := BuildMultiTxSnapBlock( + inputEnvironment, + builderKey, + chData, + algoConf, + orders, + ) + if err != nil { + log.Trace("Error(s) building multi-tx snapshot block", "err", err) + } + return inputEnvironment, usedBundles, usedSbundles + } + } else if builder := greedyBuckets; builder != nil { + return func(simBundles []types.SimulatedBundle, simSBundles []*types.SimSBundle, transactions map[common.Address]types.Transactions) (*environment, []types.SimulatedBundle, []types.UsedSBundle) { + orders := types.NewTransactionsByPriceAndNonce(inputEnvironment.signer, transactions, + simBundles, simSBundles, inputEnvironment.header.BaseFee) + + envDiff := newEnvironmentDiff(inputEnvironment.copy()) + usedBundles, usedSbundles := builder.mergeOrdersIntoEnvDiff(envDiff, orders) + envDiff.applyToBaseEnv() + return envDiff.baseEnvironment, usedBundles, usedSbundles + } + } else if builder := greedy; builder != nil { + return func(simBundles []types.SimulatedBundle, simSBundles []*types.SimSBundle, transactions map[common.Address]types.Transactions) (*environment, []types.SimulatedBundle, []types.UsedSBundle) { + orders := types.NewTransactionsByPriceAndNonce(inputEnvironment.signer, transactions, + simBundles, simSBundles, inputEnvironment.header.BaseFee) + + envDiff := newEnvironmentDiff(inputEnvironment.copy()) + usedBundles, usedSbundles := builder.mergeOrdersIntoEnvDiff(envDiff, orders) + envDiff.applyToBaseEnv() + return envDiff.baseEnvironment, usedBundles, usedSbundles + } + } else { + panic("invalid call to build block function") } -) +} func checkInterrupt(i *int32) bool { return i != nil && atomic.LoadInt32(i) != commitInterruptNone @@ -307,6 +357,9 @@ func BuildMultiTxSnapBlock( chData chainData, algoConf algorithmConfig, orders *types.TransactionsByPriceAndNonce) ([]types.SimulatedBundle, []types.UsedSBundle, error) { + // NOTE(wazzymandias): BuildMultiTxSnapBlock uses envChanges which is different from envDiff struct. + // Eventually the structs should be consolidated but for now they represent the difference between using state + // copies for building blocks (envDiff) versus using MultiTxSnapshot (envChanges). var ( usedBundles []types.SimulatedBundle usedSbundles []types.UsedSBundle @@ -371,9 +424,9 @@ func BuildMultiTxSnapBlock( } if orderFailed { - if err = changes.revert(); err != nil { - log.Error("Failed to revert changes with multi-transaction snapshot", "err", err) - buildBlockErrors = append(buildBlockErrors, fmt.Errorf("failed to revert changes: %w", err)) + if err = changes.discard(); err != nil { + log.Error("Failed to discard changes with multi-transaction snapshot", "err", err) + buildBlockErrors = append(buildBlockErrors, fmt.Errorf("failed to discard changes: %w", err)) } } else { if err = changes.apply(); err != nil { diff --git a/miner/algo_greedy.go b/miner/algo_greedy.go index 7650563ff1..4a712f03c3 100644 --- a/miner/algo_greedy.go +++ b/miner/algo_greedy.go @@ -40,37 +40,15 @@ func newGreedyBuilder( algoConf: *algoConf, } // Initialize block builder function - var buildBlockFunc BuildBlockFunc - if algoConf.EnableMultiTxSnap { - buildBlockFunc = func(simBundles []types.SimulatedBundle, simSBundles []*types.SimSBundle, transactions map[common.Address]types.Transactions) (*environment, []types.SimulatedBundle, []types.UsedSBundle) { - orders := types.NewTransactionsByPriceAndNonce(builder.inputEnvironment.signer, transactions, - simBundles, simSBundles, builder.inputEnvironment.header.BaseFee) - - usedBundles, usedSbundles, err := BuildMultiTxSnapBlock( - builder.inputEnvironment, - builder.builderKey, - builder.chainData, - builder.algoConf, - orders, - ) - if err != nil { - log.Debug("Error(s) building multi-tx snapshot block", "err", err) - } - return builder.inputEnvironment, usedBundles, usedSbundles - } - } else { - buildBlockFunc = func(simBundles []types.SimulatedBundle, simSBundles []*types.SimSBundle, transactions map[common.Address]types.Transactions) (*environment, []types.SimulatedBundle, []types.UsedSBundle) { - orders := types.NewTransactionsByPriceAndNonce(builder.inputEnvironment.signer, transactions, - simBundles, simSBundles, builder.inputEnvironment.header.BaseFee) - - envDiff := newEnvironmentDiff(builder.inputEnvironment.copy()) - usedBundles, usedSbundles := builder.mergeOrdersIntoEnvDiff(envDiff, orders) - envDiff.applyToBaseEnv() - return envDiff.baseEnvironment, usedBundles, usedSbundles - } - } + builder.buildBlockFunc = NewBuildBlockFunc( + builder.inputEnvironment, + builder.builderKey, + builder.chainData, + builder.algoConf, + nil, + builder, + ) - builder.buildBlockFunc = buildBlockFunc return builder, nil } @@ -137,9 +115,5 @@ func (b *greedyBuilder) mergeOrdersIntoEnvDiff( } func (b *greedyBuilder) buildBlock(simBundles []types.SimulatedBundle, simSBundles []*types.SimSBundle, transactions map[common.Address]types.Transactions) (*environment, []types.SimulatedBundle, []types.UsedSBundle) { - orders := types.NewTransactionsByPriceAndNonce(b.inputEnvironment.signer, transactions, simBundles, simSBundles, b.inputEnvironment.header.BaseFee) - envDiff := newEnvironmentDiff(b.inputEnvironment.copy()) - usedBundles, usedSbundles := b.mergeOrdersIntoEnvDiff(envDiff, orders) - envDiff.applyToBaseEnv() - return envDiff.baseEnvironment, usedBundles, usedSbundles + return b.buildBlockFunc(simBundles, simSBundles, transactions) } diff --git a/miner/algo_greedy_buckets.go b/miner/algo_greedy_buckets.go index 3bec22d3b3..63fad2989a 100644 --- a/miner/algo_greedy_buckets.go +++ b/miner/algo_greedy_buckets.go @@ -46,39 +46,7 @@ func newGreedyBucketsBuilder( } // Initialize block builder function - var buildBlockFunc BuildBlockFunc - if algoConf.EnableMultiTxSnap { - buildBlockFunc = func(simBundles []types.SimulatedBundle, simSBundles []*types.SimSBundle, - transactions map[common.Address]types.Transactions) (*environment, []types.SimulatedBundle, []types.UsedSBundle) { - orders := types.NewTransactionsByPriceAndNonce(builder.inputEnvironment.signer, transactions, - simBundles, simSBundles, builder.inputEnvironment.header.BaseFee) - - usedBundles, usedSbundles, err := BuildMultiTxSnapBlock( - builder.inputEnvironment, - builder.builderKey, - builder.chainData, - builder.algoConf, - orders, - ) - if err != nil { - log.Trace("Error(s) building multi-tx snapshot block", "err", err) - } - return builder.inputEnvironment, usedBundles, usedSbundles - } - } else { - buildBlockFunc = func(simBundles []types.SimulatedBundle, simSBundles []*types.SimSBundle, - transactions map[common.Address]types.Transactions) (*environment, []types.SimulatedBundle, []types.UsedSBundle) { - orders := types.NewTransactionsByPriceAndNonce(builder.inputEnvironment.signer, transactions, - simBundles, simSBundles, builder.inputEnvironment.header.BaseFee) - - envDiff := newEnvironmentDiff(builder.inputEnvironment.copy()) - usedBundles, usedSbundles := builder.mergeOrdersIntoEnvDiff(envDiff, orders) - envDiff.applyToBaseEnv() - return envDiff.baseEnvironment, usedBundles, usedSbundles - } - } - - builder.buildBlockFunc = buildBlockFunc + builder.buildBlockFunc = NewBuildBlockFunc(builder.inputEnvironment, builder.builderKey, builder.chainData, builder.algoConf, builder, nil) return builder, nil } diff --git a/miner/env_changes.go b/miner/env_changes.go index 6fc7cdc747..9e72caf1bc 100644 --- a/miner/env_changes.go +++ b/miner/env_changes.go @@ -13,7 +13,7 @@ import ( "github.com/ethereum/go-ethereum/log" ) -// envChanges is a helper struct to apply and revert changes to the environment +// envChanges is a helper struct to apply and discard changes to the environment type envChanges struct { env *environment gasPool *core.GasPool @@ -219,6 +219,8 @@ func (c *envChanges) commitBundle(bundle *types.SimulatedBundle, chData chainDat } func (c *envChanges) CommitSBundle(sbundle *types.SimSBundle, chData chainData, key *ecdsa.PrivateKey, algoConf algorithmConfig) error { + // TODO: Suggestion for future improvement: instead of checking if key is nil, panic. + // Discussed with @Ruteri, see PR#90 for details: https://github.com/flashbots/builder/pull/90#discussion_r1285567550 if key == nil { return errNoPrivateKey } @@ -392,11 +394,13 @@ func (c *envChanges) commitSBundle(sbundle *types.SBundle, chData chainData, key return nil } -// revert reverts all changes to the environment - every commit operation must be followed by a revert or apply operation -func (c *envChanges) revert() error { +// discard reverts all changes to the environment - every commit operation must be followed by a discard or apply operation +func (c *envChanges) discard() error { return c.env.state.MultiTxSnapshotRevert() } +// rollback reverts all changes to the environment - whereas apply and discard update the state, rollback only updates the environment +// the intended use is to call rollback after a commit operation has failed func (c *envChanges) rollback( gasUsedBefore uint64, gasPoolBefore *core.GasPool, profitBefore *big.Int, txsBefore []*types.Transaction, receiptsBefore []*types.Receipt) { diff --git a/miner/env_changes_test.go b/miner/env_changes_test.go index f1225308bf..bac353bd67 100644 --- a/miner/env_changes_test.go +++ b/miner/env_changes_test.go @@ -61,10 +61,6 @@ func TestBundleCommitSnaps(t *testing.T) { statedb, chData, signers := genTestSetup() env := newEnvironment(chData, statedb, signers.addresses[0], GasLimit, big.NewInt(1)) - changes, err := newEnvChanges(env) - if err != nil { - t.Fatal("can't create env changes", err) - } 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{}) @@ -79,6 +75,11 @@ func TestBundleCommitSnaps(t *testing.T) { t.Fatal("Failed to simulate bundle", err) } + changes, err := newEnvChanges(env) + if err != nil { + t.Fatal("can't create env changes", err) + } + err = changes.commitBundle(&simBundle, chData) if err != nil { t.Fatal("Failed to commit bundle", err) @@ -167,10 +168,6 @@ func TestErrorBundleCommitSnaps(t *testing.T) { statedb, chData, signers := genTestSetup() env := newEnvironment(chData, statedb, signers.addresses[0], 21000*2, big.NewInt(1)) - changes, err := newEnvChanges(env) - if err != nil { - t.Fatal("can't create env changes", err) - } // This tx will be included before bundle so bundle will fail because of gas limit tx0 := signers.signTx(4, 21000, big.NewInt(0), big.NewInt(1), signers.addresses[2], big.NewInt(0), []byte{}) @@ -188,6 +185,11 @@ func TestErrorBundleCommitSnaps(t *testing.T) { t.Fatal("Failed to simulate bundle", err) } + changes, err := newEnvChanges(env) + if err != nil { + t.Fatal("can't create env changes", err) + } + _, _, err = changes.commitTx(tx0, chData) if err != nil { t.Fatal("Failed to commit tx0", err) @@ -348,7 +350,7 @@ func TestBlacklistSnaps(t *testing.T) { t.Fatal("committed blacklisted transaction: trace") } - err = changes.revert() + err = changes.discard() if err != nil { t.Fatal("failed reverting changes", err) } diff --git a/miner/environment_diff.go b/miner/environment_diff.go index 73c1c78d69..f5cb4765d1 100644 --- a/miner/environment_diff.go +++ b/miner/environment_diff.go @@ -237,6 +237,8 @@ func (envDiff *environmentDiff) commitPayoutTx(amount *big.Int, sender, receiver } func (envDiff *environmentDiff) commitSBundle(b *types.SimSBundle, chData chainData, interrupt *int32, key *ecdsa.PrivateKey, algoConf algorithmConfig) error { + // TODO: Suggestion for future improvement: instead of checking if key is nil, panic. + // Discussed with @Ruteri, see PR#90 for details: https://github.com/flashbots/builder/pull/90#discussion_r1285567550 if key == nil { return errNoPrivateKey }