requested changes and fix ibc tests

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

@@ -168,6 +168,7 @@ impl Reader {
             .await
     }
 
+    // TODO(https://github.com/astriaorg/astria/issues/1879): refactor to not return an empty tuple
     #[instrument(skip_all, err)]
     async fn initialize(
         &mut self,
@@ -198,7 +199,7 @@ impl Reader {
                     .wrap_err("failed to get sequencer chain ID")?;
             let expected_sequencer_chain_id = executor.sequencer_chain_id();
             ensure!(
-                expected_sequencer_chain_id == actual_sequencer_chain_id.to_string(),
+                expected_sequencer_chain_id == actual_sequencer_chain_id.as_str(),
                 "expected Celestia chain id `{expected_sequencer_chain_id}` does not match \
                  actual: `{actual_sequencer_chain_id}`"
             );

crates/astria-conductor/src/conductor/inner.rs

@@ -331,14 +331,20 @@ mod tests {
         assert!(super::check_for_restart("executor", &err.unwrap_err()));
 
         let celestia_height_error: Result<&str, super::StopHeightExceded> =
-            Err(super::StopHeightExceded::Celestia);
+            Err(super::StopHeightExceded::Celestia {
+                firm_height: 1,
+                stop_height: 1,
+            });
         let err = celestia_height_error.wrap_err("wrapper_1");
         let err = err.wrap_err("wrapper_2");
         let err = err.wrap_err("wrapper_3");
         assert!(super::check_for_restart("executor", &err.unwrap_err()));
 
         let sequencer_height_error: Result<&str, super::StopHeightExceded> =
-            Err(super::StopHeightExceded::Sequencer);
+            Err(super::StopHeightExceded::Sequencer {
+                soft_height: 1,
+                stop_height: 1,
+            });
         let err = sequencer_height_error.wrap_err("wrapper_1");
         let err = err.wrap_err("wrapper_2");
         let err = err.wrap_err("wrapper_3");

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

@@ -68,11 +68,27 @@ pub(crate) struct StateNotInit;
 pub(crate) struct StateIsInit;
 
 #[derive(Debug, thiserror::Error)]
-pub(crate) enum StopHeightExceded {
-    #[error("firm height exceeded sequencer stop height")]
-    Celestia,
-    #[error("soft height met or exceeded sequencer stop height")]
-    Sequencer,
+pub(super) enum StopHeightExceded {
+    #[error("firm height {firm_height} exceeded sequencer stop height {stop_height}")]
+    Celestia { firm_height: u64, stop_height: u64 },
+    #[error("soft height {soft_height} met or exceeded sequencer stop height {stop_height}")]
+    Sequencer { soft_height: u64, stop_height: u64 },
+}
+
+impl StopHeightExceded {
+    fn celestia(firm_height: u64, stop_height: u64) -> Self {
+        StopHeightExceded::Celestia {
+            firm_height,
+            stop_height,
+        }
+    }
+
+    fn sequencer(soft_height: u64, stop_height: u64) -> Self {
+        StopHeightExceded::Sequencer {
+            soft_height,
+            stop_height,
+        }
+    }
 }
 
 #[derive(Debug, thiserror::Error)]
@@ -416,29 +432,24 @@ impl Executor {
         // executing firm blocks, we let execution continue since one more firm block will be
         // executed before `execute_firm` initiates a restart. If we are in soft-only mode, we
         // return a `StopHeightExceded::Sequencer` error to signal a restart.
-        if self.state.sequencer_stop_block_height().value() > 0
-            && executable_block.height >= self.state.sequencer_stop_block_height()
-        {
-            let res = if self.mode.is_with_firm() {
-                info!(
-                    height = %executable_block.height,
-                    "received soft block whose height is greater than or equal to stop block height in {} mode. \
-                    dropping soft block and waiting for next firm block before attempting restart",
-                    self.mode,
-                );
-                Ok(())
-            } else {
+        if self.is_soft_block_height_exceded(&executable_block) {
+            if self.mode.is_with_firm() {
                 info!(
                     height = %executable_block.height,
-                    "received soft block whose height is greater than or equal to stop block \
-                    height in soft only mode. shutting down and attempting restart",
+                    "received soft block whose height is greater than or equal to stop block height. \
+                    dropping soft block and waiting for next firm block before attempting restart. \
+                    will continue logging info of soft blocks until restart",
                 );
-                Err(StopHeightExceded::Sequencer).wrap_err(
-                    "soft height met or exceeded sequencer stop height, attempting restart with \
-                     new genesis info",
-                )
-            };
-            return res;
+                return Ok(());
+            }
+            return Err(StopHeightExceded::sequencer(
+                executable_block.height.value(),
+                self.state.sequencer_stop_block_height(),
+            ))
+            .wrap_err(
+                "soft height met or exceeded sequencer stop height, attempting restart with new \
+                 genesis info",
+            );
         }
 
         let expected_height = self.state.next_expected_soft_sequencer_height();
@@ -505,15 +516,12 @@ impl Executor {
         err,
     ))]
     async fn execute_firm(&mut self, block: ReconstructedBlock) -> eyre::Result<()> {
-        if self.state.sequencer_stop_block_height().value() > 0
-            && block.header.height() > self.state.sequencer_stop_block_height()
-        {
-            info!(
-                height = %block.header.height(),
-                "received firm block whose height is greater than stop block height. \
-                exiting and attempting restart with new genesis info",
-            );
-            return Err(StopHeightExceded::Celestia).wrap_err(
+        if self.is_firm_block_height_exceded(&block) {
+            return Err(StopHeightExceded::celestia(
+                block.header.height().value(),
+                self.state.sequencer_stop_block_height(),
+            ))
+            .wrap_err(
                 "firm height exceeded sequencer stop height, attempting restart with new genesis \
                  info",
             );
@@ -731,6 +739,19 @@ impl Executor {
             self.mode,
         )
     }
+
+    /// Returns whether the height of the soft block is greater than or equal to the stop block
+    /// height.
+    fn is_soft_block_height_exceded(&self, block: &ExecutableBlock) -> bool {
+        let stop_height = self.state.sequencer_stop_block_height();
+        stop_height > 0 && block.height.value() >= stop_height
+    }
+
+    /// Returns whether the height of the firm block is greater than the stop block height.
+    fn is_firm_block_height_exceded(&self, block: &ReconstructedBlock) -> bool {
+        let stop_height = self.state.sequencer_stop_block_height();
+        stop_height > 0 && block.header.height().value() > stop_height
+    }
 }
 
 #[instrument(skip_all)]

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

@@ -113,7 +113,7 @@ fn can_map_firm_to_sequencer_height(
     {
         Err(InvalidState {
             commitment_type: "firm",
-            sequencer_start_block_height: sequencer_start_block_height.value(),
+            sequencer_start_block_height,
             rollup_number: rollup_number.into(),
         })
     } else {
@@ -137,7 +137,7 @@ fn can_map_soft_to_sequencer_height(
     {
         Err(InvalidState {
             commitment_type: "soft",
-            sequencer_start_block_height: sequencer_start_block_height.value(),
+            sequencer_start_block_height,
             rollup_number: rollup_number.into(),
         })
     } else {
@@ -236,9 +236,9 @@ forward_impls!(
     [soft_hash -> Bytes],
     [celestia_block_variance -> u64],
     [rollup_id -> RollupId],
-    [sequencer_start_block_height -> SequencerHeight],
+    [sequencer_start_block_height -> u64],
     [celestia_base_block_height -> u64],
-    [sequencer_stop_block_height -> SequencerHeight],
+    [sequencer_stop_block_height -> u64],
     [rollup_start_block_height -> u64]
 );
 
@@ -307,11 +307,11 @@ impl State {
         self.genesis_info.celestia_block_variance()
     }
 
-    fn sequencer_start_block_height(&self) -> SequencerHeight {
+    fn sequencer_start_block_height(&self) -> u64 {
         self.genesis_info.sequencer_start_block_height()
     }
 
-    fn sequencer_stop_block_height(&self) -> SequencerHeight {
+    fn sequencer_stop_block_height(&self) -> u64 {
         self.genesis_info.sequencer_stop_block_height()
     }
 
@@ -353,11 +353,10 @@ impl State {
 /// Returns `None` if `sequencer_start_block_height + rollup_number - rollup_start_block_height`
 /// overflows `u32::MAX`.
 fn map_rollup_number_to_sequencer_height(
-    sequencer_start_block_height: SequencerHeight,
+    sequencer_start_block_height: u64,
     rollup_number: u32,
     rollup_start_block_height: u64,
 ) -> Option<SequencerHeight> {
-    let sequencer_start_block_height = sequencer_start_block_height.value();
     let rollup_number: u64 = rollup_number.into();
     let sequencer_height = sequencer_start_block_height
         .checked_add(rollup_number)?
@@ -370,13 +369,13 @@ fn map_rollup_number_to_sequencer_height(
 /// Returns `None` if `sequencer_height - sequencer_start_block_height + rollup_start_block_height`
 /// underflows or if the result does not fit in `u32`.
 pub(super) fn map_sequencer_height_to_rollup_height(
-    sequencer_start_height: SequencerHeight,
+    sequencer_start_height: u64,
     sequencer_height: SequencerHeight,
     rollup_start_block_height: u64,
 ) -> Option<u32> {
     sequencer_height
         .value()
-        .checked_sub(sequencer_start_height.value())?
+        .checked_sub(sequencer_start_height)?
         .checked_add(rollup_start_block_height)?
         .try_into()
         .ok()
@@ -464,7 +463,7 @@ mod tests {
     fn assert_height_is_correct(left: u32, right: u32, expected: u32) {
         assert_eq!(
             SequencerHeight::from(expected),
-            map_rollup_number_to_sequencer_height(SequencerHeight::from(left), right, 0)
+            map_rollup_number_to_sequencer_height(left.into(), right, 0)
                 .expect("left + right is so small, they should never overflow"),
         );
     }

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

@@ -23,6 +23,14 @@ impl GenesisInfoError {
     fn no_rollup_id() -> Self {
         Self(GenesisInfoErrorKind::NoRollupId)
     }
+
+    fn invalid_sequencer_id(source: tendermint::Error) -> Self {
+        Self(GenesisInfoErrorKind::InvalidSequencerId(source))
+    }
+
+    fn invalid_celestia_id(source: celestia_tendermint::Error) -> Self {
+        Self(GenesisInfoErrorKind::InvalidCelestiaId(source))
+    }
 }
 
 #[derive(Debug, thiserror::Error)]
@@ -31,6 +39,10 @@ enum GenesisInfoErrorKind {
     IncorrectRollupIdLength(IncorrectRollupIdLength),
     #[error("`rollup_id` was not set")]
     NoRollupId,
+    #[error("invalid tendermint chain ID for sequencer")]
+    InvalidSequencerId(#[source] tendermint::Error),
+    #[error("invalid celestia-tendermint chain ID for celestia")]
+    InvalidCelestiaId(#[source] celestia_tendermint::Error),
 }
 
 /// Genesis Info required from a rollup to start an execution client.
@@ -57,9 +69,9 @@ pub struct GenesisInfo {
     /// The rollup block number to map to the sequencer start block height.
     rollup_start_block_height: u64,
     /// The chain ID of the sequencer network.
-    sequencer_chain_id: String,
+    sequencer_chain_id: tendermint::chain::Id,
     /// The chain ID of the celestia network.
-    celestia_chain_id: String,
+    celestia_chain_id: celestia_tendermint::chain::Id,
 }
 
 impl GenesisInfo {
@@ -69,13 +81,13 @@ impl GenesisInfo {
     }
 
     #[must_use]
-    pub fn sequencer_start_block_height(&self) -> tendermint::block::Height {
-        self.sequencer_start_block_height
+    pub fn sequencer_start_block_height(&self) -> u64 {
+        self.sequencer_start_block_height.into()
     }
 
     #[must_use]
-    pub fn sequencer_stop_block_height(&self) -> tendermint::block::Height {
-        self.sequencer_stop_block_height
+    pub fn sequencer_stop_block_height(&self) -> u64 {
+        self.sequencer_stop_block_height.into()
     }
 
     #[must_use]
@@ -84,13 +96,13 @@ impl GenesisInfo {
     }
 
     #[must_use]
-    pub fn sequencer_chain_id(&self) -> &str {
+    pub fn sequencer_chain_id(&self) -> &tendermint::chain::Id {
         &self.sequencer_chain_id
     }
 
     #[must_use]
     pub fn celestia_chain_id(&self) -> &str {
-        &self.celestia_chain_id
+        self.celestia_chain_id.as_str()
     }
 
     #[must_use]
@@ -124,15 +136,23 @@ impl Protobuf for GenesisInfo {
         };
         let rollup_id = RollupId::try_from_raw_ref(rollup_id)
             .map_err(Self::Error::incorrect_rollup_id_length)?;
+        let sequencer_chain_id = sequencer_chain_id
+            .clone()
+            .try_into()
+            .map_err(Self::Error::invalid_sequencer_id)?;
+        let celestia_chain_id = celestia_chain_id
+            .clone()
+            .try_into()
+            .map_err(Self::Error::invalid_celestia_id)?;
 
         Ok(Self {
             rollup_id,
             sequencer_start_block_height: (*sequencer_start_block_height).into(),
             sequencer_stop_block_height: (*sequencer_stop_block_height).into(),
             celestia_block_variance: *celestia_block_variance,
             rollup_start_block_height: *rollup_start_block_height,
-            sequencer_chain_id: sequencer_chain_id.clone(),
-            celestia_chain_id: celestia_chain_id.clone(),
+            sequencer_chain_id,
+            celestia_chain_id,
         })
     }
 
@@ -164,8 +184,8 @@ impl Protobuf for GenesisInfo {
             sequencer_stop_block_height,
             celestia_block_variance: *celestia_block_variance,
             rollup_start_block_height: *rollup_start_block_height,
-            sequencer_chain_id: sequencer_chain_id.clone(),
-            celestia_chain_id: celestia_chain_id.clone(),
+            sequencer_chain_id: sequencer_chain_id.to_string(),
+            celestia_chain_id: celestia_chain_id.to_string(),
         }
     }
 }