core/state: simplify journal api, remove externally visible snapshot ids

Author: holiman

cmd/evm/internal/t8ntool/execution.go

@@ -254,9 +254,9 @@ func (pre *Prestate) Apply(vmConfig vm.Config, chainConfig *params.ChainConfig,
 
 		var (
 			txContext = core.NewEVMTxContext(msg)
-			snapshot  = statedb.Snapshot()
 			prevGas   = gaspool.Gas()
 		)
+		statedb.Snapshot()
 		if tracer != nil && tracer.OnTxStart != nil {
 			tracer.OnTxStart(evm.GetVMContext(), tx, msg.From)
 		}
@@ -265,7 +265,7 @@ func (pre *Prestate) Apply(vmConfig vm.Config, chainConfig *params.ChainConfig,
 		evm.SetTxContext(txContext)
 		msgResult, err := core.ApplyMessage(evm, msg, gaspool)
 		if err != nil {
-			statedb.RevertToSnapshot(snapshot)
+			statedb.RevertSnapshot()
 			log.Info("rejected tx", "index", i, "hash", tx.Hash(), "from", msg.From, "error", err)
 			rejectedTxs = append(rejectedTxs, &rejectedTx{i, err.Error()})
 			gaspool.SetGas(prevGas)
@@ -279,7 +279,7 @@ func (pre *Prestate) Apply(vmConfig vm.Config, chainConfig *params.ChainConfig,
 			}
 			continue
 		}
-		statedb.DiscardSnapshot(snapshot)
+		statedb.DiscardSnapshot()
 		includedTxs = append(includedTxs, tx)
 		if hashError != nil {
 			return nil, nil, nil, NewError(ErrorMissingBlockhash, hashError)

cmd/evm/runner.go

@@ -176,8 +176,8 @@ func runCmd(ctx *cli.Context) error {
 	sdb := state.NewDatabase(triedb, nil)
 	statedb, _ = state.New(genesis.Root(), sdb)
 	chainConfig = genesisConfig.Config
-	id := statedb.Snapshot()
-	defer statedb.DiscardSnapshot(id)
+	statedb.Snapshot()
+	defer statedb.DiscardSnapshot()
 	if ctx.String(SenderFlag.Name) != "" {
 		sender = common.HexToAddress(ctx.String(SenderFlag.Name))
 	}

core/state/journal.go

@@ -22,23 +22,23 @@ import (
 )
 
 type journal interface {
-	// snapshot returns an identifier for the current revision of the state.
+	// snapshot starts a new journal scope which can be reverted or discarded.
 	// The lifeycle of journalling is as follows:
 	// - snapshot() starts a 'scope'.
 	// - The method snapshot() may be called any number of times.
 	// - For each call to snapshot, there should be a corresponding call to end
 	//  the scope via either of:
 	//   - revertToSnapshot, which undoes the changes in the scope, or
 	//   - discardSnapshot, which discards the ability to revert the changes in the scope.
-	snapshot() int
+	snapshot()
 
-	// revertToSnapshot reverts all state changes made since the given revision.
-	revertToSnapshot(revid int, s *StateDB)
+	// revertSnapshot reverts all state changes made since the last call to snapshot().
+	revertSnapshot(s *StateDB)
 
-	// discardSnapshot removes the snapshot with the given id; after calling this
+	// discardSnapshot removes the latest snapshot; after calling this
 	// method, it is no longer possible to revert to that particular snapshot, the
 	// changes are considered part of the parent scope.
-	discardSnapshot(revid int)
+	discardSnapshot()
 
 	// reset clears the journal so it can be reused.
 	reset()

core/state/journal_linear.go

@@ -17,21 +17,14 @@
 package state
 
 import (
-	"fmt"
 	"maps"
 	"slices"
-	"sort"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/holiman/uint256"
 )
 
-type revision struct {
-	id           int
-	journalIndex int
-}
-
 // journalEntry is a modification entry in the state change linear journal that can be
 // reverted on demand.
 type journalEntry interface {
@@ -52,8 +45,7 @@ type linearJournal struct {
 	entries []journalEntry         // Current changes tracked by the linearJournal
 	dirties map[common.Address]int // Dirty accounts and the number of changes
 
-	validRevisions []revision
-	nextRevisionId int
+	revisions []int // sequence of indexes to points in time designating snapshots
 }
 
 // compile-time interface check
@@ -71,9 +63,8 @@ func newLinearJournal() *linearJournal {
 // can be reused.
 func (j *linearJournal) reset() {
 	j.entries = j.entries[:0]
-	j.validRevisions = j.validRevisions[:0]
+	j.revisions = j.revisions[:0]
 	clear(j.dirties)
-	j.nextRevisionId = 0
 }
 
 func (j linearJournal) dirtyAccounts() []common.Address {
@@ -85,33 +76,33 @@ func (j linearJournal) dirtyAccounts() []common.Address {
 	return dirty
 }
 
-// snapshot returns an identifier for the current revision of the state.
-func (j *linearJournal) snapshot() int {
-	id := j.nextRevisionId
-	j.nextRevisionId++
-	j.validRevisions = append(j.validRevisions, revision{id, j.length()})
-	return id
+// snapshot starts a new journal scope which can be reverted or discarded.
+func (j *linearJournal) snapshot() {
+	j.revisions = append(j.revisions, len(j.entries))
 }
 
-func (j *linearJournal) revertToSnapshot(revid int, s *StateDB) {
-	// Find the snapshot in the stack of valid snapshots.
-	idx := sort.Search(len(j.validRevisions), func(i int) bool {
-		return j.validRevisions[i].id >= revid
-	})
-	if idx == len(j.validRevisions) || j.validRevisions[idx].id != revid {
-		panic(fmt.Errorf("revision id %v cannot be reverted (valid revisions: %d)", revid, len(j.validRevisions)))
-	}
-	snapshot := j.validRevisions[idx].journalIndex
-
+// revertSnapshot reverts all state changes made since the last call to snapshot().
+func (j *linearJournal) revertSnapshot(s *StateDB) {
+	id := len(j.revisions) - 1
+	revision := j.revisions[id]
 	// Replay the linearJournal to undo changes and remove invalidated snapshots
-	j.revert(s, snapshot)
-	j.validRevisions = j.validRevisions[:idx]
+	j.revertTo(s, revision)
+	j.revisions = j.revisions[:id]
 }
 
-// discardSnapshot removes the snapshot with the given id; after calling this
+// discardSnapshot removes the latest snapshot; after calling this
 // method, it is no longer possible to revert to that particular snapshot, the
 // changes are considered part of the parent scope.
-func (j *linearJournal) discardSnapshot(id int) {
+func (j *linearJournal) discardSnapshot() {
+	id := len(j.revisions) - 1
+	if id == 0 {
+		// If a transaction is applied successfully, the statedb.Finalize will
+		// end by clearing and resetting the journal. Invoking a discardSnapshot
+		// afterwards will land here: calling discard on an empty journal.
+		// This is fine
+		return
+	}
+	j.revisions = j.revisions[:id]
 }
 
 // append inserts a new modification entry to the end of the change linearJournal.
@@ -124,7 +115,7 @@ func (j *linearJournal) append(entry journalEntry) {
 
 // revert undoes a batch of journalled modifications along with any reverted
 // dirty handling too.
-func (j *linearJournal) revert(statedb *StateDB, snapshot int) {
+func (j *linearJournal) revertTo(statedb *StateDB, snapshot int) {
 	for i := len(j.entries) - 1; i >= snapshot; i-- {
 		// Undo the changes made by the operation
 		j.entries[i].revert(statedb)
@@ -158,10 +149,9 @@ func (j *linearJournal) copy() journal {
 		entries = append(entries, j.entries[i].copy())
 	}
 	return &linearJournal{
-		entries:        entries,
-		dirties:        maps.Clone(j.dirties),
-		validRevisions: slices.Clone(j.validRevisions),
-		nextRevisionId: j.nextRevisionId,
+		entries:   entries,
+		dirties:   maps.Clone(j.dirties),
+		revisions: slices.Clone(j.revisions),
 	}
 }
 

core/state/journal_set.go

@@ -18,13 +18,11 @@ package state
 
 import (
 	"bytes"
-	"fmt"
 	"maps"
 	"slices"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core/types"
-	"github.com/ethereum/go-ethereum/log"
 	"github.com/holiman/uint256"
 )
 
@@ -349,51 +347,34 @@ func (j *sparseJournal) copy() journal {
 	return cp
 }
 
-// snapshot returns an identifier for the current revision of the state.
+// snapshot starts a new journal scope which can be reverted or discarded.
 // OBS: A call to Snapshot is _required_ in order to initialize the journalling,
 // invoking the journal-methods without having invoked Snapshot will lead to
 // panic.
-func (j *sparseJournal) snapshot() int {
-	id := len(j.entries)
+func (j *sparseJournal) snapshot() {
 	j.entries = append(j.entries, newScopedJournal())
-	return id
 }
 
-// revertToSnapshot reverts all state changes made since the given revision.
-func (j *sparseJournal) revertToSnapshot(id int, s *StateDB) {
-	if id >= len(j.entries) {
-		panic(fmt.Errorf("revision id %v cannot be reverted", id))
-	}
-	// Revert the entries sequentially
-	for i := len(j.entries) - 1; i >= id; i-- {
-		entry := j.entries[i]
-		entry.revert(s)
-	}
+// revertSnapshot reverts all state changes made since the last call to snapshot().
+func (j *sparseJournal) revertSnapshot(s *StateDB) {
+	id := len(j.entries) - 1
+	j.entries[id].revert(s)
 	j.entries = j.entries[:id]
 }
 
-// discardSnapshot removes the snapshot with the given id; after calling this
+// discardSnapshot removes the latest snapshot; after calling this
 // method, it is no longer possible to revert to that particular snapshot, the
 // changes are considered part of the parent scope.
-func (j *sparseJournal) discardSnapshot(id int) {
+func (j *sparseJournal) discardSnapshot() {
+	id := len(j.entries) - 1
+	// here we must merge the 'id' with it's parent.
 	if id == 0 {
+		// If a transaction is applied successfully, the statedb.Finalize will
+		// end by clearing and resetting the journal. Invoking a discardSnapshot
+		// afterwards will land here: calling discard on an empty journal.
+		// This is fine
 		return
 	}
-	// here we must merge the 'id' with it's parent.
-	want := len(j.entries) - 1
-	have := id
-	if want != have {
-		if want == 0 && id == 1 {
-			// If a transcation is applied successfully, the statedb.Finalize will
-			// end by clearing and resetting the journal. Invoking a discardSnapshot
-			// afterwards will lead us here.
-			// Let's not panic, but it's ok to complain a bit
-			log.Error("Extraneous invocation to discard snapshot")
-			return
-		} else {
-			panic(fmt.Sprintf("journalling error, want discard(%d), have discard(%d)", want, have))
-		}
-	}
 	entry := j.entries[id]
 	parent := j.entries[id-1]
 	entry.merge(parent)

core/state/journal_test.go

@@ -75,23 +75,23 @@ func testJournalAccessList(t *testing.T, j journal) {
 	statedb.accessList = newAccessList()
 	statedb.journal = j
 
+	j.snapshot()
 	{
 		// If the journal performs the rollback in the wrong order, this
 		// will cause a panic.
-		id := j.snapshot()
 		statedb.AddSlotToAccessList(common.Address{0x1}, common.Hash{0x4})
 		statedb.AddSlotToAccessList(common.Address{0x3}, common.Hash{0x4})
-		statedb.RevertToSnapshot(id)
 	}
+	statedb.RevertSnapshot()
+	j.snapshot()
 	{
-		id := j.snapshot()
 		statedb.AddAddressToAccessList(common.Address{0x2})
 		statedb.AddAddressToAccessList(common.Address{0x3})
 		statedb.AddAddressToAccessList(common.Address{0x4})
-		statedb.RevertToSnapshot(id)
-		if statedb.accessList.ContainsAddress(common.Address{0x2}) {
-			t.Fatal("should be missing")
-		}
+	}
+	statedb.RevertSnapshot()
+	if statedb.accessList.ContainsAddress(common.Address{0x2}) {
+		t.Fatal("should be missing")
 	}
 }
 
@@ -107,25 +107,27 @@ func testJournalRefunds(t *testing.T, j journal) {
 	var statedb = &StateDB{}
 	statedb.accessList = newAccessList()
 	statedb.journal = j
-	zero := j.snapshot()
-	j.refundChange(0)
-	j.refundChange(1)
+	j.snapshot()
 	{
-		id := j.snapshot()
-		j.refundChange(2)
-		j.refundChange(3)
-		j.revertToSnapshot(id, statedb)
+		j.refundChange(0)
+		j.refundChange(1)
+		j.snapshot()
+		{
+			j.refundChange(2)
+			j.refundChange(3)
+		}
+		j.revertSnapshot(statedb)
 		if have, want := statedb.refund, uint64(2); have != want {
 			t.Fatalf("have %d want %d", have, want)
 		}
+		j.snapshot()
+		{
+			j.refundChange(2)
+			j.refundChange(3)
+		}
+		j.discardSnapshot()
 	}
-	{
-		id := j.snapshot()
-		j.refundChange(2)
-		j.refundChange(3)
-		j.discardSnapshot(id)
-	}
-	j.revertToSnapshot(zero, statedb)
+	j.revertSnapshot(statedb)
 	if have, want := statedb.refund, uint64(0); have != want {
 		t.Fatalf("have %d want %d", have, want)
 	}