-
Notifications
You must be signed in to change notification settings - Fork 897
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
Feature/engine api override #4190
Feature/engine api override #4190
Conversation
Just today there was a discussion on Discord that revealed that all CL clients already use a placeholder TTD for mainnet. So I guess the easiest would be to just use that one instead: https://discord.com/channels/595666850260713488/892088344438255616/1002018160268038164 |
d86e2c6
to
e8cb0c5
Compare
f121010
to
9810255
Compare
2963642
to
6a6e67c
Compare
* remove reliance on isMergeEnabled in pre-merge block header validation rules * create a merge-specific block header validtion stack rather than leveraging mainnet * re-add --engine-api-enabled cli param Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
leaves the door open to supporting the engine api with other MiningCoordinators, but disables all but engine_exchangeTransitionConfiguration in the absence of mergeCoordinator Signed-off-by: garyschulte <garyschulte@gmail.com>
…ation Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
6a6e67c
to
922fc18
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.
One thing that i found hard to parse, but otherwise this cleans up a ton of conditionals and class casting.
@@ -166,10 +166,10 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) | |||
final var block = | |||
new Block(newBlockHeader, new BlockBody(transactions, Collections.emptyList())); | |||
|
|||
if (mergeContext.isSyncing() || parentHeader.isEmpty()) { | |||
if (mergeContext.map(c -> c.isSyncing()).orElse(Boolean.TRUE) || parentHeader.isEmpty()) { |
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.
This is kinda hard to read. It looks like it evaluates to a no-op, does anyone else find this harder to read?
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.
I can see how it is harder to read, it is a side effect of making mergeContext an Optional. looking for an approach that is easier on the 👀
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.
Supplier/function reference ftw 👍
…ad in engine-api Signed-off-by: garyschulte <garyschulte@gmail.com>
* initial impl of engine-api override: * remove reliance on isMergeEnabled in pre-merge block header validation rules * create a merge-specific block header validtion stack rather than leveraging mainnet * re-add --engine-api-enabled cli param * make engine api usable without a merge config. leaves the door open to supporting the engine api with other MiningCoordinators, but disables all but engine_exchangeTransitionConfiguration in the absence of mergeCoordinator Signed-off-by: garyschulte <garyschulte@gmail.com>
PR description
Allow besu to have the engine api enabled, when there are not merge configs. includes:
--engine-rpc-enabled
Tested with Hive and Kurtosis since this affects block validation. Both are 🟢
Fixed Issue(s)
fixes #4172
Documentation
doc-change-required
label to this PR ifupdates are required.
Changelog