go-ethereum
go-ethereum copied to clipboard
Fix deadlock in 'SimulatedBeacon.loop'
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.
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
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
Related: #29657
We're continuing the work in #29657 instead.