feat(apollo_reverts): add revert blocks functionality

[noamsp-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[noamsp-starkware]

• WIP_INTEGRATION_REVER_FLOW_TEST #4114
• test(starknet_integration_tests): add revert flow integration test #4076
• feat(starknet_integration_tests): add snapshot tx generator fn #4075
• feat(starknet_integration_tests): add update revert config #4074
• feat(starknet_state_sync): add revert_up_to_and_including option #3989
• feat(apollo_reverts): add revert blocks functionality #3988 👈 (View in Graphite)
• feat(papyrus_storage): add revert class manager marker #3987
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Artifacts upload workflows:

• Started at 2025-02-05T17:04:49Z
• Started at 2025-02-05T18:18:13Z
• Started at 2025-02-05T18:28:33Z
• Started at 2025-02-05T18:33:34Z
• Started at 2025-02-05T18:38:40Z
• Started at 2025-02-09T19:34:10Z
• Started at 2025-02-10T09:30:48Z
• Started at 2025-02-10T15:57:04Z
• Started at 2025-02-11T08:57:21Z
• Started at 2025-02-11T09:10:09Z
• Started at 2025-02-11T09:51:07Z

[ShahakShama]

Reviewed 2 of 5 files at r1, 14 of 14 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @noamsp-starkware)

---

crates/apollo_reverts/src/lib.rs line 21 at r2 (raw file):

#[derive(Clone, Debug, Serialize, Deserialize, Validate, PartialEq)]
pub struct RevertConfig {
    pub revert_on: bool,

Consider renaming to should_revert

---

crates/apollo_reverts/src/lib.rs line 42 at r2 (raw file):

                "revert_on",
                &self.revert_on,
                "Whether the component should revert blocks.",

add " and do nothing else" at the end of the sentence

---

crates/apollo_reverts/src/lib.rs line 68 at r2 (raw file):

    if current_block_number <= revert_up_to_and_including {
        panic!(
            "{component_name} current block {current_block_number} is not larger than the target \

Add 's after {component_name}

---

crates/apollo_reverts/src/lib.rs line 83 at r2 (raw file):

        );
        info!("Reverting {component_name} block number {current_block_number}.");
        revert_block_fn(current_block_number).await;

(Unrelated to the PR, but let's fix it while we're at it)
Add an additional log after calling the revert function that the block was reverted successfully

---

crates/apollo_reverts/src/lib.rs line 93 at r2 (raw file):

}

pub fn revert_block(

Add comment or documentation stating that this will revert everything related to the block and it will succeed if there's partial information for the block

---

crates/apollo_reverts/src/lib.rs line 96 at r2 (raw file):

    storage_writer: &mut StorageWriter,
    current_block_number: BlockNumber,
) -> impl Future<Output = ()> {

Change return type to nothing and return nothing. Inside sync wrap this if needed

---

crates/starknet_batcher/src/batcher.rs line 739 at r2 (raw file):

    fn revert_block(&mut self, height: BlockNumber) -> papyrus_storage::StorageResult<()> {
        block_on(apollo_reverts::revert_block(self, height));

If you take my suggestion above there's no need to call block_on

---

crates/starknet_batcher/src/batcher.rs line 739 at r2 (raw file):

    fn revert_block(&mut self, height: BlockNumber) -> papyrus_storage::StorageResult<()> {
        block_on(apollo_reverts::revert_block(self, height));

add a use statement for revert_block

[ShahakShama]

+reviewer:@DvirYo-starkware +reviewer:@Yael-Starkware

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @DvirYo-starkware, @noamsp-starkware, and @Yael-Starkware)

[noamsp-starkware]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @DvirYo-starkware, @ShahakShama, and @Yael-Starkware)

---

crates/apollo_reverts/src/lib.rs line 42 at r2 (raw file):

Previously, ShahakShama wrote…

add " and do nothing else" at the end of the sentence

Done.

---

crates/apollo_reverts/src/lib.rs line 68 at r2 (raw file):

Previously, ShahakShama wrote…

Add 's after {component_name}

Done.

---

crates/apollo_reverts/src/lib.rs line 83 at r2 (raw file):

Previously, ShahakShama wrote…

(Unrelated to the PR, but let's fix it while we're at it)
Add an additional log after calling the revert function that the block was reverted successfully

Done.

---

crates/apollo_reverts/src/lib.rs line 93 at r2 (raw file):

Previously, ShahakShama wrote…

Add comment or documentation stating that this will revert everything related to the block and it will succeed if there's partial information for the block

Done.

---

crates/apollo_reverts/src/lib.rs line 96 at r2 (raw file):

Previously, ShahakShama wrote…

Change return type to nothing and return nothing. Inside sync wrap this if needed

Done.

---

crates/starknet_batcher/src/batcher.rs line 739 at r2 (raw file):

Previously, ShahakShama wrote…

add a use statement for revert_block

Done.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.824 ms 34.861 ms 34.902 ms]
change: [-4.8484% -3.1741% -1.6382%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
2 (2.00%) high mild
3 (3.00%) high severe

[Yael-Starkware]

not sure about the name starting with apollo prefix?
will all crates use apollo prefix? if not, lets delete it from here since it is not related to the reverts.

Reviewed 9 of 14 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @DvirYo-starkware and @ShahakShama)

---

crates/apollo_reverts/src/lib.rs line 93 at r2 (raw file):

Previously, noamsp-starkware wrote…

Done.

wdym partial information?
it seems to succeed in any case ( unless panics).

---

-- commits line 5 at r3:

Suggestion:

