Skip to content

feat(conductor): skip outdated block metadata #1120

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.

Sign up for GitHub

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

Merged
SuperFluffy merged 2 commits into from
Jun 4, 2024
Merged

feat(conductor): skip outdated block metadata #1120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
730adec
feat(conductor): drop already confirmed firm heights
SuperFluffy May 29, 2024
c6cb7a6
change base celestia heights in tests
SuperFluffy Jun 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion crates/astria-conductor/src/celestia/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -498,6 +499,7 @@ struct FetchConvertVerifyAndReconstruct {
rollup_id: RollupId,
rollup_namespace: Namespace,
sequencer_namespace: Namespace,
executor: executor::Handle<StateIsInit>,
}

impl FetchConvertVerifyAndReconstruct {
Expand All @@ -514,6 +516,7 @@ impl FetchConvertVerifyAndReconstruct {
rollup_id,
rollup_namespace,
sequencer_namespace,
executor,
} = self;

let new_blobs = fetch_new_blobs(
Expand Down Expand Up @@ -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
Expand Down
39 changes: 28 additions & 11 deletions crates/astria-conductor/src/celestia/verify.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ use super::{
block_verifier,
convert::ConvertedBlobs,
};
use crate::executor::{
self,
StateIsInit,
};

pub(super) struct VerifiedBlobs {
celestia_height: u64,
Expand Down Expand Up @@ -94,24 +98,37 @@ struct VerificationTaskKey {
pub(super) async fn verify_metadata(
blob_verifier: Arc<BlobVerifier>,
converted_blobs: ConvertedBlobs,
mut executor: executor::Handle<StateIsInit>,
) -> 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"
);
Comment on lines +112 to +118
Copy link
Contributor

@noot noot Jun 3, 2024

Choose a reason for hiding this comment

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

is this guaranteed safe? ie are these blobs already guaranteed to be executed/verified as soft blocks already and we're sure that they are the same as the ones already verified?

Copy link
Contributor Author

@SuperFluffy SuperFluffy Jun 4, 2024

Choose a reason for hiding this comment

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

Note that this is checking against the next expected firm height - this is independent of the soft blocks.

} 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 {
Expand Down
36 changes: 10 additions & 26 deletions crates/astria-conductor/tests/blackbox/firm_only.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ async fn simple() {
mount_celestia_blobs!(
test_conductor,
celestia_height: 1,
sequencer_height: 3,
sequencer_heights: [3],
);

mount_sequencer_commit!(
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -200,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(
Expand Down Expand Up @@ -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,
);

mount_celestia_blobs!(
test_conductor,
celestia_height: 2,
sequencer_height: 7,
sequencer_heights: [6, 7],
);

mount_sequencer_commit!(
test_conductor,
height: 6u32,
);

mount_sequencer_validator_set!(test_conductor, height: 5u32);

mount_sequencer_commit!(
test_conductor,
height: 7u32,
Expand All @@ -296,7 +280,7 @@ async fn skips_already_executed_heights() {
hash: [2; 64],
parent: [1; 64],
),
base_celestia_height: 2,
base_celestia_height: 1,
);

timeout(
Expand Down Expand Up @@ -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!(
Expand Down
9 changes: 5 additions & 4 deletions crates/astria-conductor/tests/blackbox/helpers/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
11 changes: 11 additions & 0 deletions crates/astria-conductor/tests/blackbox/helpers/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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] {
Copy link
Contributor Author

@SuperFluffy SuperFluffy May 29, 2024

Choose a reason for hiding this comment

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

We need a unique block hash for each sequencer height.

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())],
Expand Down
4 changes: 2 additions & 2 deletions crates/astria-conductor/tests/blackbox/soft_and_firm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ async fn simple() {
mount_celestia_blobs!(
test_conductor,
celestia_height: 1,
sequencer_height: 3,
sequencer_heights: [3],
);

mount_sequencer_commit!(
Expand Down Expand Up @@ -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!(
Expand Down