fix crash when attempting to call debug_getAccessList on block that doesn't contain one. remove the new pre/post-tx tracer hooks and use the system contract start/end hooks instead in the bal tracer

core/block_access_list_tracer.go

@@ -17,6 +17,18 @@ type BlockAccessListTracer struct {
 
 	// the access list index that changes are currently being recorded into
 	balIdx uint16
+
+	// the number of system calls that have been invoked, used when building
+	// an access list to determine if the system calls being executed are
+	// before/after the block transactions.
+	sysCallCount int
+
+	// true if the tracer is processing post-tx state changes.  in this case
+	// we won't record the final index after the end of the second post-tx
+	// system contract but after the finalization of the block.
+	// This is because we have EIP-4895 withdrawals which are processed after the
+	// last system contracts execute and must be included in the BAL.
+	isPostTx bool
 }
 
 // NewBlockAccessListTracer returns an BlockAccessListTracer and a set of hooks
@@ -26,7 +38,6 @@ func NewBlockAccessListTracer() (*BlockAccessListTracer, *tracing.Hooks) {
 	}
 	hooks := &tracing.Hooks{
 		OnBlockFinalization:  balTracer.OnBlockFinalization,
-		OnPreTxExecutionDone: balTracer.OnPreTxExecutionDone,
 		OnTxEnd:              balTracer.TxEndHook,
 		OnTxStart:            balTracer.TxStartHook,
 		OnEnter:              balTracer.OnEnter,
@@ -38,11 +49,16 @@ func NewBlockAccessListTracer() (*BlockAccessListTracer, *tracing.Hooks) {
 		OnStorageRead:        balTracer.OnStorageRead,
 		OnAccountRead:        balTracer.OnAcountRead,
 		OnSelfDestructChange: balTracer.OnSelfDestruct,
+		OnSystemCallEnd:      balTracer.OnSystemCallEnd,
 	}
 	wrappedHooks, _ := tracing.WrapWithJournal(hooks)
 	return balTracer, wrappedHooks
 }
 
+func (a *BlockAccessListTracer) SetPostTx() {
+	a.isPostTx = true
+}
+
 // AccessList returns the constructed access list.
 // It is assumed that this is only called after all the block state changes
 // have been executed and the block has been finalized.
@@ -50,9 +66,15 @@ func (a *BlockAccessListTracer) AccessList() *bal.AccessListBuilder {
 	return a.builder
 }
 
