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

Execution service 4: execution->consensus interface #1535

Merged
merged 50 commits into from
Mar 26, 2024

Conversation

tsahee
Copy link
Contributor

@tsahee tsahee commented Mar 24, 2023

execution now only makes requests for consensus client using a reasonbly-defined API (not yet asyncronous)
This PR mostly deals with node_interface and sync_monitor.
These two created heavy dependency of execution node in inner-consensus details.
Also classic-messages moved to execution for API simplicity.

Part of NIT-1262

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Mar 24, 2023
Base automatically changed from execution-service-3 to master October 4, 2023 17:17
@@ -213,6 +218,44 @@ func (t *InboxTracker) GetBatchCount() (uint64, error) {
return count, nil
}

func (t *InboxTracker) FindL1BatchForMessage(pos arbutil.MessageIndex) (uint64, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

used to be static_block_validator: FindBatchContainingMessageIndex

execution/nodeInterface/NodeInterface.go Show resolved Hide resolved
execution/gethexec/node.go Outdated Show resolved Hide resolved
execution/nodeInterface/NodeInterface.go Outdated Show resolved Hide resolved
execution/nodeInterface/NodeInterface.go Show resolved Hide resolved
return 0, false, err
}
if lastBatchMessageCount <= pos {
return 0, false, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this is the right decision, but we could consider returning a special error in this case instead of having the bool return. If we stick with the bool return, we should add a comment explaining what it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specialized errors are fickle, especially in interfaces that will later be over rpc. I think a boolean makes sense here.
Added documentation. LMKWYT.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. I don't see the documentation there, maybe it didn't get pushed up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

d'oh!

system_tests/nodeinterface_test.go Outdated Show resolved Hide resolved
system_tests/nodeinterface_test.go Outdated Show resolved Hide resolved
execution/gethexec/sync_monitor.go Outdated Show resolved Hide resolved
execution/gethexec/sync_monitor.go Show resolved Hide resolved
Copy link
Collaborator

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

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

LGTM aside from one outstanding comment

Copy link
Collaborator

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

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

LGTM

@PlasmaPower PlasmaPower enabled auto-merge March 26, 2024 20:47
@PlasmaPower PlasmaPower merged commit bf01c86 into master Mar 26, 2024
8 checks passed
@PlasmaPower PlasmaPower deleted the execution-service-4 branch March 26, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants