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

feat(proto): create v2 execution API #1962

Merged
merged 12 commits into from
Feb 26, 2025
Merged

Conversation

ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Feb 11, 2025

Summary

Creates v2 execution API to support upcoming v2 changes to Conductor and Geth.

Background

These changes are meant to support the upcoming v2 upgrades to Conductor and astria-geth, which will facilitate the ability to have rollup forks, driven by the need to migrate Forma to Mainnet.

Changes from v1

  • Removed GenesisInfo and associated RPC.
  • Removed BatchGetBlocks RPC and associated types.
  • Removed GetCommitmentState RPC.
  • Introduced ExecutionSession and associated RPC CreateExecutionSession:
    • An ExecutionSession is the execution client-side's current session, which starts at a specific block number and can end at one as well.
    • Upon startup and after each restart (if stop numbers are specified), CreateExecutionSession should be called, to which the server should respond with the current session, containing an ID for the session, the CommitmentState, and ExecutionSessionParams.
  • Added ExecutionSessionParams, which contains the necessary configuration for the execution client to run for the entirety of the session, consisting of the following:
    • rollup_id
    • rollup_start_block_number, the first block to be executed against the rollup
    • rollup_end_block_number, the final block to be executed against the rollup before ending the current execution session.
    • sequencer_chain_id
    • sequencer_start_block_height, the sequencer height to map to rollup_start_block_number.
    • celestia_chain_id
    • celestia_max_search_height_look_ahead (formerly celestia_block_variance)
  • Added session_id to ExecuteBlockRequest and UpdateCommitmentStateRequest, so that the server can validate the calls are for the correct execution session.
  • Changed Block to ExecutedBlockMetadata and made corresponding service changes.
  • Added ExecuteBlockResponse to follow AIP.
  • Changed CommitmentState.base_celestia_height to lowest_celestia_search_height
  • Represented all hashes as Strings so that they are displayed in whichever format the hashes are in, as opposed to Base64.
  • Separated all top-level proto entities into separate files (per 1-1-1 best practice)

Testing

No testing needed.

Changelogs

Protos have no changelog

@ethanoroshiba ethanoroshiba added the proto pertaining to the Astria Protobuf spec label Feb 11, 2025
astria.primitive.v1.RollupId rollup_id = 1;
// The first block height on the sequencer chain to use for rollup transactions.
// This is mapped to `rollup_first_block_number`.
uint32 sequencer_first_block_height = 2;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know we did away with sequencer_start_block_height in favor of sequencer_start_height before, but somehow sequencer_first_height feels like it carries less information than sequencer_first_block_height to me. Happy to change, though

@ethanoroshiba ethanoroshiba marked this pull request as ready for review February 11, 2025 19:13
@ethanoroshiba ethanoroshiba requested review from a team and Fraser999 as code owners February 11, 2025 19:13
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

Left some notes for discussion on the need for rollup_halt_at_stop_number.

Going down this rabbit hole - I wonder if we can avoid setting rollup_stop_block_number entirely in favor of using gRPC error codes?

For example, OUT_OF_RANGE or RESOURCE_EXHAUSTED might be valid error codes that can trigger conductor to re-fetch the genesis and/or commitment states.

Copy link
Member

@joroshiba joroshiba left a comment

Choose a reason for hiding this comment

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

Approved, but noting there is one extra file here were we should delete.

Either as a part of this PR or as follow up can we get a execution api v2 spec doc?

@ethanoroshiba
Copy link
Contributor Author

Either as a part of this PR or as follow up can we get a execution api v2 spec doc?

Noting that I have made a followup issue here for the spec: #1986. Prefer to get this landed ASAP

Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

This looks good overall.

I only have a bunch of naming and field ordering requests (important to get this right because future changes would be breaking).

// Celestia blobs).
string celestia_chain_id = 6;
// The allowed variance in celestia for sequencer blocks to have been posted.
uint64 celestia_block_variance = 7;
Copy link
Member

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.

Co-authored-by: Superfluffy <[email protected]>
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

I have some thoughts on the block fields in CommitmentState. I think we should define bespoke SoftRollupBlock and FirmRollupBlock (or alternatively, SoftCommitmentState and FirmCommitmentState, which we will populate from the executed RollupBlock conductor side).

Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

Left some comments on the response types of the RPCs. We are sometimes relatively strict about naming these things (for example in the auctioneer protos), but here we are loser.

Should we enforce a *Request -> *Response convention everywhere (where for example GetBlock responds with a GetBlockResponse { rollup_block: RollupBlock } (so that the block is just a field of the response)?

Copy link
Member

@joroshiba joroshiba left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

We need to fix the type of the ExecuteBlock.prev_block_hash request to match the ExecutedBlockMetadata.

@ethanoroshiba ethanoroshiba added this pull request to the merge queue Feb 26, 2025
Merged via the queue into main with commit 94e8d14 Feb 26, 2025
48 checks passed
@ethanoroshiba ethanoroshiba deleted the eoroshiba/v2-execution-api branch February 26, 2025 17:50
@ethanoroshiba ethanoroshiba linked an issue Feb 26, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proto pertaining to the Astria Protobuf spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create V2 Execution API to support Forma migration
4 participants