-
Notifications
You must be signed in to change notification settings - Fork 468
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
Conversation
These tests often fail in CI because the chain doesn't move fast enough. Doing it serially should help.
DelayedMaxMessageCount is confusing with delayed inbox messages
arbnode/inbox_tracker.go
Outdated
@@ -213,6 +218,44 @@ func (t *InboxTracker) GetBatchCount() (uint64, error) { | |||
return count, nil | |||
} | |||
|
|||
func (t *InboxTracker) FindL1BatchForMessage(pos arbutil.MessageIndex) (uint64, error) { |
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.
used to be static_block_validator: FindBatchContainingMessageIndex
This reverts commit 7e82acf.
return 0, false, err | ||
} | ||
if lastBatchMessageCount <= pos { | ||
return 0, false, nil |
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'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.
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.
Specialized errors are fickle, especially in interfaces that will later be over rpc. I think a boolean makes sense here.
Added documentation. LMKWYT.
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.
Makes sense. I don't see the documentation there, maybe it didn't get pushed up?
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.
d'oh!
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.
LGTM aside from one outstanding comment
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.
LGTM
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