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

Implement engine_exchangeCapabilities method #5212

Merged
merged 8 commits into from
Jan 27, 2023

Conversation

rubo
Copy link
Contributor

@rubo rubo commented Jan 25, 2023

Changes

  • Implemented engine_exchangeCapabilities method as specified in the spec
  • In addition, the execution client warns if it has more methods (considers only the activated ones on its end) than found in the consensus client.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

@rubo rubo force-pushed the feature/engine_getcapabilities branch from aec4e8b to c20eaa7 Compare January 25, 2023 21:32
@rubo rubo marked this pull request as ready for review January 25, 2023 21:33
@rubo rubo requested review from LukaszRozmej and MarekM25 and removed request for LukaszRozmej January 25, 2023 21:33
@rubo rubo added the rpc label Jan 25, 2023
Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no caching

@rubo rubo marked this pull request as draft January 26, 2023 09:10
@rubo
Copy link
Contributor Author

rubo commented Jan 26, 2023

no caching

Take a closer look.

MarekM25

This comment was marked as resolved.

@MarekM25 MarekM25 self-requested a review January 26, 2023 09:14
@MarekM25 MarekM25 self-requested a review January 26, 2023 09:30
@rubo rubo changed the title Implement engine_getCapabilities method Implement engine_exchangeCapabilities method Jan 26, 2023
Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also print warnings when our exchange capabilities are not the same.

@rubo rubo marked this pull request as ready for review January 26, 2023 11:08
@rubo rubo force-pushed the feature/engine_getcapabilities branch from b459446 to d0e0f01 Compare January 26, 2023 13:01
Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still a few things to address

src/Nethermind/Nethermind.Merge.Plugin/EngineRpcModule.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/EngineRpcModule.cs Outdated Show resolved Hide resolved

public IBlockProcessingQueue BlockProcessingQueue { get; set; } = null!;
public IBlockTree BlockTree { get; set; } = null!;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was changed in TestBlockchain?

Copy link
Contributor Author

@rubo rubo Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added mutator for LogManager, line 260.

@rubo rubo marked this pull request as draft January 26, 2023 23:13
@rubo rubo requested a review from MarekM25 January 26, 2023 23:20
@rubo rubo marked this pull request as ready for review January 27, 2023 10:47
{
watch.Stop();

Metrics.ForkchoiceUpdedExecutionTime = watch.ElapsedMilliseconds;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are changing the metrics, please remove

_capabilities[nameof(IEngineRpcModule.engine_exchangeTransitionConfigurationV1)] = true;
_capabilities[nameof(IEngineRpcModule.engine_executionStatus)] = true;
_capabilities[nameof(IEngineRpcModule.engine_forkchoiceUpdatedV1)] = true;
_capabilities[nameof(IEngineRpcModule.engine_forkchoiceUpdatedV2)] = spec.WithdrawalsEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you think that some list of the methods will be better?

@rubo rubo force-pushed the feature/engine_getcapabilities branch from c3353a5 to dcc4d73 Compare January 27, 2023 13:35
@rubo rubo merged commit a9cb737 into master Jan 27, 2023
@rubo rubo deleted the feature/engine_getcapabilities branch January 27, 2023 13:51
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.

3 participants