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): add bundle and optimistic block apis #1519

Merged
merged 5 commits into from
Oct 2, 2024

Conversation

itamarreif
Copy link
Contributor

@itamarreif itamarreif commented Sep 18, 2024

Summary

This adds the protobuf definitions for optimistic block stream and the bundle stream.

Background

The Auctioneer will auction off a bundle slot which will be placed at the top of the rollup block deterministically during execution. The APIs in this PR are used to drive the bundle auction (detailed in this document). At a high level:

The Auctioneer receives blocks optimistically from the sequencer in order to maximize the auction duration.

While the auction is running, the Auctioneer receives orders from the rollup node that have been validated against the optimistic block.

After a block is committed, the Auctioneer will submit the highest paying bundle for inclusion by the sequencer.

Optimistic Block Stream

  1. The Sequencer will stream blocks to the Auctioneer optimistically, i.e. before they are finalized (ProcessProposal).
  2. The Sequencer will also stream block commitments to the Auctioneer, which mark blocks as final thanks to CometBFT's single slot finality.

Bundle Stream

  1. The Auctioneer will stream optimistic blocks to the rollup node for execution. The rollup will execute the block and return the resulting header info.
  2. The rollup node will stream bundles (indexed by parent block hash) to the Auctioneer.

Changes

  • Add the bundle service to the composer apis
  • Add the optimistic block service to the sequencer block apis

Breaking Changelist

  • Bulleted list of breaking changes, any notes on migration. Delete section if none.

Related Issues

closes #1553

// This is the block hash for the proposed block.
bytes sequencer_block_hash = 1;
// The hash of previous rollup block, which new block will be created on top of.
bytes prev_rollup_block_hash = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we assume that this will be the latest soft block of the rollup? Because I believe we would have to query the soft block from auctioneer via the GetCommitmentState method which might add to latency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can probably remove this? what is it used for

// Sequencer block.
bytes base_sequencer_block_hash = 3;
// The hash of previous rollup block, on top of which the bundle will be executed as ToB.
bytes prev_rollup_block_hash = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure what Auctioneer would do with this information

Copy link
Contributor

Choose a reason for hiding this comment

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

Auctioneer would require it to build the bundle to send to the sequencer. When conductor is applying this bundle, we need to make sure that we are applying on the correct parent block.

Comment on lines 15 to 19
// The fee that can be expected to be received for submitting this bundle.
// This allows the bundle producer to stream any confirmed bundles they would be ok
// with submitting. Used to avoid race conditions in received bundle packets. Could
// also be used by a bundle submitter to allow multiple entities to submit bundles.
uint64 fee = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

how is this verified? also what token is this in (rollup or sequencer)? i'm guessing rollup?

Copy link
Contributor

Choose a reason for hiding this comment

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

this would be in rollup token and since we are running the auctioneer, we should be able to assume that this information passed between geth and the composer can be trusted, similar to how we can assume the information between a geth node and a conductor can be trusted.

Copy link
Contributor

Choose a reason for hiding this comment

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

the fee will mainly be used to maintain the highest paying transaction seen so far in the auctioneer.

Copy link
Contributor

Choose a reason for hiding this comment

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

another way would be for auctioneer to de-serialize the EVM transaction which would be more trustless but would be higher latency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this would be in rollup token and since we are running the auctioneer, we should be able to assume that this information passed between geth and the composer can be trusted, similar to how we can assume the information between a geth node and a conductor can be trusted.

you mean passed between geth and auctioneer? so the fee is parsed by geth from the txs and verified in geth, then set to auctioneer essentially?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah.

transactions are submitted to geth via the mempool rpc. when the geth mempool promotes transactions to pending they are checked for nonce and sufficient balance for the fee. geth provides a subscription for pending transactions (similar to what we do in composer) where promoted txs are sent to.
the geth-side bundle service will reap transactions from the pending transactions subscription (this is a Subscription object internal to geth that the websocket subscription also uses), and wrap them in this Bundle struct which it will then stream to the auctioneer.

because we plan to clear the mempool on geth after every optimistic block execution, the idea is that bundles received by the auctioneer via this stream after the optimistic execution result will have been checked against the optimistic state by geth's mempool's promotion to pending.

the auctioneer then has three streams, one for optimistic block execution results, one for block commitments and one for bundles. more on this in this part of the doc: https://www.notion.so/astria-org/20240919-Status-Update-1066bd31a90c80b68bb2ea9b898645d0?pvs=4#1066bd31a90c80f193e3d6cb72a2f6e9

