feat(proto, conductor, cd)!: changes for Forma migration #1843

ethanoroshiba · 2024-12-03T15:24:54Z

Summary

Makes the changes necessary in the monorepo to migrate Forma to Mainnet.

Background

Migrating Forma to mainnet necessitates introducing stop and start logic into the Conductor. This makes the proto and Conductor changes necessary for that. In addition to this, this will allow for future needs to stop and start EVMs on our networks. These changes will also need to be reflected in the GenesisInfo sent by rollups to the conductor.

The shutdown and restart logic for the Conductor should be as follows:

Firm Only Mode: Once the stop height is reached for the firm block stream, the firm block at this height is executed (and commitment state updated) before restarting the internal conductor, prompting the rollup for a new GenesisInfo with new start/stop heights (and potentially chain IDs).
Soft and Firm Mode: Once the stop height is reached for the soft block stream, no more soft blocks are executed (including the one at the stop height). The firm block is still executed for this height, and then Conductor restarts.
Soft Only Mode: Once the stop height is reached for the soft block stream, no more soft blocks are executed (including the one at the stop height). Conductor is then restarted.

The aforementioned rollup changes to be made in astria-geth necessitate changing our current charts config for the following. "Forks" now describes a set of instructions for when and how to stop and restart the EVM at different heights, such that changes (such as network migration) can be made.

config:
        # < non astria-prefixed items... >
        AstriaOverrideGenesisExtraData:
        AstriaRollupName:
        AstriaForks: []
                # - < fork name >:
                #         height: ""
                #         halt: ""
                #         snapshotChecksum: ""
                #         extraDataOverride: ""
                #         feeCollector: ""
                #         EIP1559Params: {}
                #         sequencer:
                #                 chainId: ""
                #                 addressPrefix: ""
                #                 startHeight: ""
                #                 stopHeight: ""
                #         celestia:
                #                 chainId: ""
                #                 startHeight: ""
                #                 heightVariance: ""
                #         bridgeAddresses: []

Changes

Changed sequencer_genesis_block_height to sequencer_start_block_height in GenesisInfo (in the execution API).
Added sequencer_stop_block_height, rollup_start_block_height, sequencer_chain_id, and celestia_chain_id to GenesisInfo.
Changed sequencer to rollup height calculation to start from rollup_start_block_height in GenesisInfo, so that Conductor doesn't start from rollup height 1 again after restarting.
Removed sequencer and celestia chain ID environment vars from Conductor, and changed chain ID checks to rely on obtaining these from the initialized executor state.
Added astriaForks to EVM rollup config, to reflect changes in astria-geth: https://github.com/astriaorg/astria-geth/pull/59/files.
Added restart logic for when the stop height is reached.

Testing

Added restart logic to check_for_restart_ok unit test.
Added blackbox tests to ensure conductor correctly restarts in each mode, and continues to execute at the correct height after fetching updated genesis info and commitment state.

Changelogs

Changelogs updated

Breaking Changelist

Proto changes are not technically breaking, since fields were only added, but the old genesis info shape will be rejected since it does not contain the chain IDs to validate against the networks the conductor is connecting to (and would fail even if this was the case, due to sequencer_stop_block_height being read from wire format as 0). Thus, rollups will need to provide an updated GenesisInfo to conductor (includingastria-geth).
Broke domain type GenesisInfo

Related Issues

closes #1841

ethanoroshiba · 2024-12-09T21:36:24Z

Opening this for review despite failing status checks so that review isn't blocking on Geth changes. Once a Geth image can be pulled, we'll be able to run the smoke tests.

SuperFluffy · 2024-12-10T14:25:34Z

proto/executionapis/astria/execution/v1/execution.proto

@@ -14,9 +14,15 @@ message GenesisInfo {
  // The rollup_id is the unique identifier for the rollup chain.
  astria.primitive.v1.RollupId rollup_id = 1;
  // The first block height of sequencer chain to use for rollup transactions.
-  uint32 sequencer_genesis_block_height = 2;
+  uint32 sequencer_start_block_height = 2;


Does this warrant a breaking change?

I don't believe this would be a breaking change (even though the proto breaking changes lint would disagree), since the field is identified by the number itself as opposed to the name. The name change is just to more accurately reflect the new functionality, since conductor can now start from anywhere, not just genesis. I do think the proto changes at large are effectively breaking though, since the old shape will no longer work with with conductor (chain IDs will be empty strings, so the components will never pass initialization).

You are right that the name is indeed a better reflection of how this field is used.

However, I would in general be more conservative when changing APIs: a breaking change (in this case, in the JSON serialization) weighs heavier than a name that's no longer a completely honest representation of the functionality.

SuperFluffy · 2024-12-10T14:27:46Z

Note on the PR: the document #1843 is private. Please put all relevant information into this PR without referring to an outside, private document.

…rg/astria into ENG-1031/forma_migration

SuperFluffy · 2024-12-10T14:31:19Z

crates/astria-core/src/execution/v1/mod.rs

-    sequencer_genesis_block_height: tendermint::block::Height,
+    sequencer_start_block_height: tendermint::block::Height,
+    /// The Sequencer block height to stop at.
+    sequencer_stop_block_height: tendermint::block::Height,


The case stop_block_height < start_block_height is not covered in the tests. Should they be?

Hmm, I think it would still just restart in this case? Josh introduced some logic such that if it receives a stop height of 0, it acts as if there is no stop block height. But in the case where stop block height is not 0 and is less than start block height, the conductor will go to execute and then just restart again, so the functionality should already be tested, but I can add a specific test for this scenario just to be sure if you'd like. In either case, if the stop height is ever below the start, I think something is wrong on the rollup end

crates/astria-core/src/execution/v1/mod.rs

SuperFluffy · 2024-12-10T14:33:10Z

crates/astria-core/src/execution/v1/mod.rs

@@ -61,14 +69,34 @@ impl GenesisInfo {
    }

    #[must_use]
-    pub fn sequencer_genesis_block_height(&self) -> tendermint::block::Height {
-        self.sequencer_genesis_block_height
+    pub fn sequencer_start_block_height(&self) -> tendermint::block::Height {


If you are breaking the astria-core anyway, let's make tendermint::block::Height an implementation detail and have this method return u32 instead.

Height supports Into<u64> but not u32, so I used that instead in 2f7ed73. Can convert again to u32 if preferred.

SuperFluffy · 2024-12-10T14:33:27Z

crates/astria-core/src/execution/v1/mod.rs

+    }
+
+    #[must_use]
+    pub fn sequencer_stop_block_height(&self) -> tendermint::block::Height {


Same here, let's have this return a u32 instead

crates/astria-conductor/src/executor/mod.rs

SuperFluffy · 2024-12-13T14:42:35Z

crates/astria-conductor/src/executor/mod.rs

@@ -396,6 +412,35 @@ impl Executor {
        // TODO(https://github.com/astriaorg/astria/issues/624): add retry logic before failing hard.


Comment for above: the error cases are not actual errors but part of normal execution. As such, I would change the return type of execute_soft to encode if a restart should happen due to the max being reached.

SuperFluffy · 2024-12-13T14:44:46Z

crates/astria-conductor/src/executor/mod.rs

@@ -457,19 +505,36 @@ impl Executor {
        err,
    ))]
    async fn execute_firm(&mut self, block: ReconstructedBlock) -> eyre::Result<()> {


The same arguments as for execute_soft also apply to execute_firm. Please see my comments there,

proto changes, add stop/restart logic

d8b9778

github-actions bot added conductor pertaining to the astria-conductor crate proto pertaining to the Astria Protobuf spec cd labels Dec 3, 2024

ethanoroshiba temporarily deployed to BUF December 3, 2024 15:25 — with GitHub Actions Inactive

ethanoroshiba added the override-freeze label Dec 3, 2024

add soft and firm test, update change log and valus

fdbe302

ethanoroshiba temporarily deployed to BUF December 3, 2024 19:01 — with GitHub Actions Inactive

ethanoroshiba added the allow-breaking-proto label Dec 3, 2024

version updates

30bd915

ethanoroshiba temporarily deployed to BUF December 3, 2024 19:24 — with GitHub Actions Inactive

update chart.lock

ff9ad8c

ethanoroshiba temporarily deployed to BUF December 3, 2024 19:26 — with GitHub Actions Inactive

add rollup_start_block_height field to GenesisInfo

af132a1

ethanoroshiba temporarily deployed to BUF December 4, 2024 20:02 — with GitHub Actions Inactive

update height mapping calculation, flush out tests

59456a4

ethanoroshiba temporarily deployed to BUF December 4, 2024 21:54 — with GitHub Actions Inactive

ethanoroshiba added 2 commits December 5, 2024 12:30

geth devtag

afc2499

Merge branch 'main' into ENG-1031/forma_migration

f7c9682

ethanoroshiba temporarily deployed to BUF December 5, 2024 19:05 — with GitHub Actions Inactive

ethanoroshiba marked this pull request as ready for review December 9, 2024 21:36

ethanoroshiba requested review from a team as code owners December 9, 2024 21:36

ethanoroshiba requested review from Fraser999, SuperFluffy and aajimal December 9, 2024 21:36

fix height logic

90b1f11

ethanoroshiba temporarily deployed to BUF December 9, 2024 22:58 — with GitHub Actions Inactive

rustfmt

2a3ca48

ethanoroshiba temporarily deployed to BUF December 9, 2024 23:06 — with GitHub Actions Inactive

SuperFluffy reviewed Dec 10, 2024

View reviewed changes

charts changes to reflect changes in geth

581424f

ethanoroshiba temporarily deployed to BUF December 10, 2024 17:11 — with GitHub Actions Inactive

fix extraDataOverride naming

36572d0

ethanoroshiba temporarily deployed to BUF December 10, 2024 17:28 — with GitHub Actions Inactive

remove rollup start height from geth config

6e790c1

ethanoroshiba temporarily deployed to BUF December 10, 2024 19:12 — with GitHub Actions Inactive

ethanoroshiba changed the title ~~feat(proto, conductor)!: changes for Forma migration~~ feat(proto, conductor, cd)!: changes for Forma migration Dec 12, 2024

ethanoroshiba and others added 4 commits December 12, 2024 13:42

fix erroneous doc comment

5e562e4

Merge branch 'main' into ENG-1031/forma_migration

8988c88

stop height of 0 means do not stop

d971d5c

Merge branch 'ENG-1031/forma_migration' of https://github.com/astriao…

fa7474c

…rg/astria into ENG-1031/forma_migration

ethanoroshiba temporarily deployed to BUF December 13, 2024 14:26 — with GitHub Actions Inactive

charts updates

fa7af2a

ethanoroshiba temporarily deployed to BUF December 13, 2024 14:33 — with GitHub Actions Inactive

SuperFluffy reviewed Dec 13, 2024

View reviewed changes

fix charts

ea80fbb

ethanoroshiba temporarily deployed to BUF December 13, 2024 16:43 — with GitHub Actions Inactive

Fix values

22edac8

ethanoroshiba temporarily deployed to BUF December 13, 2024 16:47 — with GitHub Actions Inactive

requested changes and fix ibc tests

2f7ed73

ethanoroshiba deployed to BUF December 13, 2024 21:07 — with GitHub Actions Active

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(proto, conductor, cd)!: changes for Forma migration #1843

feat(proto, conductor, cd)!: changes for Forma migration #1843

ethanoroshiba commented Dec 3, 2024 •

edited

Loading

ethanoroshiba commented Dec 9, 2024

SuperFluffy Dec 10, 2024

ethanoroshiba Dec 10, 2024 •

edited

Loading

SuperFluffy Dec 13, 2024

SuperFluffy commented Dec 10, 2024

SuperFluffy Dec 10, 2024

ethanoroshiba Dec 13, 2024

SuperFluffy Dec 10, 2024

ethanoroshiba Dec 13, 2024

SuperFluffy Dec 10, 2024

SuperFluffy Dec 13, 2024

SuperFluffy Dec 13, 2024

		@@ -396,6 +412,35 @@ impl Executor {
		// TODO(https://github.com/astriaorg/astria/issues/624): add retry logic before failing hard.

feat(proto, conductor, cd)!: changes for Forma migration #1843

Are you sure you want to change the base?

feat(proto, conductor, cd)!: changes for Forma migration #1843

Conversation

ethanoroshiba commented Dec 3, 2024 • edited Loading

Summary

Background

Changes

Testing

Changelogs

Breaking Changelist

Related Issues

ethanoroshiba commented Dec 9, 2024

Choose a reason for hiding this comment

ethanoroshiba Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanoroshiba commented Dec 3, 2024 •

edited

Loading

ethanoroshiba Dec 10, 2024 •

edited

Loading