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

eth/catalyst: ensure period zero mode leaves no pending txs in pool #30264

Merged
merged 10 commits into from
Aug 19, 2024

Conversation

lightclient
Copy link
Member

@lightclient lightclient commented Aug 4, 2024

closes #29475, replaces #29657, #30104

The original problem noted in #29475 was a deadlock where runReorg was trying to write to newTxs subscription, but the reader was already waiting for runReorg to finish in a call to txpool.Sync(). This PR solves that issue by creating a worker thread in the simulated beacon API which allows the API's main event handler to never block. Instead, when a new tx or withdrawal event is received it notifies the worker thread to begin constructing a block. It uses a buffered channel to ensure that while the worker is building a block, additional requests to construct a block are not dropped nor blocked on.

Although this was also solved by spawning a new goroutine on each newTxs event #29657, an additional issue around handling txs was noticed. It's possible for a newTxs event to have more txs than can be included in a single block. Because we only called Commit once, these txs would be left dangling in the pool. This PR also fixes this issue by calling txpool.Sync() to ensure the reorg has happened to account for the recently produced block, followed by txpool.Stats() to check if there are any remaining executable txs.

There is a concern here is that the relationship between the miner determining if the pending txs can be included and the result of txpool.Stats() is sort of weak. It's possible that if those get out-of-whack the simulated beacon api could go into an infinite loop of block production. Since the miner only checks if txs can 1) pay the appropriate miner tip, 2) afford the base fee, and 3) fit within the gas limit. Currently I can't see how this would happen unless maybe a local tx submits a base below the theoretical floor (7 wei). Open to suggestions on improving that.

The PR #30104 did not have this issue, because it would build blocks until it got an empty block. Originally I felt this was rather crude and polluted the separation of sealBlock with the concept of allowEmpty. It still feels hacky to me, but given the complexity of the subsystems (miner, txpool, sim), it may actually be the cleanest way to ensure we get the desired outcome. One scenario the current PR can handle that #30104 cannot though is if there are pending txs which cannot pay the base fee due to a previous iteration pushing the base fee too high. Th

--

I also tacked on a commit to move the call for txpool.Sync() out of forkchoiceUpdated and into the simulated beacon. @karalabe originally added it into fcu, would be curious to know if there was a reason for adding there versus in the simulator. By adding it to the simulator we can simplify the parameters for forkchoiceUpdated slightly.

If it seems better to go down the path set by #30104 I can adjust this PR to just check the resulting block's tx count and stop the loop once it receives an empty block. I still think there are some worth while renamings / improvements in this PR.

Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

nitpicks, otherwise sgtm!

eth/catalyst/simulated_beacon.go Outdated Show resolved Hide resolved
eth/catalyst/simulated_beacon.go Outdated Show resolved Hide resolved
eth/catalyst/simulated_beacon.go Outdated Show resolved Hide resolved
eth/catalyst/simulated_beacon_api.go Outdated Show resolved Hide resolved
eth/catalyst/simulated_beacon.go Outdated Show resolved Hide resolved
@lightclient
Copy link
Member Author

PR feedback addressed, PTAL @rjl493456442.

Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

sgtm

@rjl493456442 rjl493456442 added this to the 1.14.9 milestone Aug 13, 2024
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@lightclient
Copy link
Member Author

Added a comment about potential spin loop. LGTM though.

@lightclient lightclient merged commit 84565dc into ethereum:master Aug 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transactions stuck in pending in dev-mode
5 participants