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

@@ -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)
 }

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{}{}

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.

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()
 

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
 }

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 {