getPayload interruption + debug timing logs #187
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sebastianst
force-pushed
the
seb/getpayload-interrupt-logging
branch
2 times, most recently
from
75f241a to
534f228
Compare
We only build the empty block if we don't use the tx pool. So if we use the tx pool, a forkchoiceUpdated call would miss the implicit validation that's happening during empty block building, so we need to add it back.
This commit changes the way the block builder/update routine and the resolution functions Resolve and ResolveFull synchronize. Resolve(Full) now signal the payload builder to pause and set the interrupt signal in case any block building is ongoing. They then wait for the interrupted block building to complete. This allowed to simplify the Payload implementation somewhat because the builder routine is now guaranteed to return before the resulting fields (full, fullFees etc) are read, and closing of the `stop` channel is now synchronized with a sync.Once. So the mutex and conditional variable could be removed and we only use two simple signalling channels `stop` and `done` for synchronization.
Some test in the miner and catalyst package assume that getPayload can be immediately called after forkchoiceUpdated and then to return some built block. Because of the new behavior of payload resolution to interrupt any ongoing payload building process, this creates a race condition on block building. The new testing mode, which can be enabled by setting the package variable IsPayloadBuildingTest to true, guarantees that always at least one full block is built. It's hacky, but seems to be the easiest and less-intrusive way to enable the new behavior of payload resolution while still keeping all tests happy.
- Priotize stop signal over recommit - Don't start payload building update if last update duration doesn't fit until slot timeout.
sebastianst
force-pushed
the
seb/getpayload-interrupt-logging
branch
from
534f228 to
cbd965d
Compare
When resolving, we don't want to wait for the latest update. If a full block is available, we just return that one, as before. Payload building is still interrupted, but exits in the background.
sebastianst
force-pushed
the
seb/getpayload-interrupt-logging
branch
2 times, most recently
from
5e38a3a to
2fb4bbe
Compare
sebastianst
force-pushed
the
seb/getpayload-interrupt-logging
branch
from
2fb4bbe to
30ad954
Compare
|
#186 was merged. Do we still need this logging? @sebastianst can we close this? |
|
Yes we can close, this was just used during testing and debugging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is #166 on top of #186 plus improved logging configs with env vars and thresholds.
Metadata
Part of https://github.com/ethereum-optimism/client-pod/issues/163