Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(proto): create v2 execution API #1962
feat(proto): create v2 execution API #1962
Changes from 6 commits
f7813b2
42457e9
746e455
7d47c50
b0fe683
2ac64af
1f3d1c5
ebeaed9
0c2992d
738199a
60fd068
25c741f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
This field is confusing. How should this work in practice?
Is this intended to interact with the celestia block variance that we have define in the execution config?
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.
This field is the first height that Conductor begins fetching Celestia data at. I think we could clarify this, but additionally, it seems a bit pointless to include in
CommitmentState
I think? It should stay the same for an entire execution session, so I think maybe we should move it thereThere 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.
it doesn't stay the same. it's part of the commitment state because afaik, conductor updates it when updating the commitment state. If it did not then if the rollup restarted, conductor would have to start searching from the initial base celestia height. If the rollup has been running for awhile, this could be months of data it doesn't need to look through
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.
Ah I see, you're right. Perhaps
base_celestia_height
is a bit of a misnomer then? Perhaps something likecurrent_celestia_height
,latest_celestia_height
, or justcelestia_height
?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.
We could at best do something like
firm_found_at_celestia_height
.It's important to remember that we don't even have a guarantee that if the sequencer height S gets included at Celestia height C, that then S+1 gets included at C' >= C.
We have this guarantee right now in practice due to how we run the relayer, but for distributed relayer designs we might end up with another scenario (delays, latencies, network issues with resubmission attempts).
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 feel like
latest_celestia_height
could reflect that it represents the latest height that a firm block has been found at, but maybe it's too confusing. I personally thinkfirm_found_at_celestia_height
sounds a bit more confusing, perhaps we just leave as is and update the descriptionThere 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 think if
firm_found_at_celestia_height
means the firm in this commitment state was found at this celestia height, then the meaning is pretty clear. base, current, latest, etc are all a bit more vague.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.
but in that case I think
celestia_height
carries the same meaning and is more conciseThere 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.
Changed to
celestia_height
in 0c2992d and added a more detailed description of how this is updated and why.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.
@joroshiba I think we should define a fixed variance here and not perform a calculation inside conductor. Right now it's 6x this value, but I don't see a reason to not just set it here without extra conductor shenanigans.