NullPointerException when fast syncing QBFT chain

[matthew1001]

Description

2023-09-06 09:09:07.295+01:00 | BftProcessorExecutor-QBFT-0 | ERROR | EventMultiplexer | State machine threw exception while processing event \{BlockTimerExpiry{Round Identifier=ConsensusRoundIdentifier{Sequence=378, Round=0}}\}
java.lang.IllegalStateException: Block creation failed unexpectedly. Will restart on next block added to chain.
        at org.hyperledger.besu.ethereum.blockcreation.AbstractBlockCreator.createBlock(AbstractBlockCreator.java:292)
        at org.hyperledger.besu.ethereum.blockcreation.AbstractBlockCreator.createBlock(AbstractBlockCreator.java:156)
        at org.hyperledger.besu.ethereum.blockcreation.AbstractBlockCreator.createBlock(AbstractBlockCreator.java:142)
        at org.hyperledger.besu.consensus.qbft.statemachine.QbftRound.createAndSendProposalMessage(QbftRound.java:130)
        at org.hyperledger.besu.consensus.qbft.statemachine.QbftBlockHeightManager.handleBlockTimerExpiry(QbftBlockHeightManager.java:136)
        at org.hyperledger.besu.consensus.common.bft.statemachine.BaseBftController.handleBlockTimerExpiry(BaseBftController.java:167)
        at org.hyperledger.besu.consensus.common.bft.EventMultiplexer.handleBftEvent(EventMultiplexer.java:65)
        at java.base/java.util.Optional.ifPresent(Optional.java:178)
        at org.hyperledger.besu.consensus.common.bft.BftProcessor.run(BftProcessor.java:65)
        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)
Caused by: java.lang.NullPointerException: Cannot invoke "org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.selectTransactions(org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions$TransactionSelector)" because "this.pendingTransactions" is null
        at org.hyperledger.besu.ethereum.eth.transactions.TransactionPool.selectTransactions(TransactionPool.java:526)
        at org.hyperledger.besu.ethereum.blockcreation.BlockTransactionSelector.buildTransactionListForBlock(BlockTransactionSelector.java:274)
        at org.hyperledger.besu.ethereum.blockcreation.AbstractBlockCreator.selectTransactions(AbstractBlockCreator.java:362)
        at org.hyperledger.besu.ethereum.blockcreation.AbstractBlockCreator.createBlock(AbstractBlockCreator.java:200)
        ... 11 more

Steps to Reproduce (Bug)

1. Start a 5 node QBFT chain
2. Stop one of the nodes and delete its database
3. Restart the node with --sync-mode=FAST --fast-sync-min-peers=4 (the min peers is needed to avoid the min number of 5 before syncing starts)

Expected behavior: The node syncs with the chain and then resumes proposing & validating blocks (since it was before it was stopped and its data deleted)

Actual behavior: NPE ^^

[matthew1001]

I've tested a code fix that simply checks for null in TransactionPool.selectTransactions() and the sync completes successfully, and the node starts validating and proposing blocks in sync with the rest of the chain.

However, this feels like it might be glossing over a more structural change that is needed, and that we shouldn't be reaching TransactionPool.selectTransactions() if the node is still syncing.

I don't have enough experience of the MiningCoordinator to know if it should not be starting (or at least not starting fully) while the node is syncing, much like the transaction pool behaviour today. In that case, the fix would be to have the mining coordinator call SyncState.subscribeCompletionReached() and register a callback to start the mining coordinator fully once the node has synced.

Alternatively a middle ground might be to have the mining coordinator start is it does today, but possible only handle certain BFT events once syncing is complete. For example it could ignore BLOCK_TIMER_EXPIRY events until syncing is complete.

I'd benefit on others input as to the most appropriate fix.

[matthew1001]

Started looking at this under #5861, taking the approach of not starting a BFT mining coordinator until the node is sync'd.

[non-fungible-nelson]

Need any assistance let us know