core: check txn length when building a transaction set

[jmank88]

This PR proposes a length check to prevent an index out of range panic when building NewTransactionsByPriceAndNonce. Perhaps empty slice values are actually 'bad data' which shouldn't have been in the input map in the first place, but this check is cheap.

panic: runtime error: index out of range

goroutine 105 [running]:
github.com/ethereum/go-ethereum/core/types.NewTransactionsByPriceAndNonce(0x10cd7e0, 0xc4214605a0, 0xc422d00f90, 0x0)
	/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/core/types/transaction.go:334 +0x4ec
github.com/ethereum/go-ethereum/miner.(*worker).commitNewWork(0xc4202534a0)
	/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/miner/worker.go:454 +0x9dc
github.com/ethereum/go-ethereum/miner.(*worker).update(0xc4202534a0)
	/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/miner/worker.go:254 +0x781
created by github.com/ethereum/go-ethereum/miner.newWorker
	/go-ethereum/build/_workspace/src/github.com/ethereum/go-ethereum/miner/worker.go:158 +0x4ae

[karalabe]

How did you get the crash in the first place? Empty slices should most definitely not end up in that map.

[jmank88]

Nothing unusual - just sending a regular transaction on a private clique network. I have yet to pinpoint where/why the tx is being discarded. We patched this on a fork back in April, but it was only occasional and we assumed that we had introduced the problem with a previous change. However, this is now happening consistently for us with the latest geth.

Version: 1.8.13-unstable
Git Commit: 2e0391ea84ba02b6a8462209f55019f07d01bdd4

[karalabe]

Any chance of providing a repro script for us? I'd be really curious what goes wrong.

[jmank88]

Dodging the panic helps, but we're still blocked by the transaction being dropped, so I'll track it down one way or another.

[jmank88]

I traced it to Removed fairness-exceeding pending transaction, even as the only tx in the pool.
It turns out that specifying only one field for the transaction pool configuration results in the omitted fields being zeroed out. In our case, with just NoLocals set explicitly, GlobalSlots and AccountSlots are set to 0 instead of the default values.

[Eth.TxPool]
NoLocals = true

So there are a few potential problems here:

1. Should fields omitted from a config.toml be zeroed, or set to defaults? (our fork is patched with a more exhaustive (*TxPoolConfig).sanitize() method to set defaults - I would be happy to put up a PR)

2. If a tx is legitimately bumped in this way during an add, the method should not return successful as if the tx was added. Perhaps a check after calling promoteExecutables() to verify it's still in the pool would be enough.

3. After calling (*txList).Cap, the list could be empty. Some callers follow up with a if list.Empty() { delete(pool.pending, addr) }, but not all.

[karalabe]

Oh, good catch, they should most definitely not default to 0. That would explain why the weird behavior happens.

[jmank88]

PR for 1 here. This should prevent the original empty slice panic, and avoid 3. 2 may still be a problem (but not a panic, just misleading).

[sunanxiang]

Has this problem been solved? I also have this problem. @jmank88

[sunanxiang]

for from, accTxs := range txs {
	heads = append(heads, accTxs[0])
	// Ensure the sender address is from the signer
	acc, _ := Sender(signer, accTxs[0])
	txs[acc] = accTxs[1:]
	if from != acc {
		delete(txs, from)
	}
}

when acc is not equal to from, len(accTxs) is zero ,this problem will arise. I think there should have a judgement to deal with the length of accTxs.

[karalabe]

Closing this in favor of #17210, please check the review comments and fix.

@sunanxiang That seems to be an unrelated but interesting problem. Could you describe how you managed to hit this issue and how I could reproduce it? Please open a new issue for it.