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

Add engine_signalSuperchainV1 #7693

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

emlautarom1
Copy link
Contributor

@emlautarom1 emlautarom1 commented Oct 31, 2024

Fixes #7658

Changes

  • Add method engine_signalSuperchainV1

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

Notes on testing

It might require additional testing using an actual node.

Documentation

Requires documentation update

  • Yes
  • No

Optimism engine API must be updated to include the new method.

Requires explanation in Release Notes

  • Yes
  • No

Remarks

The current implementation only performs logging in case of being behind the recommended/required versions. We should discuss if we want to add some opt-in halting mechanism as suggested by the specification.

The build field of the OptimismProtocolVersion.V0 is filled with NETH since we only have 8 bytes available. While this field is ignored when determining version precedence (as the specification demands), the op-geth implementation actually fails comparisons when builds are not the same.

Comment on lines 71 to 78
if (currentVersion < signal.Recommended)
{
await _signalSuperchainHandler.OnBehindRecommended(signal.Recommended);
}
if (currentVersion < signal.Required)
{
await _signalSuperchainHandler.OnBehindRequired(signal.Required);
}
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why those are needed? Only to write a log? Seems overblown to do task and await it just for that?

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to pass signal to signalSuperchainHandler and that's it? It should check the versions and do what is needed.

Copy link
Contributor Author

@emlautarom1 emlautarom1 Nov 1, 2024

Choose a reason for hiding this comment

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

Seems overblown to do task and await it just for that?

Logging warnings/errors is just one possible implementation and the interface is designed for possible async implementations.

As the specification says, we might want to add the option to allow the user to trigger a shutdown when the current version is behind the required one.

Wouldn't it be better to pass signal to signalSuperchainHandler and that's it?

We could but then all implementations would need to perform the < checks so I decided to move that branching logic to the OptimismEngineRpcModule class itself, making implementations only care about what to do when behind recommended/required.

Copy link
Member

Choose a reason for hiding this comment

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

you can encapsulate those checks in some method (extension method on signal for example), and return some enum (can be flags). Then implementations would only check that enum and act on it.

Comment on lines 25 to 27
OptimismProtocolVersion CurrentVersion { get; }
Task OnBehindRecommended(OptimismProtocolVersion recommended);
Task OnBehindRequired(OptimismProtocolVersion required);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
OptimismProtocolVersion CurrentVersion { get; }
Task OnBehindRecommended(OptimismProtocolVersion recommended);
Task OnBehindRequired(OptimismProtocolVersion required);
OptimismProtocolVersion Signal(OptimismSuperchainSignal signal);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement engine_signalSuperchainV1 for Optimism
2 participants