  This commit introduces the crate apollo revets, which unifies block
  reverting functionality.

---

crates/starknet_batcher/src/batcher.rs line 738 at r3 (raw file):

    }

    fn revert_block(&mut self, height: BlockNumber) -> papyrus_storage::StorageResult<()> {

add a comment that this function panics if there is a storage failure to revert the block.

Suggestion:

fn revert_block(&mut self, height: BlockNumber) {

---

crates/starknet_consensus_manager/src/consensus_manager.rs line 143 at r3 (raw file):

                |_| panic!("Failed to revert block at height {height} in the batcher"),
            )
        };

revert_block never fails, please add a comment that it will panic if the revert fails.

Code quote:

        let revert_blocks_fn = move |height| async move {
            self.batcher_client.revert_block(RevertBlockInput { height }).await.unwrap_or_else(
                |_| panic!("Failed to revert block at height {height} in the batcher"),
            )
        };

---

crates/starknet_consensus_manager/src/consensus_manager_test.rs line 61 at r3 (raw file):

#[case::equal_block(BATCHER_HEIGHT)]
#[should_panic(expected = "Batcher's current block 10 is not larger than the target block 11. No \
                           reverts are needed.")]

lets be precise about the term since current block is confusing.

Suggestion:

#[should_panic(expected = "Batcher's storage height marker 10 is not larger than the target block 10. No \
                           reverts are needed.")]
#[case::equal_block(BATCHER_HEIGHT)]
#[should_panic(expected = "Batcher's storage height marker 10 is not larger than the target block 11. No \
                           reverts are needed.")]

---

crates/starknet_consensus_manager/src/consensus_manager_test.rs line 96 at r3 (raw file):

        Arc::new(MockStateSyncClient::new()),
        Arc::new(EmptyClassManagerClient),
    );

Suggestion:

    let consensus_manager = ConsensusManager::new(
        manager_config: ConsensusManagerConfig::default(),
        Arc::new(mock_batcher),
        Arc::new(MockStateSyncClient::new()),
        Arc::new(EmptyClassManagerClient),
    );

---

crates/apollo_reverts/src/lib.rs line 52 at r3 (raw file):

                "Whether the component should revert block and do nothing else.",
                ParamPrivacyInput::Public,
            ),

