-
Notifications
You must be signed in to change notification settings - Fork 763
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
miner,eth: latest block as pending block on rollup #93
Conversation
6ab0387
to
ed39d5d
Compare
ed39d5d
to
fadc776
Compare
do not merge until we figure out test-failures in ethereum-optimism/optimism#5736 |
fadc776
to
8cc6df7
Compare
Figured out the issue: the producing-side of the chain-heads subscription started blocking the forkchoice API. There needs to be at least one subscriber, which is the miner apparently. So instead I now implemented it to drain the produced triggers, without creating pending-block computation work, as well as ignoring incoming news txs, rather than unsubscribing to the tx events. Monorepo e2e tests should pass now. |
8cc6df7
to
8a2aa07
Compare
cleaned up the diff a little bit more. Ready for review now |
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.
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. Can you back port it to a new branch based off of release/v1.101105
?
@trianglesphere I've created a branch with this change and #95 back ported onto We probably should have a better name for that branch but I wasn't sure what pattern we used so shoved it under I think we should also get both PRs merged into |
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.
The flag was not included in the list of flags. Failed to start b/c of this.
Line 160 in 6fe4241
}, utils.NetworkFlags, utils.DatabasePathFlags) |
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
Description
This fixes the pending-block behavior to be more compatible with pre-bedrock assumptions: requests for the "pending" block/header get served the "latest" block. This prevents transactions from the tx-pool from being revealed before they are committed to by the sequencer
Additionally this updates other lesser used API endpoints that utilize the "pending" mining state as well:
StateAndHeaderByNumberOrHash
) continues processing as "latest" state when the pending state is not available, like in the rollup case.And to prevent unnecessary pending-block computations, the worker in the
miner
package is changed to:buildPayload
, and thus we can disable the pending-block work-triggering parts by draining the events that create those, rather than processing them (innewWorkLoop
). Draining is important, as unsubscribing would leave the producing subscription channels filling up, causing the engine API to hang after producing blocks.The engine API will still be initiating tasks for block-building, but does so by merely using the block-building functionality of the worker, not the whole recommit and snapshot thing that still remains from L1 PoW days / pending-block support.
I also suspect that this improves sequencing performance, or at least reduces block-building lag spikes: rebuilding the pending block continuously on the head block which changes every 2 seconds, and re-applying transactions, is expensive. Previously this was worked-around on the sequencer node by setting the pending-block gas limit to be very low (5000 gas to effectively force the pending-blocks to be empty), but now it doesn't try to build it at all anymore, which means there is less contention around the state of the chain and the tx-pool.
Tests
ethereum-optimism/optimism#5736
Replaces #81
Fix CLI-3793