core: avoid modification of accountSet cache in tx_pool

[duanhao0814]

when runReorg, we may copy the dirtyAccounts' accountSet cache to promoteAddrs
in which accounts will be promoted, however, if we have reset request at the
same time, we may reuse promoteAddrs and modify the cache content which is
against the original intention of accountSet cache. So, we need to make a new
slice here to avoid modify accountSet cache.

[fjl]

This change looks OK, but I would like to know if the re-use of the cache causes a bug. If there is a bug in txpool which is fixed by this change, we should add a test for that.

[duanhao0814]

As I see, re-use of this cache may not cause a bug here because accountSet.cache is only used as a return value in function flatten() and the function flatten() will be called in two situations:

1. used in runReorg as we see above, in which, dirtyAccounts is declared in scheduleReorgLoop and reset to nil directly after we call scheduleReorgLoop:
   

   go-ethereum/core/tx_pool.go

   Lines 961 to 967 in 3f649d4

   	go pool.runReorg(nextDone, reset, dirtyAccounts, queuedEvents)
   	// Prepare everything for the next round of reorg
   	curDone, nextDone = nextDone, make(chan struct{})
   	launchNextRun = false
   	reset, dirtyAccounts = nil, nil

   
   which means we will not call flatten() twice for one dirtyAccounts object, so modification to its cache may not cause a bug
2. used in the exported function TxPool.Locals, the only use of TxPool.Locals will not modify the cache so this is safe:
   

   go-ethereum/miner/worker.go

   Lines 952 to 957 in 3f649d4

   	for _, account := range w.eth.TxPool().Locals() {
   	if txs := remoteTxs[account]; len(txs) > 0 {
   	delete(remoteTxs, account)
   	localTxs[account] = txs
   	}
   	}

[karalabe]

The old comment was referring to the logical flow. Please add that back. The new comments seems to be just a text-explanation of the line below, which doesn't make much sense. Please revert that.

[duanhao0814]

Fixed

[holiman]

This looks a bit odd to me: https://github.com/ethereum/go-ethereum/pull/21159/files#diff-8fecce9bb4c486ebc22226cf681416e2R1020-R1022

Basically, if reset != nil, we're going to discard the result from flatten. So why do we even call it? Couldn't we just

	var promoteAddrs []common.Address
	if dirtyAccounts != nil && reset != nil {
		promoteAddrs = dirtyAccounts.flatten()
	}
	pool.mu.Lock()
	if reset != nil {

Not sure if I'm missing some subtle side-effect

[duanhao0814]

This looks a bit odd to me: https://github.com/ethereum/go-ethereum/pull/21159/files#diff-8fecce9bb4c486ebc22226cf681416e2R1020-R1022

Basically, if reset != nil, we're going to discard the result from flatten. So why do we even call it? Couldn't we just

	var promoteAddrs []common.Address
	if dirtyAccounts != nil && reset != nil {
		promoteAddrs = dirtyAccounts.flatten()
	}
	pool.mu.Lock()
	if reset != nil {

Not sure if I'm missing some subtle side-effect

Yes! I agree that if reset != nil, we even don't need to call flatten at all.
And also, when reset != nil, we can allocate an empty slice with capable size.

[holiman]

LGTM!

[holiman]

should be reset == nil, right?