fix: dump command #186
Closed
fix: dump command #186
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
|
Thank you for your contribution. We'll leave it for now and merge the upstream fix ethereum-optimism/op-geth@28d5521 to reduce further conflict. |
|
If you do need to use the dump command now, feel free to use the this fixed version. |
bnoieh
pushed a commit
to bnoieh/op-geth
that referenced
this pull request
Nov 29, 2024
…oad) (bnb-chain#186) * miner: Add block building interruption on payload resolution (getPayload) * miner: Change full payload resolution, fix and add test * miner: Add parameter validation if skipping empty block 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. * miner: Always wait for block builder result after interrupting 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. * miner: Add testing mode to module 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. * miner: Further improve block building interruption - Priotize stop signal over recommit - Don't start payload building update if last update duration doesn't fit until slot timeout. * miner: Partially revert rework of payload build stopping 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. * miner: Return early when building interrupted payload updates * Remove global variable to change miner behaviour. Use a longer wait in tests for the payload to build. * miner: Interrupt first payload building job Also added interrupt test. Had to add sleep to make non-interrupt test work. * eth/catalyst: Add even more sleeps to make tests get over payload interruption * Deterministically wait for payloads to build the first full block * eth/catalyst,miner: Improve payload full block waiting in tests Also fix a bug in TestNilWithdrawals where the withdrawals weren't added to the ephemeral BuildPayloadArgs instance for re-calculating the payload id. * miner: Calculate sane block building time in validateParams Also always stop interrupt timer after fillTransactions in generateWork. --------- Co-authored-by: Adrian Sutton <adrian@oplabs.co>
bnoieh
pushed a commit
to bnoieh/op-geth
that referenced
this pull request
Nov 29, 2024
…oad) (bnb-chain#186) * miner: Add block building interruption on payload resolution (getPayload) * miner: Change full payload resolution, fix and add test * miner: Add parameter validation if skipping empty block 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. * miner: Always wait for block builder result after interrupting 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. * miner: Add testing mode to module 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. * miner: Further improve block building interruption - Priotize stop signal over recommit - Don't start payload building update if last update duration doesn't fit until slot timeout. * miner: Partially revert rework of payload build stopping 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. * miner: Return early when building interrupted payload updates * Remove global variable to change miner behaviour. Use a longer wait in tests for the payload to build. * miner: Interrupt first payload building job Also added interrupt test. Had to add sleep to make non-interrupt test work. * eth/catalyst: Add even more sleeps to make tests get over payload interruption * Deterministically wait for payloads to build the first full block * eth/catalyst,miner: Improve payload full block waiting in tests Also fix a bug in TestNilWithdrawals where the withdrawals weren't added to the ephemeral BuildPayloadArgs instance for re-calculating the payload id. * miner: Calculate sane block building time in validateParams Also always stop interrupt timer after fillTransactions in generateWork. --------- Co-authored-by: Adrian Sutton <adrian@oplabs.co>
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
Currently
dumpcommand would close db after creating it immediatelyRationale
dumpcommand is not working