cleanups and documentation

Author: jwasinger

core/blockchain.go

@@ -1892,10 +1892,9 @@ func (bc *BlockChain) insertChain(chain types.Blocks, setHead bool, makeWitness
 		start := time.Now()
 
 		blockHasAccessList := block.Body().AccessList != nil
-		// BAL generation/verification not enabled pre-selfdestruct removal
-
-		// TODO: state diff recording will be activated any time it's cancun.
-		// This shouldn't happen....
+		// BAL generation/verification is not enabled pre-selfdestruct removal.
+		// TODO: there is a bug where state diff recording will be activated if
+		// cancun is enabled, this should be fixed before merging!
 		forkSupportsBAL := bc.chainConfig.IsCancun(block.Number(), block.Time())
 		makeBAL := forkSupportsBAL && !blockHasAccessList
 		validateBAL := forkSupportsBAL && blockHasAccessList
@@ -2025,6 +2024,7 @@ func (bc *BlockChain) processBlock(parentRoot common.Hash, block *types.Block, s
 			vmCfg := bc.cfg.VmConfig
 			vmCfg.Tracer = nil
 			if block.Body().AccessList == nil {
+				// only use the state prefetcher for non-BAL blocks.
 				bc.prefetcher.Prefetch(block, throwaway, vmCfg, &interrupt)
 			}
 

core/state/state_object.go

@@ -285,7 +285,6 @@ func (s *stateObject) finalise() *bal.AccountState {
 			// to avoid thrashing the data structures.
 			delete(s.uncommittedStorage, key)
 
-			// TODO: ensure that we should record the diff here: i.e. this storage slot had a different val at the start of the transaction
 			if s.db.enableStateDiffRecording {
 				if s.db.constructionBAL != nil {
 					s.db.BlockAccessList().StorageWrite(uint16(s.db.balIndex), s.address, key, value)
@@ -638,6 +637,7 @@ func (s *stateObject) setCode(codeHash common.Hash, code []byte) {
 	s.data.CodeHash = codeHash[:]
 }
 
+// setCode sets the code and hash and dirty markers.
 func (s *stateObject) setCodeModified(codeHash common.Hash, code []byte) {
 	s.setCode(codeHash, code)
 	s.dirtyCode = true

core/state/statedb.go

@@ -318,6 +318,8 @@ func (s *StateDB) AddRefund(gas uint64) {
 	s.refund += gas
 }
 
+// InstantiateWithStateDiffs instantiates the live object based on accounts
+// which appeared in the total state diff of a block, and were also preexisting.
 func (s *StateDB) InstantiateWithStateDiffs(totalDiff *bal.StateDiff) {
 	stateAccounts := new(sync.Map)
 	wg := new(sync.WaitGroup)
@@ -826,14 +828,11 @@ func (s *StateDB) Finalise(deleteEmptyObjects bool) (diff *bal.StateDiff) {
 			continue
 		}
 		if obj.selfDestructed || (deleteEmptyObjects && obj.empty()) {
-			// TODO: for testing purposes we should probably have tests that create/destroy the same account multiple times via this same edge-case with create2
-			if obj.txPreBalance != nil && !obj.txPreBalance.IsZero() { // TODO: IsZero check is somehow needed for coinbase.  figure out why.
-				// TODO: need to ensure that we aren't recording accounts in the state diff/BAL which were created/selfdestructed in the same transaction.
-				// It should be as easy as modifying the check above to not include selfdestructed contracts.  However, doing this causes some blockchain tests
-				// to fail and I'm not sure why.
-				//
-				// TODO: the coinbase can be touched and then become empty here: see BlockchainBAL/GeneralStateTests/stExample/solidityExample.json/solidityExample_d0g0v0_Cancun
-				// ^ coinbase probably shouldn't be in the BAL/statediff in this case.
+			// record state diffs for any preexisting accounts which became empty
+			if s.enableStateDiffRecording && obj.txPreBalance != nil && !obj.txPreBalance.IsZero() {
+				// TODO: IsZero check is somehow needed for coinbase.  figure out why.
+				// TODO: the above check should be as easy as checking that the account was not selfdestructed in the
+				// current transaction, but that causes spec tests to fail.  need to figure out why.
 				if s.constructionBAL != nil {
 					s.constructionBAL.BalanceChange(uint16(s.balIndex), obj.address, uint256.NewInt(0))
 				}
@@ -855,7 +854,7 @@ func (s *StateDB) Finalise(deleteEmptyObjects bool) (diff *bal.StateDiff) {
 
 			if s.enableStateDiffRecording && (accountPost.Nonce != nil || accountPost.Code != nil || accountPost.StorageWrites != nil || accountPost.Balance != nil) {
 				// the account executed SENDALL but did not send a balance, don't include it in the diff
-				// TODO: probably shouldn't include the account in the dirty set in this case
+				// TODO: probably shouldn't include the account in the dirty set in this case (unrelated to the BAL changes)
 				diff.Mutations[obj.address] = accountPost
 			}
 			s.markUpdate(addr)
@@ -1062,6 +1061,7 @@ func (s *StateDB) SetTxSender(sender common.Address) {
 	s.sender = sender
 }
 
+// ApplyPrestate applies finalised pre-state changes which occurred between the start of the block and the end of the previous transaction.
 func (s *StateDB) ApplyPrestate(prestateDiff *bal.StateDiff) {
 	for addr, accountDiff := range prestateDiff.Mutations {
 		stateObject, preexisting := s.stateObjects[addr]
@@ -1110,6 +1110,8 @@ func (s *StateDB) ApplyPrestate(prestateDiff *bal.StateDiff) {
 	}
 }
 
+// ApplyStateDiff applies a state diff to the StateDB.  All state changes will be marked as mutations
+// for the purpose of applying them in the next call to Commit.
 func (s *StateDB) ApplyStateDiff(diff *bal.StateDiff) {
 	for addr, accountDiff := range diff.Mutations {
 		stateObject, ok := s.stateObjects[addr]

core/state_processor.go

@@ -158,6 +158,8 @@ func (p *StateProcessor) Process(block *types.Block, statedb *state.StateDB, cfg
 	}, nil
 }
 
+// ProcessWithAccessList performs EVM execution and state root computation for a block which is known
+// to contain an access list.
 func (p *StateProcessor) ProcessWithAccessList(block *types.Block, statedb *state.StateDB, cfg vm.Config) (chan *ProcessResultWithMetrics, error) {
 	var (
 		header      = block.Header()
@@ -179,12 +181,12 @@ func (p *StateProcessor) ProcessWithAccessList(block *types.Block, statedb *stat
 	rootCalcErrCh := make(chan error) // used for communicating if the state root calculation doesn't match the reported root
 	pStart := time.Now()
 	var (
-		tPreprocess  time.Duration
+		tPreprocess  time.Duration // time to create a set of prestates for parallel transaction execution
 		tVerifyStart time.Time
-		tVerify      time.Duration
+		tVerify      time.Duration // time to compute and verify the state root
 		tExecStart   time.Time
-		tExec        time.Duration
-		tPostprocess time.Duration
+		tExec        time.Duration // time to execute block transactions
+		tPostprocess time.Duration // time to perform post-transaction execution system calls and withdrawals.
 	)
 
 	// called by resultHandler when all transactions have successfully executed.
@@ -252,9 +254,6 @@ func (p *StateProcessor) ProcessWithAccessList(block *types.Block, statedb *stat
 		finalDiff := postTxState.Finalise(true)
 		computedDiff.Merge(finalDiff)
 
-		// TODO:  at each step, we should only be comparing the "intermediate" state diffs
-		// not the entire state diff up until that point.
-
 		if err := bal.ValidateStateDiff(expectedStateDiff, computedDiff); err != nil {
 			return &ProcessResult{
 				Error: fmt.Errorf("post-transaction-execution state transition produced a different diff that what was reported in the BAL"),
@@ -340,12 +339,12 @@ func (p *StateProcessor) ProcessWithAccessList(block *types.Block, statedb *stat
 	execTx := func(ctx context2.Context, tx *types.Transaction, idx int, db *state.StateDB, prestateDiff, expectedDiff *bal.StateDiff) *txExecResult {
 		db.ApplyPrestate(prestateDiff)
 		// if an error with another transaction rendered the block invalid, don't proceed with executing this one
-		// TODO: also interrupt any currently-executing transactions if one failed.
 		select {
 		case <-ctx.Done():
 			return &txExecResult{err: ctx.Err()}
 		default:
 		}
+		// TODO: also interrupt any currently-executing transactions if one failed.
 		var tracingStateDB = vm.StateDB(db)
 		if hooks := cfg.Tracer; hooks != nil {
 			tracingStateDB = state.NewHookedState(db, hooks)
@@ -362,7 +361,7 @@ func (p *StateProcessor) ProcessWithAccessList(block *types.Block, statedb *stat
 		db.SetTxSender(sender)
 		db.SetTxContext(tx.Hash(), idx)
 
-		evm.StateDB = db // TODO: unsure if need to set this here since the evm should maintain a reference to the db but I recall that adding this fixed some broken tests
+		evm.StateDB = db
 		gp := new(GasPool)
 		gp.SetGas(block.GasLimit())
 		var gasUsed uint64
@@ -398,47 +397,47 @@ func (p *StateProcessor) ProcessWithAccessList(block *types.Block, statedb *stat
 	context = NewEVMBlockContext(header, p.chain, nil)
 	evm := vm.NewEVM(context, tracingStateDB, p.config, cfg)
 
-	// process beacon-root and parent block system contracts.
-	// do not include the storage writes in the BAL:
-	// * beacon root will be provided as a standalone field in the BAL
-	// * parent block hash is already in the header field of the block
-
+	// convert the BAL entries into an ordered list of state diffs per index
 	intermediateStateDiffs := bal.BuildStateDiffs(block.Body().AccessList, len(block.Transactions()))
 	preTxDiff := intermediateStateDiffs[0]
 
-	totalDiff := bal.StateDiff{make(map[common.Address]*bal.AccountState)}
-	var totalDiffs []*bal.StateDiff
-	for _, diff := range intermediateStateDiffs {
-		totalDiff.Merge(diff.Copy()) // TODO: it shouldn't be necessary to Copy here
-		totalDiffs = append(totalDiffs, totalDiff.Copy())
-	}
-	statedb.InstantiateWithStateDiffs(&totalDiff)
-
-	computedDiff := &bal.StateDiff{make(map[common.Address]*bal.AccountState)}
+	// validate the correctness of pre-transaction execution state changes
+	computedPreTxDiff := &bal.StateDiff{make(map[common.Address]*bal.AccountState)}
 	if beaconRoot := block.BeaconRoot(); beaconRoot != nil {
-		computedDiff.Merge(ProcessBeaconBlockRoot(*beaconRoot, evm))
+		computedPreTxDiff.Merge(ProcessBeaconBlockRoot(*beaconRoot, evm))
 	}
 	if p.config.IsPrague(block.Number(), block.Time()) || p.config.IsVerkle(block.Number(), block.Time()) {
-		computedDiff.Merge(ProcessParentBlockHash(block.ParentHash(), evm))
+		computedPreTxDiff.Merge(ProcessParentBlockHash(block.ParentHash(), evm))
 	}
-
-	if err := bal.ValidateStateDiff(preTxDiff, computedDiff); err != nil {
+	if err := bal.ValidateStateDiff(preTxDiff, computedPreTxDiff); err != nil {
 		return nil, err
 	}
 
-	var txPrestates []*state.StateDB
+	// compute the aggregate state diff of the block
+	totalDiff := bal.StateDiff{make(map[common.Address]*bal.AccountState)}
+	var totalDiffs []*bal.StateDiff
+	for _, diff := range intermediateStateDiffs {
+		totalDiff.Merge(diff.Copy()) // TODO: it shouldn't be necessary to Copy here
+		totalDiffs = append(totalDiffs, totalDiff.Copy())
+	}
 
-	// Iterate over and process the individual transactions
+	// instantiate a set of StateDBs to be used for executing each transaction in parallel
+	statedb.InstantiateWithStateDiffs(&totalDiff)
+	var txPrestates []*state.StateDB
 	for range block.Transactions() {
 		state := statedb.Copy()
 		txPrestates = append(txPrestates, state)
 	}
+
+	// compute the post-tx state prestate (before applying final block system calls and eip-4895 withdrawals)
 	postTxState := statedb.Copy()
-	postTxState.ApplyStateDiff(totalDiffs[len(totalDiffs)-2])
+	postTxState.ApplyPrestate(totalDiffs[len(totalDiffs)-2])
 	postTxState.Finalise(true)
 
 	tPreprocess = time.Since(pStart)
 
+	// execute transactions and state root calculation in parallel
+
 	tExecStart = time.Now()
 	go resultHandler(intermediateStateDiffs[len(block.Transactions())+1], postTxState)
 	var workers errgroup.Group
@@ -455,7 +454,6 @@ func (p *StateProcessor) ProcessWithAccessList(block *types.Block, statedb *stat
 
 	statedb.ApplyStateDiff(&totalDiff)
 	statedb.Finalise(true)
-	//fmt.Printf("total diff is\n%s\napplied diff is\n%s\n", totalDiff.String(), appliedDiff.String())
 	go calcAndVerifyRoot(statedb.Copy(), block, rootCalcErrCh)
 
 	return resCh, nil

core/types/bal/bal_encoding.go

@@ -380,10 +380,8 @@ type AccountState struct {
 	StorageWrites map[common.Hash]common.Hash `json:"StorageWrites,omitempty"`
 }
 
-func (a *AccountState) IsEmpty() bool {
-	return a.Balance == nil || a.Balance.IsZero() || a.Nonce == nil || a.Code == nil
-}
-
+// Merge the changes of a future AccountState into the caller, resulting in the
+// combined state changes through next.
 func (a *AccountState) Merge(next *AccountState) {
 	if next.Balance != nil {
 		a.Balance = next.Balance
@@ -474,7 +472,7 @@ func (a *AccountState) Copy() *AccountState {
 	return res
 }
 
-// ValidateStateDiff asserts that the state from layerDiff are present in totalDiffAtLayer.
+// ValidateStateDiff asserts that both state diffs are equivalent.
 func ValidateStateDiff(balDiff, computedDiff *StateDiff) error {
 	for addr, computedAccountDiff := range computedDiff.Mutations {
 		balAccountDiff, ok := balDiff.Mutations[addr]
@@ -500,7 +498,8 @@ func (s *StateDiff) String() string {
 	return res.String()
 }
 
-// merge the future state from next into the current diff modifying the caller
+// Merge merges the state changes present in next into the caller.  After,
+// the state of the caller is the aggregate diff through next.
 func (s *StateDiff) Merge(next *StateDiff) {
 	for account, diff := range next.Mutations {
 		if mut, ok := s.Mutations[account]; ok {
@@ -529,35 +528,6 @@ func (s *StateDiff) Merge(next *StateDiff) {
 	}
 }
 
-func (s *StateDiff) MergePrev(prev *StateDiff) {
-	for account, diff := range prev.Mutations {
-		if mut, ok := s.Mutations[account]; ok {
-			if mut.Balance == nil {
-				mut.Balance = diff.Balance
-			}
-			if mut.Code == nil {
-				mut.Code = diff.Code
-			}
-			if mut.Nonce == nil {
-				mut.Nonce = diff.Nonce
-			}
-			if len(diff.StorageWrites) > 0 {
-				if mut.StorageWrites == nil {
-					mut.StorageWrites = diff.StorageWrites
-				} else {
-					for key, val := range diff.StorageWrites {
-						if _, ok := mut.StorageWrites[key]; !ok {
-							mut.StorageWrites[key] = val
-						}
-					}
-				}
-			}
-		} else {
-			s.Mutations[account] = diff
-		}
-	}
-}
-
 func (s *StateDiff) Copy() *StateDiff {
 	res := &StateDiff{make(map[common.Address]*AccountState)}
 	for addr, accountDiff := range s.Mutations {
@@ -685,10 +655,7 @@ func (it *BALIterator) Next() (mutations *StateDiff) {
 	return &diff
 }
 
-// BuildStateDiffs returns a set of ordered state diffs from the BAL
-// corresponding to each txIndex.  "until" is specified as the maximum
-// index which will be contained in the result.
-// It returns the aggregated state diff of all layers and a state diff for each layer.
+// BuildStateDiffs computes the ordered set of state diffs from an access list.
 func BuildStateDiffs(bal *BlockAccessList, txCount int) []*StateDiff {
 	stateDiffs := make([]*StateDiff, txCount+2)
 	for i := 0; i < len(stateDiffs); i++ {