go-ethereum icon indicating copy to clipboard operation
go-ethereum copied to clipboard

Fix deadlock in 'SimulatedBeacon.loop'

Open nikitagashkov opened this issue 10 months ago • 2 comments

Fixes #29475.

Right now there's a possibility of a deadlock between TxPool and SimulatedBeacon waiting each other (see issue for details), this MR tries to overcome it by scheduling block generation to background upon request from TxPool (<-newTxs).

There's one more case to fix (<-a.sim.withdrawals.pending) but it's a bit more tricky since we need to buffer withdrawals before submission.

For now, this MR fixes the simplest case, if the solution is acceptable, I'm willing to extend it.

nikitagashkov avatar Apr 06 '24 19:04 nikitagashkov

Hmm I don't get how this addresses the deadlock. I think the core issue is that we are using the engine from the outside, so we have no control over which transactions are mined into the block. We could add some mechanism to periodically check the txpool to see if there are pending transactions there and trigger a commit then, but that is kinda jank imo

MariusVanDerWijden avatar Apr 08 '24 09:04 MariusVanDerWijden

Hmm I don't get how this addresses the deadlock. I think the core issue is that we are using the engine from the outside, so we have no control over which transactions are mined into the block.

@MariusVanDerWijden, my idea is to make <-newTxs and <-a.sim.withdrawals.pending cases non-blocking, so that we can utilize them as triggers to start creating yet another block with whatever's currently in the pool. This will allow us to continue reacting to newTxs if something gets added into the pool while we're busy building the block

Currently, SimulatedBeacon waits for the TxPool to reset before committing (introduced in #28837). However (upon deadlock), the TxPool itself waits for SimulatedBeacon to become ready to accept newTxs. It looks like we must continue waiting for the pool to sync before sealing the block to reliably produce blocks without leaving queued TXs behind

We could add some mechanism to periodically check the txpool to see if there are pending transactions there and trigger a commit then, but that is kinda jank imo

I also prefer the current approach with on-demand sealing

This needs some changes. If we start committing in a bg goroutine, we should also do the sealBlock for withdrawals in the background.

@fjl, yep, I agree, there's a FIXME for the sealBlock above, I just want to sync on the approach before the implementation

nikitagashkov avatar Apr 11 '24 20:04 nikitagashkov

Related: #29657

fjl avatar Jun 25 '24 13:06 fjl

We're continuing the work in #29657 instead.

fjl avatar Jun 25 '24 13:06 fjl