Suggestion:

            ser_param(
                "should_revert",
                &self.should_revert,
                "If set true, the component would revert blocks and do nothing else.",
                ParamPrivacyInput::Public,
            ),

---

crates/apollo_reverts/src/lib.rs line 66 at r3 (raw file):

    Fut: Future<Output = ()>,
{
    if current_block_number <= revert_up_to_and_including {

current block is not well defined.

Suggestion:

storage_height_marker

---

crates/apollo_reverts/src/lib.rs line 68 at r3 (raw file):

    if current_block_number <= revert_up_to_and_including {
        panic!(
            "{component_name}'s current block {current_block_number} is not larger than the \

Suggestion:

{component_name}'s current storage height marker {current_block_number} is not larger than the \

---

crates/apollo_reverts/src/lib.rs line 96 at r3 (raw file):

/// Reverts everything related to the block, will succeed if there is partial information for the
/// block.
pub fn revert_block(storage_writer: &mut StorageWriter, current_block_number: BlockNumber) {

add a comment that this function panics if the storage reader fails to revert

Code quote:

pub fn revert_block(storage_writer: &mut StorageWriter, current_block_number: BlockNumber) {

[noamsp-starkware]

Reviewable status: 9 of 14 files reviewed, 15 unresolved discussions (waiting on @DvirYo-starkware, @ShahakShama, and @Yael-Starkware)

---

crates/apollo_reverts/src/lib.rs line 66 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

current block is not well defined.

Done.

---

crates/apollo_reverts/src/lib.rs line 96 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

add a comment that this function panics if the storage reader fails to revert

Done.

---

crates/starknet_batcher/src/batcher.rs line 738 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

add a comment that this function panics if there is a storage failure to revert the block.

Done.

---

crates/starknet_consensus_manager/src/consensus_manager.rs line 143 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

revert_block never fails, please add a comment that it will panic if the revert fails.

Done.

---

crates/starknet_consensus_manager/src/consensus_manager_test.rs line 61 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

lets be precise about the term since current block is confusing.

Done.

---

-- commits line 5 at r3:
Done.

---

crates/apollo_reverts/src/lib.rs line 52 at r3 (raw file):

                "Whether the component should revert block and do nothing else.",
                ParamPrivacyInput::Public,
            ),

Done.

---

crates/apollo_reverts/src/lib.rs line 68 at r3 (raw file):

    if current_block_number <= revert_up_to_and_including {
        panic!(
            "{component_name}'s current block {current_block_number} is not larger than the \

Done.

---

crates/starknet_consensus_manager/src/consensus_manager_test.rs line 96 at r3 (raw file):

        Arc::new(MockStateSyncClient::new()),
        Arc::new(EmptyClassManagerClient),
    );

Done.

[Yael-Starkware]

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @DvirYo-starkware, @noamsp-starkware, and @ShahakShama)

---

crates/starknet_batcher/src/batcher.rs line 738 at r3 (raw file):

Previously, noamsp-starkware wrote…

Done.

please add the comment also to this function.
and also remove the Result return value, since this function never fails.

---

crates/starknet_consensus_manager/src/consensus_manager.rs line 143 at r3 (raw file):

Previously, noamsp-starkware wrote…

Done.

the revert block should not return a result at all.

---

crates/starknet_consensus_manager/src/consensus_manager.rs line 139 at r4 (raw file):

            .height;

        // This will panic if the revert fails.

Suggestion:

This function will panic if the revert fails.

[Yael-Starkware]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @DvirYo-starkware, @noamsp-starkware, and @ShahakShama)

---

crates/apollo_reverts/src/lib.rs line 115 at r4 (raw file):

        .unwrap()
        .commit()
        .unwrap();

all of these function calls succeed even if there is nothing to revert?

is there a test for this?

Code quote:

    storage_writer
        .begin_rw_txn()
        .unwrap()
        .revert_header(current_block_number)
        .unwrap()
        .0
        .revert_body(current_block_number)
        .unwrap()
        .0
        .revert_state_diff(current_block_number)
        .unwrap()
        .0
        .try_revert_class_manager_marker(current_block_number)
        .unwrap()
        .try_revert_base_layer_marker(current_block_number)
        .unwrap()
        .commit()
        .unwrap();

[noamsp-starkware]

crates/starknet_consensus_manager/src/consensus_manager.rs line 143 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

the revert block should not return a result at all.

Some errors can return from revert_block before calling apollo_reverts::revert_block, i suggest we print the error and panic if received.

Code snippet:

pub async fn revert_block(&mut self, input: RevertBlockInput) -> BatcherResult<()> {
        info!("Reverting block at height {}.", input.height);
        let height = self.get_height_from_storage()?.prev().ok_or(
            BatcherError::StorageHeightMarkerMismatch {
                marker_height: BlockNumber(0),
                requested_height: input.height,
            },
        )?;

        if height != input.height {
            return Err(BatcherError::StorageHeightMarkerMismatch {
                marker_height: height.unchecked_next(),
                requested_height: input.height,
            });
        }

        if let Some(height) = self.active_height {
            info!("Aborting all work on height {} due to a revert request.", height);
            self.abort_active_height().await;
        }

        self.storage_writer.revert_block(height);
        STORAGE_HEIGHT.decrement(1);
        Ok(())
    }

[Yael-Starkware]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @DvirYo-starkware, @noamsp-starkware, and @ShahakShama)

---

crates/starknet_consensus_manager/src/consensus_manager.rs line 143 at r3 (raw file):

Previously, noamsp-starkware wrote…

Some errors can return from revert_block before calling apollo_reverts::revert_block, i suggest we print the error and panic if received.

oh, you're right, the internal function is the one that panics, yes so please revert back to printing the errors.

[noamsp-starkware]

Reviewable status: 12 of 15 files reviewed, 10 unresolved discussions (waiting on @DvirYo-starkware, @ShahakShama, and @Yael-Starkware)

---

crates/apollo_reverts/src/lib.rs line 115 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

all of these function calls succeed even if there is nothing to revert?

is there a test for this?

Unless an internal storage error occurs, these should succeed (revert if a legit block number provided, otherwise do nothing and return Ok(Self))

---

crates/starknet_batcher/src/batcher.rs line 738 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

please add the comment also to this function.
and also remove the Result return value, since this function never fails.

Done.

---

crates/starknet_consensus_manager/src/consensus_manager.rs line 143 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

oh, you're right, the internal function is the one that panics, yes so please revert back to printing the errors.

Done.

---

crates/starknet_consensus_manager/src/consensus_manager.rs line 139 at r4 (raw file):

            .height;

        // This will panic if the revert fails.

Done.

[ShahakShama]

Reviewed 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @DvirYo-starkware and @Yael-Starkware)

---

crates/apollo_reverts/src/lib.rs line 93 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

wdym partial information?
it seems to succeed in any case ( unless panics).

Yes. What I meant is that it will not fail. @noamsp-starkware rephrase by writing instead of "will succeed if", "will succeed even if"

[ShahakShama]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @DvirYo-starkware and @Yael-Starkware)

[Yael-Starkware]

Reviewed 2 of 14 files at r2, 5 of 5 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @DvirYo-starkware)

---

crates/apollo_reverts/src/lib.rs line 115 at r4 (raw file):

Previously, noamsp-starkware wrote…

Unless an internal storage error occurs, these should succeed (revert if a legit block number provided, otherwise do nothing and return Ok(Self))

it there a test for these function - to check that they don't fail in case that the block_number is newer than the marker? I guess there should be a test in papyrus storage.

[Yael-Starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @DvirYo-starkware)

[noamsp-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @DvirYo-starkware)

---

crates/apollo_reverts/src/lib.rs line 115 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

it there a test for these function - to check that they don't fail in case that the block_number is newer than the marker? I guess there should be a test in papyrus storage.

Yes, these are all tested except for class_manager_marker.
I'll add a test for that in a different PR link you.