Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deadlock in 'SimulatedBeacon.loop' #29476

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nikitagashkov
Copy link

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.

@MariusVanDerWijden
Copy link
Member

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

Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nikitagashkov
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transactions stuck in pending in dev-mode
3 participants