-func (a *BlockAccessListTracer) OnPreTxExecutionDone() {
-	a.builder.FinaliseIdxChanges(0)
-	a.balIdx++
+func (a *BlockAccessListTracer) OnSystemCallEnd() {
+	if a.isPostTx {
+		return
+	}
+	a.sysCallCount++
+	if a.sysCallCount == 2 {
+		a.builder.FinaliseIdxChanges(a.balIdx)
+		a.balIdx++
+	}
 }
 
 func (a *BlockAccessListTracer) TxStartHook(vm *tracing.VMContext, tx *types.Transaction, from common.Address) {

core/blockchain.go

@@ -2231,7 +2231,6 @@ func (bc *BlockChain) ProcessBlock(parentRoot common.Hash, block *types.Block, s
 		defer func() {
 			bc.cfg.VmConfig.Tracer = nil
 		}()
-
 	}
 	// Process block using the parent state as reference point
 	pstart := time.Now()

core/parallel_state_processor.go

@@ -52,9 +52,12 @@ func (p *ParallelStateProcessor) prepareExecResult(block *types.Block, allStateR
 	header := block.Header()
 
 	balTracer, hooks := NewBlockAccessListTracer()
+	balTracer.SetPostTx()
+
 	tracingStateDB := state.NewHookedState(postTxState, hooks)
 	context := NewEVMBlockContext(header, p.chain, nil)
-	postTxState.SetAccessListIndex(len(block.Transactions()) + 1)
+	lastBALIdx := len(block.Transactions()) + 1
+	postTxState.SetAccessListIndex(lastBALIdx)
 
 	cfg := vm.Config{
 		Tracer:                  hooks,
@@ -122,6 +125,8 @@ func (p *ParallelStateProcessor) prepareExecResult(block *types.Block, allStateR
 	diff, stateReads := balTracer.builder.FinalizedIdxChanges()
 	allStateReads.Merge(stateReads)
 
+	// TODO: if there is a failure, we need to print out the detailed logs explaining why the BAL validation failed
+	// but logs are disabled when we are running tests to prevent a ton of output
 	balIdx := len(block.Transactions()) + 1
 	if !postTxState.BlockAccessList().ValidateStateDiff(balIdx, diff) {
 		return &ProcessResultWithMetrics{
@@ -129,9 +134,9 @@ func (p *ParallelStateProcessor) prepareExecResult(block *types.Block, allStateR
 		}
 	}
 
-	if err := postTxState.BlockAccessList().ValidateStateReads(*allStateReads); err != nil {
+	if !postTxState.BlockAccessList().ValidateStateReads(lastBALIdx, *allStateReads) {
 		return &ProcessResultWithMetrics{
-			ProcessResult: &ProcessResult{Error: err},
+			ProcessResult: &ProcessResult{Error: fmt.Errorf("BAL validation failure")},
 		}
 	}
 
@@ -315,8 +320,6 @@ func (p *ParallelStateProcessor) Process(block *types.Block, stateTransition *st
 		ProcessParentBlockHash(block.ParentHash(), evm)
 	}
 
-	balTracer.OnPreTxExecutionDone()
-
 	diff, stateReads := balTracer.builder.FinalizedIdxChanges()
 	if !statedb.BlockAccessList().ValidateStateDiff(0, diff) {
 		return nil, fmt.Errorf("BAL validation failure")

core/state/bal_reader.go

@@ -143,7 +143,42 @@ func (r *BALReader) ModifiedAccounts() (res []common.Address) {
 	return res
 }
 
-func (r *BALReader) ValidateStateReads(computedReads bal.StateAccesses) error {
+func logReadsDiff(idx int, address common.Address, computedReads map[common.Hash]struct{}, expectedReads []*bal.EncodedStorage) {
+	expectedReadsMap := make(map[common.Hash]struct{})
+	for _, er := range expectedReads {
+		expectedReadsMap[er.ToHash()] = struct{}{}
+	}
+
+	allReads := make(map[common.Hash]struct{})
+
+	for er := range expectedReadsMap {
+		allReads[er] = struct{}{}
+	}
+	for cr := range computedReads {
+		allReads[cr] = struct{}{}
+	}
+
+	var missingExpected, missingComputed []common.Hash
+
+	for storage := range allReads {
+		_, hasComputed := computedReads[storage]
+		_, hasExpected := expectedReadsMap[storage]
+		if hasComputed && !hasExpected {
+			missingExpected = append(missingExpected, storage)
+		}
+		if !hasComputed && hasExpected {
+			missingComputed = append(missingComputed, storage)
+		}
+	}
+	if len(missingExpected) > 0 {
+		log.Error("read storage slots which were not reported in the BAL", "index", idx, "address", address, missingExpected)
+	}
+	if len(missingComputed) > 0 {
+		log.Error("did not read storage slots which were reported in the BAL", "index", idx, "address", address, missingComputed)
+	}
+}
+
+func (r *BALReader) ValidateStateReads(idx int, computedReads bal.StateAccesses) bool {
 	// 1. remove any slots from 'allReads' which were written
 	// 2. validate that the read set in the BAL matches 'allReads' exactly
 	for addr, reads := range computedReads {
@@ -154,22 +189,25 @@ func (r *BALReader) ValidateStateReads(computedReads bal.StateAccesses) error {
 			}
 		}
 		if _, ok := r.accesses[addr]; !ok {
-			return fmt.Errorf("account %x was accessed during execution but is not present in the access list", addr)
+			log.Error(fmt.Sprintf("account %x was accessed during execution but is not present in the access list", addr))
+			return false
 		}
 
 		expectedReads := r.accesses[addr].StorageReads
 		if len(reads) != len(expectedReads) {
-			return fmt.Errorf("mismatch between the number of computed reads and number of expected reads")
+			logReadsDiff(idx, addr, reads, expectedReads)
+			return false
 		}
 
 		for _, slot := range expectedReads {
 			if _, ok := reads[slot.ToHash()]; !ok {
-				return fmt.Errorf("expected read is missing from BAL")
+				log.Error("expected read is missing from BAL")
+				return false
 			}
 		}
 	}
 
-	return nil
+	return true
 }
 
 // changesAt returns all state changes occurring at the given index.

eth/api_debug.go

@@ -546,5 +546,8 @@ func (api *DebugAPI) GetBlockAccessList(number rpc.BlockNumberOrHash) (interface
 	if block == nil {
 		return nil, fmt.Errorf("block not found")
 	}
+	if block.Body().AccessList == nil {
+		return nil, nil
+	}
 	return block.Body().AccessList.StringableRepresentation(), nil
 }

tests/block_test.go

@@ -214,7 +214,6 @@ func execBlockTest(t *testing.T, bt *testMatcher, test *BlockTest, buildAndVerif
 
 	for _, snapshot := range snapshotConf {
 		for _, dbscheme := range dbschemeConf {
-			//tracer := logger.NewJSONLogger(&logger.Config{}, os.Stdout)
 			if err := bt.checkFailure(t, test.Run(snapshot, dbscheme, false, buildAndVerifyBAL, nil, nil)); err != nil {
 				t.Errorf("test with config {snapshotter:%v, scheme:%v} failed: %v", snapshot, dbscheme, err)
 				return