ConcurrentModificationException in TransactionPool.addRemoteTransaction()

[matthew1001]

Description

2023-06-13 19:49:44.061+00:00 | EthScheduler-Transactions-0 | ERROR | Besu | Uncaught exception in thread "EthScheduler-Transactions-0"
java.util.ConcurrentModificationException
	at java.base/java.util.TreeMap$ValueSpliterator.forEachRemaining(TreeMap.java:3226)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
	at org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.signalInvalidAndGetDependentTransactions(AbstractPendingTransactionsSorter.java:539)
	at org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.signalInvalidAndRemoveDependentTransactions(AbstractPendingTransactionsSorter.java:546)
	at org.hyperledger.besu.ethereum.eth.transactions.TransactionPool.addRemoteTransaction(TransactionPool.java:340)
	at org.hyperledger.besu.ethereum.eth.transactions.TransactionPool.lambda$addRemoteTransactions$9(TransactionPool.java:268)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.stream.SortedOps$SizedRefSortingSink.end(SortedOps.java:357)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:510)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
	at org.hyperledger.besu.ethereum.eth.transactions.TransactionPool.addRemoteTransactions(TransactionPool.java:266)
	at org.hyperledger.besu.ethereum.eth.transactions.TransactionsMessageProcessor.processTransactionsMessage(TransactionsMessageProcessor.java:87)
	at org.hyperledger.besu.ethereum.eth.transactions.TransactionsMessageProcessor.processTransactionsMessage(TransactionsMessageProcessor.java:60)
	at org.hyperledger.besu.ethereum.eth.transactions.TransactionsMessageHandler.lambda$exec$0(TransactionsMessageHandler.java:51)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)

Versions

OS: Linux x86_64
Besu version: 23.4.1
Java: OpenJDK 17
Consensus: QBFT
Memory: 6GB, Max heap: 1.5GB

This was during a performance run invoking a simple storage contract at ~ 40 TPS.

[matthew1001]

In

besu/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/PendingTransactionsForSender.java

Line 115 in 857c9ac

public Stream<PendingTransaction> streamPendingTransactions() {

a stream is created of the pendingTransactions values. This is returned to the caller which streams/filters/collects the entries. Any update to PendingTransactionsForSender.pendingTransactions in that time will (I think) cause the ConcurrentModificationException

Because pendingTransactions is a java.util.TreeMap there's no obvious java.util.concurrent.* class to use instead.

[matthew1001]

Managed to capture the trace entry that gives the reason we were hitting this path:

2023-06-16 17:14:52.271+00:00 | vert.x-worker-thread-8 | TRACE | TransactionPool | rejecting transaction 0xa1cdb5dfacb961e533e0e183995bb5dae5d604c2766f38e86cec91407b219d24 due to chain head not available yet

[fab-10]

I will check you PR, and want to give you a bit of context here.
The signalInvalidAndGetDependentTransactions stuff was a patch added to try to limit the pollution of the txpool with transactions that could not be executed because they follow a transaction that is invalid, but that strategy is not optimal, and your case surface a scenario where the transaction probably is valid by itself, but the invalidation is due to the chain head not available.

There is a new implementation of the txpool (#5290) that is better than the current one, in all aspects and will replace the current implementation soon.
We are running it in our prod nodes without any issues for weeks now, and it will be great if you can try it.