Skip to content
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

Merged
merged 7 commits into from
Jul 31, 2022

Conversation

garyschulte
Copy link
Contributor

@garyschulte garyschulte commented Jul 28, 2022

PR description

Allow besu to have the engine api enabled, when there are not merge configs. includes:

  • un-hiding/un-deprecating --engine-rpc-enabled
  • remove old isMergeEnabled checks in a few block header validators
  • use merge-specific block header validator, rather than appending to MainnetBlockHeaderValidator
  • disable all engine api methods except engine_exchangeTransitionConfigurationV1 in the absence of an engine-api-compatible coordinator
  • add "placeholder" TTD in the absence of a TTD configuration

Tested with Hive and Kurtosis since this affects block validation. Both are 🟢

Fixed Issue(s)

fixes #4172

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

@garyschulte garyschulte marked this pull request as ready for review July 28, 2022 05:26
@garyschulte garyschulte marked this pull request as draft July 28, 2022 05:28
@daniellehrner
Copy link
Contributor

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

@garyschulte garyschulte marked this pull request as ready for review July 28, 2022 21:00
@garyschulte garyschulte force-pushed the feature/engine-api-override branch 2 times, most recently from f121010 to 9810255 Compare July 28, 2022 22:58
@garyschulte garyschulte force-pushed the feature/engine-api-override branch 3 times, most recently from 2963642 to 6a6e67c Compare July 29, 2022 13:59
* 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>
Copy link
Contributor

@jflo jflo left a 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()) {
Copy link
Contributor

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?

Copy link
Contributor Author

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 👀

Copy link
Contributor Author

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>
@garyschulte garyschulte enabled auto-merge (squash) July 31, 2022 03:34
@garyschulte garyschulte merged commit 826648b into hyperledger:main Jul 31, 2022
@garyschulte garyschulte deleted the feature/engine-api-override branch July 31, 2022 13:17
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement override to force engine-api in the absence of TTD
4 participants