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

@@ -260,11 +260,6 @@ func prepareInitialState(s *StateDB) {
 	}
 
 	s.Finalise(true)
-
-	// NOTE(wazzymandias):
-	//   We want to test refund is properly reverted for snapshots - state.StateDB clears refund on Finalise
-	//   so refund is set here to emulate state with non-zero value.
-	s.AddRefund(rng.Uint64())
 }
 
 func testMultiTxSnapshot(t *testing.T, actions func(s *StateDB)) {
@@ -359,7 +354,6 @@ func TestMultiTxSnapshotRefund(t *testing.T) {
 			s.SetCode(addr, []byte{0x80})
 		}
 		s.Finalise(true)
-		s.AddRefund(1000)
 	})
 }
 
@@ -499,40 +493,6 @@ func TestStackBasic(t *testing.T) {
 	}
 }
 
-func TestStackRefund(t *testing.T) {
-	testMultiTxSnapshot(t, func(s *StateDB) {
-		const counter = 10
-
-		s.AddRefund(500)
-		previousRefunds := make([]uint64, 0, counter)
-		previousRefunds = append(previousRefunds, s.GetRefund())
-
-		for i := 0; i < counter; i++ {
-			previousRefunds = append(previousRefunds, s.GetRefund())
-			if err := s.NewMultiTxSnapshot(); err != nil {
-				t.Errorf("NewMultiTxSnapshot failed: %v", err)
-				t.FailNow()
-			}
-			s.Finalise(true)
-			s.AddRefund(1000)
-		}
-
-		for i := 0; i < counter; i++ {
-			if err := s.MultiTxSnapshotRevert(); err != nil {
-				t.Errorf("MultiTxSnapshotRevert failed: %v", err)
-				t.FailNow()
-			}
-			actualRefund := s.GetRefund()
-			expectedRefund := previousRefunds[len(previousRefunds)-1]
-			if actualRefund != expectedRefund {
-				t.Errorf("expected refund to be %d, got %d", expectedRefund, actualRefund)
-				t.FailNow()
-			}
-			previousRefunds = previousRefunds[:len(previousRefunds)-1]
-		}
-	})
-}
-
 func TestStackSelfDestruct(t *testing.T) {
 	testMultiTxSnapshot(t, func(s *StateDB) {
 		if err := s.NewMultiTxSnapshot(); err != nil {
@@ -577,6 +537,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
 }