fix: do not expose settled blocks in an invalid state

sae/block_builder.go

@@ -66,7 +66,7 @@ func (b *blockBuilder) rebuild(
 ) (*blocks.Block, error) {
 	// Moving sender caching into [VM.ParseBlock] is not robust as there is
 	// insufficient spam protection, so it must be done here.
-	signer := b.exec.SignerForBlock(block)
+	signer := b.exec.SignerForBlock(block.EthBlock())
 	core.SenderCacher.Recover(signer, block.Transactions()) // asynchronous
 
 	txs := make([]*txgossip.LazyTransaction, len(block.Transactions()))

sae/blocks.go

@@ -108,6 +108,47 @@ func canonicalBlock(db ethdb.Database, num uint64) (*types.Block, error) {
 	return b, nil
 }
 
+func (vm *VM) settledBlockFromDB(db ethdb.Reader, hash common.Hash, num uint64) (*blocks.Block, error) {
+	// Before doing any disk IO, we sanity check that num is for a settled
+	// block.
+	//
+	// If using this function with [readByHash] this check is required.
+	// Otherwise, there is a possible (read: near impossible but non-zero)
+	// chance that [VM.VerifyBlock] and [VM.AcceptBlock] were *both* called
+	// between checking the in-memory block store and loading the canonical
+	// number from the database. That could result in attempting to restore an
+	// unexecuted block, which would report an error.
+	//
+	// TODO(arr4n) I think [readHash] should be providing this guarantee
+	// as it has access to the [syncMap] and its lock.
+	if vm.last.settled.Load().Height() < num {
+		return nil, database.ErrNotFound
+	}
+
+	ethB := rawdb.ReadBlock(db, hash, num)
+	if num > vm.last.synchronous {
+		return blocks.RestoreSettledBlock(
+			ethB,
+			vm.log(),
+			vm.db,
+			vm.xdb,
+			vm.exec.ChainConfig(),
+		)
+	}
+
+	b, err := vm.blockBuilder.new(ethB, nil, nil)
+	if err != nil {
+		return nil, err
+	}
+	// Excess is only used for executing the next block, which can never
+	// be the case if `b` isn't actually the last synchronous block, so
+	// passing the same value for all is OK.
+	if err := b.MarkSynchronous(vm.hooks, vm.db, vm.xdb, vm.config.ExcessAfterLastSynchronous); err != nil {
+		return nil, err
+	}
+	return b, nil
+}
+
 // GetBlock returns the block with the given ID, or [database.ErrNotFound].
 //
 // It is expected that blocks that have been successfully verified should be
@@ -124,47 +165,7 @@ func (vm *VM) GetBlock(ctx context.Context, id ids.ID) (*blocks.Block, error) {
 		func(b *blocks.Block) *blocks.Block {
 			return b
 		},
-		func(db ethdb.Reader, hash common.Hash, num uint64) (*blocks.Block, error) {
-			// A block that's not in memory has either been rejected, not yet
-			// verified, or settled. Of these, only the latter would be in the
-			// database.
-			//
-			// There is, however, a negligible (read: near impossible) but
-			// non-zero chance that [VM.VerifyBlock] and [VM.AcceptBlock] were
-			// *both* called between [readByHash] checking the in-memory block
-			// store and loading the canonical number from the database. That
-			// could result in an unexecuted block, which would cause an error
-			// when restoring it.
-			//
-			// TODO(arr4n) I think [readHash] should be providing this guarantee
-			// as it has access to the [syncMap] and its lock.
-			if vm.last.settled.Load().Height() < num {
-				return nil, database.ErrNotFound
-			}
-
-			ethB := rawdb.ReadBlock(db, hash, num)
-			if num > vm.last.synchronous {
-				return blocks.RestoreSettledBlock(
-					ethB,
-					vm.log(),
-					vm.db,
-					vm.xdb,
-					vm.exec.ChainConfig(),
-				)
-			}
-
-			b, err := vm.blockBuilder.new(ethB, nil, nil)
-			if err != nil {
-				return nil, err
-			}
-			// Excess is only used for executing the next block, which can never
-			// be the case if `b` isn't actually the last synchronous block, so
-			// passing the same value for all is OK.
-			if err := b.MarkSynchronous(vm.hooks, vm.db, vm.xdb, vm.config.ExcessAfterLastSynchronous); err != nil {
-				return nil, err
-			}
-			return b, nil
-		},
+		vm.settledBlockFromDB,
 		database.ErrNotFound,
 	)
 }

sae/rpc.go