service BundleService {
// Stream blocks from the Auctioneer to Geth for optimistic execution. Geth will stream back
// metadata from the executed blocks.
rpc ExecuteOptimisticBlocks(stream BaseBlock) returns (stream astria.execution.v1alpha2.Block);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
rpc ExecuteOptimisticBlocks(stream BaseBlock) returns (stream astria.execution.v1alpha2.Block);
rpc ExecuteOptimisticBlock(stream BaseBlock) returns (stream astria.execution.v1alpha2.Block);

however i'm not sure why this is needed when we already have an ExecuteBlock method - they seem to be taking the same inputs and doing the same thing (just executing a block and returning it)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Its not just doing the same thing as conductor. When geth receives blocks from ExecuteOptimisticBlock, it does the following:

  1. Execute the block and update the optimistically synced fork
  2. Update the legacy mempool to use the optimistic block to perform stateful checks on
  3. Clear the mempool of transactions to allow searchers to send transactions valid on a new block.

(2) and (3) will be done with events

It is also a good idea to separate it out from ExecuteBlock for the following reasons:

  1. We are using a stream here instead of a unary RPC for efficiency
  2. ExecuteBlock holds a lock when executing blocks. it might be better to separate out the calls and let only conductor only interact with ExecuteBlock to avoid any unnecessary contentions.
  3. Conductor is already an audited path, changes in conductor would involve us to re-audit it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the optimistically synced fork

does an auctioneer-geth have two separate forks then? an optimistic fork and a fork from conductor blocks? i'm not sure this is functionally different from the existing head-soft-firm block structure in geth, optimistic can be head, soft and firm as the same as now

does ExecuteOptimisticBlock atomically update the block to be head as well then? if so, it'll need a lock, as SetCommitmentState also updates the head of chain

Copy link
Contributor

Choose a reason for hiding this comment

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

yes so currently, we are maintaining 2 separate forks. An optimistic fork and a fork from conductor blocks. We are not updating the block to be head in geth as per the current impl.
We maintain a separate block pointer for the optimistic block as per: https://github.com/astriaorg/flame/blob/bharath/add-mempool-clearing-event/core/blockchain.go#L242
This is updated in ExecuteOptimisticBlock method as per https://github.com/astriaorg/flame/blob/bharath/add-mempool-clearing-event/grpc/execution/server.go#L302 (I haven't hooked it up with the grpc streams interface yet, so this name will change)
Essentially, we do the following:

  1. We insert the optimistic block into the chain without setting the head
  2. We set the optimistic block pointer to this block header to reference the state in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

my thinking is that, by this method we don't need to hold the lock held in ExecuteBlock and UpdateStateCommitment because the pieces of data being operated on are different and do not interfere.

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.

some nits on naming but otherwise looks good to me.

move optimistic_block to sequencerblock

remove prev_rollup_block_hash from BaseBlock

move bundle service to executionapis

request type for baseblock

separate bundle service into OptimisticExecutionService and BundleService
@itamarreif itamarreif force-pushed the itamarreif/auctioneer/initial-protos branch from a491b88 to 69b4ecd Compare October 2, 2024 20:39
@itamarreif itamarreif added this pull request to the merge queue Oct 2, 2024
Merged via the queue into main with commit b54ccb9 Oct 2, 2024
44 checks passed
@itamarreif itamarreif deleted the itamarreif/auctioneer/initial-protos branch October 2, 2024 20:57
github-merge-queue bot pushed a commit that referenced this pull request Oct 7, 2024
…ution in proposal phase (#1562)

## Summary
refactor the sequencer app to generate and store the resulting
`SequencerBlock` after transaction execution even in the proposal phase.

## Background
previously, we were only generating the `SequencerBlock` in
`finalize_block`, however with the upcoming builder APIs (#1519) we
require the (proposed) `SequencerBlock` to be available after execution
in the proposal phase.

## Changes
- create a `post_execute_transactions` method and move the
after-execution logic that generates the `SequencerBlock` from
`finalize_block` to there.
- call this method after transaction execution in `process_proposal`. 
- if txs were executed in `prepare_proposal`,
`post_execute_transactions` is still called in `process_proposal`, as
the block hash is not available in `prepare_proposal`.

## Testing
existing unit tests pass, app logic was not changed, just refactored

## Related Issues
 related to #1322
steezeburger added a commit that referenced this pull request Oct 7, 2024
* main: (34 commits)
  feat(proto): add bundle and optimistic block apis (#1519)
  feat(sequencer)!: make empty transactions invalid  (#1609)
  chore(sequencer): simplify boolean expressions in `transaction container` (#1595)
  refactor(cli): merge argument parsing and command execution (#1568)
  feat(charts): astrotrek chart (#1513)
  chore(charts): genesis template to support latest changes (#1594)
  fix(ci): code freeze action fix (#1610)
  chore(sequencer)!: exclusively use Borsh encoding for stored data (ENG-768) (#1492)
  ci: code freeze through github actions (#1588)
  refactor(sequencer): use builder pattern for transaction container tests (#1592)
  feat(conductor)!: implement chain ID checks (#1482)
  chore(ci): upgrade audit-check (#1577)
  feat(sequencer)!: transaction categories on UnsignedTransaction (#1512)
  fix(charts): sequencer prometheus rules   (#1598)
  chore(all): Migrate all instances of `#[allow]` to `#[expect]` (#1561)
  chore(charts,sequencer-faucet): asset precision (#1517)
  chore(docs): endpoints (#1543)
  fix(docker): use target binary build param as name of image entrypoint (#1573)
  fix(ci): ibc bridge test timeout (#1587)
  Feature: Add `graph-node` to charts. (#1489)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auctioneer cd ci issues that are related to ci and github workflows conductor pertaining to the astria-conductor crate docker proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add v1alpha1 proto auctioneer apis
4 participants