diff --git a/sae/block_builder.go b/sae/block_builder.go index 28654e6f..1c411a28 100644 --- a/sae/block_builder.go +++ b/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())) diff --git a/sae/blocks.go b/sae/blocks.go index a445955c..36026622 100644 --- a/sae/blocks.go +++ b/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, ) } diff --git a/sae/rpc.go b/sae/rpc.go index 977dc6ac..c703168f 100644 --- a/sae/rpc.go +++ b/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 { diff --git a/saexec/execution.go b/saexec/execution.go index 713e3c9b..f5a3ef0c 100644 --- a/saexec/execution.go +++ b/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) diff --git a/saexec/saexec.go b/saexec/saexec.go index 9bcfa5a9..00261524 100644 --- a/saexec/saexec.go +++ b/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].