@@ -260,18 +260,18 @@ type blockChainAPI struct {
 // We override [ethapi.BlockChainAPI.GetBlockReceipts] so that we do not return
 // an error when a user queries a known, but not yet executed, block.
 func (b *blockChainAPI) GetBlockReceipts(ctx context.Context, blockNrOrHash rpc.BlockNumberOrHash) ([]map[string]any, error) {
-	blk, err := b.b.getBlock(blockNrOrHash)
-	if err != nil || !blk.Executed() {
+	receipts, blk, err := b.b.getReceipts(blockNrOrHash)
+	if err != nil || blk == nil {
 		return nil, nil //nolint:nilerr // This follows Geth behavior for [ethapi.BlockChainAPI.GetBlockReceipts]
 	}
 
-	signer := b.b.vm.exec.SignerForBlock(blk)
 	hash := blk.Hash()
 	num := blk.NumberU64()
+	signer := b.b.vm.exec.SignerForBlock(blk)
 	txs := blk.Transactions()
 
 	result := make([]map[string]any, len(txs))
-	for i, receipt := range blk.Receipts() {
+	for i, receipt := range receipts {
 		result[i] = ethapi.MarshalReceipt(receipt, hash, num, signer, txs[i], i)
 	}
 	return result, nil
@@ -628,37 +628,32 @@ func (b *ethAPIBackend) SetHead(uint64) {
 }
 
 func (b *ethAPIBackend) GetReceipts(ctx context.Context, hash common.Hash) (types.Receipts, error) {
-	blk, err := b.getBlock(rpc.BlockNumberOrHashWithHash(hash, false))
-	if err != nil || !blk.Executed() {
+	receipts, _, err := b.getReceipts(rpc.BlockNumberOrHashWithHash(hash, false))
+	if err != nil {
 		return nil, nil //nolint:nilerr // This follows Geth behavior for [ethapi.Backend.GetReceipts]
 	}
-	return blk.Receipts(), nil
+	return receipts, nil
 }
 
-// TODO(arr4n) this returns settled blocks in an invalid state. Use
-// [VM.GetBlock] in or after PR 183.
-func (b *ethAPIBackend) getBlock(numOrHash rpc.BlockNumberOrHash) (*blocks.Block, error) {
-	n, hash, err := b.resolveBlockNumberOrHash(numOrHash)
-	if err != nil {
-		return nil, err
-	}
-	if blk, ok := b.vm.blocks.Load(hash); ok {
-		return blk, nil
-	}
-
-	ethBlock := rawdb.ReadBlock(b.vm.db, hash, n)
-	if ethBlock == nil {
-		return nil, nil
-	}
-
-	blk, err := b.vm.blockBuilder.new(ethBlock, nil, nil)
+// getReceipts resolves receipts and the underlying [types.Block] by number or
+// hash, checking in-memory blocks first then falling back to the database.
+// Returns nils for blocks that are not yet executed.
+func (b *ethAPIBackend) getReceipts(numOrHash rpc.BlockNumberOrHash) (types.Receipts, *types.Block, error) {
+	blk, err := readByNumberOrHash(
+		b,
+		numOrHash,
+		func(b *blocks.Block) *blocks.Block {
+			return b
+		},
+		b.vm.settledBlockFromDB,
+	)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
-	if err := blk.RestoreExecutionArtefacts(b.vm.db, b.vm.xdb, b.vm.exec.ChainConfig()); err != nil {
-		return nil, fmt.Errorf("restoring execution artefacts: %w", err)
+	if !blk.Executed() {
+		return nil, nil, nil
 	}
-	return blk, nil
+	return blk.Receipts(), blk.EthBlock(), nil
 }
 
 type noopSubscription struct {

saexec/execution.go

@@ -114,7 +114,7 @@ func (e *Executor) execute(b *blocks.Block, logger logging.Logger) error {
 	gasPool := core.GasPool(math.MaxUint64) // required by geth but irrelevant so max it out
 	var blockGasConsumed gas.Gas
 
-	signer := e.SignerForBlock(b)
+	signer := e.SignerForBlock(b.EthBlock())
 	receipts := make(types.Receipts, len(b.Transactions()))
 	for ti, tx := range b.Transactions() {
 		stateDB.SetTxContext(tx.Hash(), ti)

saexec/saexec.go

@@ -134,8 +134,8 @@ func (e *Executor) Close() error {
 }
 
 // SignerForBlock returns the transaction signer for the block.
-func (e *Executor) SignerForBlock(b *blocks.Block) types.Signer {
-	return types.MakeSigner(e.chainConfig, b.Number(), b.BuildTime())
+func (e *Executor) SignerForBlock(b *types.Block) types.Signer {
+	return types.MakeSigner(e.chainConfig, b.Number(), b.Time())
 }
 
 // ChainConfig returns the config originally passed to [New].