-
Notifications
You must be signed in to change notification settings - Fork 20k
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
eth/catalyst: ensure period zero mode leaves no pending txs in pool #30264
Conversation
8c8644f
to
1eb342e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicks, otherwise sgtm!
PR feedback addressed, PTAL @rjl493456442. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Added a comment about potential spin loop. LGTM though. |
closes #29475, replaces #29657, #30104
The original problem noted in #29475 was a deadlock where
runReorg
was trying to write tonewTxs
subscription, but the reader was already waiting forrunReorg
to finish in a call totxpool.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 anewTxs
event to have more txs than can be included in a single block. Because we only calledCommit
once, these txs would be left dangling in the pool. This PR also fixes this issue by callingtxpool.Sync()
to ensure the reorg has happened to account for the recently produced block, followed bytxpool.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 ofallowEmpty
. 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 offorkchoiceUpdated
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 forforkchoiceUpdated
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.