From 730adec1b34956bc66f98079353809c01003b90c Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Wed, 29 May 2024 15:05:06 +0200 Subject: [PATCH 1/2] feat(conductor): drop already confirmed firm heights --- crates/astria-conductor/src/celestia/mod.rs | 5 ++- .../astria-conductor/src/celestia/verify.rs | 39 +++++++++++++------ .../tests/blackbox/firm_only.rs | 32 ++++----------- .../tests/blackbox/helpers/macros.rs | 9 +++-- .../tests/blackbox/helpers/mod.rs | 11 ++++++ .../tests/blackbox/soft_and_firm.rs | 4 +- 6 files changed, 58 insertions(+), 42 deletions(-) diff --git a/crates/astria-conductor/src/celestia/mod.rs b/crates/astria-conductor/src/celestia/mod.rs index d642c9dbcc..d69b87f764 100644 --- a/crates/astria-conductor/src/celestia/mod.rs +++ b/crates/astria-conductor/src/celestia/mod.rs @@ -427,6 +427,7 @@ impl RunningReader { rollup_id: self.rollup_id, rollup_namespace: self.rollup_namespace, sequencer_namespace: self.sequencer_namespace, + executor: self.executor.clone(), }; self.reconstruction_tasks.spawn(height, task.execute()); scheduled.push(height); @@ -498,6 +499,7 @@ struct FetchConvertVerifyAndReconstruct { rollup_id: RollupId, rollup_namespace: Namespace, sequencer_namespace: Namespace, + executor: executor::Handle, } impl FetchConvertVerifyAndReconstruct { @@ -514,6 +516,7 @@ impl FetchConvertVerifyAndReconstruct { rollup_id, rollup_namespace, sequencer_namespace, + executor, } = self; let new_blobs = fetch_new_blobs( @@ -585,7 +588,7 @@ impl FetchConvertVerifyAndReconstruct { "decoded Sequencer header and rollup info from raw Celestia blobs", ); - let verified_blobs = verify_metadata(blob_verifier, decoded_blobs).await; + let verified_blobs = verify_metadata(blob_verifier, decoded_blobs, executor).await; { // allow: histograms require f64; precision loss would be no problem diff --git a/crates/astria-conductor/src/celestia/verify.rs b/crates/astria-conductor/src/celestia/verify.rs index b0b814bf99..1f44662da5 100644 --- a/crates/astria-conductor/src/celestia/verify.rs +++ b/crates/astria-conductor/src/celestia/verify.rs @@ -50,6 +50,10 @@ use super::{ block_verifier, convert::ConvertedBlobs, }; +use crate::executor::{ + self, + StateIsInit, +}; pub(super) struct VerifiedBlobs { celestia_height: u64, @@ -94,24 +98,37 @@ struct VerificationTaskKey { pub(super) async fn verify_metadata( blob_verifier: Arc, converted_blobs: ConvertedBlobs, + mut executor: executor::Handle, ) -> VerifiedBlobs { let (celestia_height, header_blobs, rollup_blobs) = converted_blobs.into_parts(); let mut verification_tasks = JoinMap::new(); let mut verified_header_blobs = HashMap::with_capacity(header_blobs.len()); + let next_expected_firm_sequencer_height = + executor.next_expected_firm_sequencer_height().value(); + for (index, blob) in header_blobs.into_iter().enumerate() { - verification_tasks.spawn( - VerificationTaskKey { - index, - block_hash: blob.block_hash(), - sequencer_height: blob.height(), - }, - blob_verifier - .clone() - .verify_metadata(blob) - .in_current_span(), - ); + if blob.height().value() < next_expected_firm_sequencer_height { + info!( + next_expected_firm_sequencer_height, + sequencer_height_in_metadata = blob.height().value(), + "dropping Sequencer metadata item without verifying against Sequencer because its \ + height is below the next expected firm height" + ); + } else { + verification_tasks.spawn( + VerificationTaskKey { + index, + block_hash: blob.block_hash(), + sequencer_height: blob.height(), + }, + blob_verifier + .clone() + .verify_metadata(blob) + .in_current_span(), + ); + } } while let Some((key, verification_result)) = verification_tasks.join_next().await { diff --git a/crates/astria-conductor/tests/blackbox/firm_only.rs b/crates/astria-conductor/tests/blackbox/firm_only.rs index ea7b5056b4..18e5b64f99 100644 --- a/crates/astria-conductor/tests/blackbox/firm_only.rs +++ b/crates/astria-conductor/tests/blackbox/firm_only.rs @@ -55,7 +55,7 @@ async fn simple() { mount_celestia_blobs!( test_conductor, celestia_height: 1, - sequencer_height: 3, + sequencer_heights: [3], ); mount_sequencer_commit!( @@ -136,13 +136,7 @@ async fn submits_two_heights_in_succession() { mount_celestia_blobs!( test_conductor, celestia_height: 1, - sequencer_height: 3, - ); - - mount_celestia_blobs!( - test_conductor, - celestia_height: 2, - sequencer_height: 4, + sequencer_heights: [3, 4], ); mount_sequencer_commit!( @@ -250,26 +244,16 @@ async fn skips_already_executed_heights() { height: 2u32, ); - // The blob with sequencer height 5 will be fetched but never forwarded + // The blob contains sequencer heights 6 and 7, but no commits or validator sets are mounted. + // XXX: A non-fetch cannot be tested for programmatically right now. Running the test with + // tracing enabled should show that the sequencer metadata at height 6 is explicitly + // skipped. mount_celestia_blobs!( test_conductor, celestia_height: 1, - sequencer_height: 6, + sequencer_heights: [6, 7], ); - mount_celestia_blobs!( - test_conductor, - celestia_height: 2, - sequencer_height: 7, - ); - - mount_sequencer_commit!( - test_conductor, - height: 6u32, - ); - - mount_sequencer_validator_set!(test_conductor, height: 5u32); - mount_sequencer_commit!( test_conductor, height: 7u32, @@ -348,7 +332,7 @@ async fn fetch_from_later_celestia_height() { mount_celestia_blobs!( test_conductor, celestia_height: 4, - sequencer_height: 3, + sequencer_heights: [3], ); mount_sequencer_commit!( diff --git a/crates/astria-conductor/tests/blackbox/helpers/macros.rs b/crates/astria-conductor/tests/blackbox/helpers/macros.rs index f3abbee2dc..4f6779c7d0 100644 --- a/crates/astria-conductor/tests/blackbox/helpers/macros.rs +++ b/crates/astria-conductor/tests/blackbox/helpers/macros.rs @@ -111,11 +111,12 @@ macro_rules! signed_header { #[macro_export] macro_rules! mount_celestia_blobs { ( - $test_env:ident,celestia_height: - $celestia_height:expr,sequencer_height: - $sequencer_height:expr $(,)? + $test_env:ident, + celestia_height: $celestia_height:expr, + sequencer_heights: [ $($sequencer_height:expr),+ ] + $(,)? ) => {{ - let blobs = $crate::helpers::make_blobs(&[$sequencer_height]); + let blobs = $crate::helpers::make_blobs(&[ $( $sequencer_height ),+ ]); $test_env .mount_celestia_blob_get_all( $celestia_height, diff --git a/crates/astria-conductor/tests/blackbox/helpers/mod.rs b/crates/astria-conductor/tests/blackbox/helpers/mod.rs index 3f9c2701dc..66f415c2b3 100644 --- a/crates/astria-conductor/tests/blackbox/helpers/mod.rs +++ b/crates/astria-conductor/tests/blackbox/helpers/mod.rs @@ -473,7 +473,18 @@ fn make_config() -> Config { #[must_use] pub fn make_sequencer_block(height: u32) -> astria_core::sequencerblock::v1alpha1::SequencerBlock { + fn repeat_bytes_of_u32_as_array(val: u32) -> [u8; 32] { + let repr = val.to_le_bytes(); + [ + repr[0], repr[1], repr[2], repr[3], repr[0], repr[1], repr[2], repr[3], repr[0], + repr[1], repr[2], repr[3], repr[0], repr[1], repr[2], repr[3], repr[0], repr[1], + repr[2], repr[3], repr[0], repr[1], repr[2], repr[3], repr[0], repr[1], repr[2], + repr[3], repr[0], repr[1], repr[2], repr[3], + ] + } + astria_core::protocol::test_utils::ConfigureSequencerBlock { + block_hash: Some(repeat_bytes_of_u32_as_array(height)), chain_id: Some(crate::SEQUENCER_CHAIN_ID.to_string()), height, sequence_data: vec![(crate::ROLLUP_ID, data())], diff --git a/crates/astria-conductor/tests/blackbox/soft_and_firm.rs b/crates/astria-conductor/tests/blackbox/soft_and_firm.rs index 9d9a1e059e..0dacd4af58 100644 --- a/crates/astria-conductor/tests/blackbox/soft_and_firm.rs +++ b/crates/astria-conductor/tests/blackbox/soft_and_firm.rs @@ -76,7 +76,7 @@ async fn simple() { mount_celestia_blobs!( test_conductor, celestia_height: 1, - sequencer_height: 3, + sequencer_heights: [3], ); mount_sequencer_commit!( @@ -191,7 +191,7 @@ async fn missing_block_is_fetched_for_updating_firm_commitment() { mount_celestia_blobs!( test_conductor, celestia_height: 1, - sequencer_height: 3, + sequencer_heights: [3], ); mount_sequencer_commit!( From c6cb7a69b4c27d38e280d97a1d6b44e8c3f6a11b Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Tue, 4 Jun 2024 13:29:01 +0200 Subject: [PATCH 2/2] change base celestia heights in tests --- crates/astria-conductor/tests/blackbox/firm_only.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/astria-conductor/tests/blackbox/firm_only.rs b/crates/astria-conductor/tests/blackbox/firm_only.rs index 18e5b64f99..51cf03db5f 100644 --- a/crates/astria-conductor/tests/blackbox/firm_only.rs +++ b/crates/astria-conductor/tests/blackbox/firm_only.rs @@ -194,7 +194,7 @@ async fn submits_two_heights_in_succession() { hash: [3; 64], parent: [2; 64], ), - base_celestia_height: 2, + base_celestia_height: 1, ); timeout( @@ -280,7 +280,7 @@ async fn skips_already_executed_heights() { hash: [2; 64], parent: [1; 64], ), - base_celestia_height: 2, + base_celestia_height: 1, ); timeout(