fix: ordering of disk then memory state changes on execution

Author: ARR4N

blocks/db.go

@@ -4,14 +4,15 @@ import (
 	"encoding/binary"
 	"fmt"
 	"math/big"
-	"time"
 
 	"github.com/ava-labs/libevm/common"
 	"github.com/ava-labs/libevm/core/types"
 	"github.com/ava-labs/libevm/ethdb"
 	"github.com/ava-labs/libevm/trie"
 )
 
+/* ===== Common =====*/
+
 func blockNumDBKey(prefix string, blockNum uint64) []byte {
 	return binary.BigEndian.AppendUint64([]byte(prefix), blockNum)
 }
@@ -26,19 +27,16 @@ func execResultsDBKey(blockNum uint64) []byte {
 	return blockNumDBKey("sae-post-exec-", blockNum)
 }
 
-// WritePostExecutionState writes, to w, the values passed to
-// [Block.MarkExecuted].
-func (b *Block) WritePostExecutionState(w ethdb.KeyValueWriter) error {
-	e := b.execution.Load()
-	if e == nil {
-		return fmt.Errorf("writing post-execution state of block %d before execution", b.Height())
-	}
+func (b *Block) writePostExecutionState(w ethdb.KeyValueWriter, e *executionResults) error {
 	return b.writeToKVStore(w, execResultsDBKey, e.MarshalCanoto())
 }
 
-// RestorePostExecutionState is the inverse of [Block.WritePostExecutionState].
-// The receipts MUST match those originally passed to [Block.MarkExecuted]
-// before the post-execution state was written to the database.
+// RestorePostExecutionState restores b to the same post-execution state as when
+// [Block.MarkExecuted] was called on it. This is only expected to be used after
+// a restart.
+//
+// The receipts MUST match those originally passed to [Block.MarkExecuted] as
+// they will be checked against the persisted Merkle root.
 func (b *Block) RestorePostExecutionState(db ethdb.Database, receipts types.Receipts) error {
 	e, err := readExecResults(db, b.NumberU64())
 	if err != nil {
@@ -47,7 +45,7 @@ func (b *Block) RestorePostExecutionState(db ethdb.Database, receipts types.Rece
 	if e.receiptRoot != types.DeriveSha(receipts, trie.NewStackTrie(nil)) {
 		return fmt.Errorf("restoring execution state of block %d: receipt-root mismatch", b.Height())
 	}
-	return b.MarkExecuted(e.byGas.Clone(), time.Time{}, receipts, e.stateRootPost)
+	return b.markExecuted(e)
 }
 
 // StateRootPostExecution returns the state root passed to [Block.MarkExecuted]

blocks/execution.go

@@ -7,7 +7,9 @@ import (
 
 	"github.com/ava-labs/avalanchego/vms/components/gas"
 	"github.com/ava-labs/libevm/common"
+	"github.com/ava-labs/libevm/core/rawdb"
 	"github.com/ava-labs/libevm/core/types"
+	"github.com/ava-labs/libevm/ethdb"
 	"github.com/ava-labs/libevm/trie"
 	"github.com/ava-labs/strevm/gastime"
 )
@@ -48,10 +50,10 @@ func (e *executionResults) Equal(f *executionResults) bool {
 // time(s) and with the specified results. This function MUST NOT be called more
 // than once. The wall-clock [time.Time] is for metrics only.
 //
-// Note: only in-memory state is updated. Receipts SHOULD be stored
-// independently of a call to MarkExecuted, along with a call to
-// [Block.WritePostExecutionState].
-func (b *Block) MarkExecuted(byGas *gastime.Time, byWall time.Time, receipts types.Receipts, stateRootPost common.Hash) error {
+// MarkExecuted guarantees that state is persisted to the database before
+// in-memory indicators of execution are updated. [Block.Executed] returning
+// true is therefore indicative of a successful database write by MarkExecuted.
+func (b *Block) MarkExecuted(db ethdb.Database, isLastSyncBlock bool, byGas *gastime.Time, byWall time.Time, receipts types.Receipts, stateRootPost common.Hash) error {
 	var used gas.Gas
 	for _, r := range receipts {
 		used += gas.Gas(r.GasUsed)
@@ -65,22 +67,45 @@ func (b *Block) MarkExecuted(byGas *gastime.Time, byWall time.Time, receipts typ
 		receiptRoot:   types.DeriveSha(receipts, trie.NewStackTrie(nil)),
 		stateRootPost: stateRootPost,
 	}
+
+	batch := db.NewBatch()
+	if !isLastSyncBlock {
+		// Although the last synchronous block is required to be the head block
+		// at the time of upgrade, not setting the head here allows this method
+		// to be called idempotently for that block. This is useful when
+		// converting it to an SAE block.
+		rawdb.WriteHeadBlockHash(batch, b.Hash())
+		rawdb.WriteHeadHeaderHash(batch, b.Hash())
+	}
+	rawdb.WriteReceipts(batch, b.Hash(), b.NumberU64(), receipts)
+	if err := b.writePostExecutionState(batch, e); err != nil {
+		return err
+	}
+	if err := batch.Write(); err != nil {
+		return err
+	}
+
+	return b.markExecuted(e)
+}
+
+func (b *Block) markExecuted(e *executionResults) error {
 	if !b.execution.CompareAndSwap(nil, e) {
 		b.log.Error("Block re-marked as executed")
 		return fmt.Errorf("block %d re-marked as executed", b.Height())
 	}
 	return nil
 }
 
-// Executed reports whether [Block.MarkExecuted] has been called.
+// Executed reports whether [Block.MarkExecuted] has been called and returned
+// without error.
 func (b *Block) Executed() bool {
 	return b.execution.Load() != nil
 }
 
 func zero[T any]() (z T) { return }
 
 // ExecutedByGasTime returns a clone of the gas time passed to
-// [Block.MarkExecuted] or nil if no such call has been made.
+// [Block.MarkExecuted] or nil if no such successful call has been made.
 func (b *Block) ExecutedByGasTime() *gastime.Time {
 	if e := b.execution.Load(); e != nil {
 		return e.byGas.Clone()
@@ -90,7 +115,7 @@ func (b *Block) ExecutedByGasTime() *gastime.Time {
 }
 
 // ExecutedByWallTime returns the wall time passed to [Block.MarkExecuted] or
-// the zero time if no such call has been made.
+// the zero time if no such successful call has been made.
 func (b *Block) ExecutedByWallTime() time.Time {
 	if e := b.execution.Load(); e != nil {
 		return e.byWall
@@ -100,7 +125,7 @@ func (b *Block) ExecutedByWallTime() time.Time {
 }
 
 // Receipts returns the receipts passed to [Block.MarkExecuted] or nil if no
-// such call has been made.
+// such successful call has been made.
 func (b *Block) Receipts() types.Receipts {
 	if e := b.execution.Load(); e != nil {
 		return slices.Clone(e.receipts)
@@ -110,7 +135,7 @@ func (b *Block) Receipts() types.Receipts {
 }
 
 // PostExecutionStateRoot returns the state root passed to [Block.MarkExecuted]
-// or the zero hash if no such call has been made.
+// or the zero hash if no such successful call has been made.
 func (b *Block) PostExecutionStateRoot() common.Hash {
 	if e := b.execution.Load(); e != nil {
 		return e.stateRootPost

db.go

@@ -39,6 +39,8 @@ func (vm *VM) upgradeLastSynchronousBlock(hash common.Hash) error {
 	}
 
 	if err := block.MarkExecuted(
+		vm.db,
+		true,
 		gastime.New(
 			block.Time(),
 			// TODO(arr4n) get the gas target and post-execution excess of the
@@ -51,30 +53,19 @@ func (vm *VM) upgradeLastSynchronousBlock(hash common.Hash) error {
 	); err != nil {
 		return err
 	}
-
-	batch := vm.db.NewBatch()
-	if err := block.WritePostExecutionState(batch); err != nil {
-		return err
-	}
-	if err := block.WriteLastSettledNumber(batch); err != nil {
-		return err
-	}
-	if err := batch.Write(); err != nil {
+	if err := block.WriteLastSettledNumber(vm.db); err != nil {
 		return err
 	}
+	block.MarkSettled()
 
+	if err := block.CheckInvariants(true, true); err != nil {
+		return fmt.Errorf("upgrading last synchronous block: %v", err)
+	}
 	vm.logger().Info(
 		"Last synchronous block before SAE",
 		zap.Uint64("timestamp", block.Time()),
 		zap.Stringer("hash", block.Hash()),
 	)
-
-	// Although the block isn't returned, do this defensively in case of a
-	// future refactor.
-	block.MarkSettled()
-	if err := block.CheckInvariants(true, true); err != nil {
-		return fmt.Errorf("upgrading last synchronous block: %v", err)
-	}
 	return nil
 }
 

saexec/execution.go

@@ -11,7 +11,6 @@ import (
 	"github.com/ava-labs/avalanchego/vms/components/gas"
 	"github.com/ava-labs/libevm/common"
 	"github.com/ava-labs/libevm/core"
-	"github.com/ava-labs/libevm/core/rawdb"
 	"github.com/ava-labs/libevm/core/state"
 	"github.com/ava-labs/libevm/core/state/snapshot"
 	"github.com/ava-labs/libevm/core/types"
@@ -156,28 +155,17 @@ func (e *Executor) execute(ctx context.Context, b *blocks.Block) error {
 	if err != nil {
 		return err
 	}
-	if err := b.MarkExecuted(e.gasClock.Clone(), endTime, receipts, root); err != nil {
+	// The strict ordering of the next 3 calls guarantees invariants that MUST
+	// NOT be broken:
+	//
+	// 1. [blocks.Block.MarkExecuted] guarantees disk then in-memory changes.
+	// 2. Internal indicator of last executed MUST follow in-memory change.
+	// 3. External indicator of last executed MUST follow internal indicator.
+	if err := b.MarkExecuted(e.db, false, e.gasClock.Clone(), endTime, receipts, root); err != nil {
 		return err
 	}
-
-	batch := e.db.NewBatch()
-	rawdb.WriteHeadBlockHash(batch, b.Hash())
-	rawdb.WriteHeadHeaderHash(batch, b.Hash())
-	// TODO(arr4n) move writing of receipts into settlement, where the
-	// associated state root is committed on the trie DB. For now it's done here
-	// to support immediate eth_getTransactionReceipt as the API treats
-	// last-executed as the "latest" block and the upstream API implementation
-	// requires this write.
-	rawdb.WriteReceipts(batch, b.Hash(), b.NumberU64(), receipts)
-	if err := b.WritePostExecutionState(batch); err != nil {
-		return err
-	}
-	if err := batch.Write(); err != nil {
-		return err
-	}
-
-	e.sendPostExecutionEvents(b.Block, receipts)
-	e.lastExecuted.Store(b)
+	e.lastExecuted.Store(b)                      // (2)
+	e.sendPostExecutionEvents(b.Block, receipts) // (3)
 
 	e.log.Debug(
 		"Block execution complete",