Skip to content
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.

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

fix(apollo_reverts): change confusing info log #4298

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

noamsp-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

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

@noamsp-starkware noamsp-starkware marked this pull request as ready for review February 20, 2025 09:17
Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion


crates/apollo_reverts/src/lib.rs line 74 at r1 (raw file):

    info!(
        "Reverting {component_name} from storage height marker {storage_height_marker} to target \

Is this change because storage height marker is +1 than block? If yes, revert this change, rename the variables to be block_number and change revert_block_fn to be -1 than what it is right now

@noamsp-starkware noamsp-starkware force-pushed the noam.s/apollo_reverts_change_misleading_log branch from abf8d92 to 79ac53b Compare February 20, 2025 11:53
Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware)


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

/// the block.
// This function will panic if the storage reader fails to revert.
pub fn revert_block(storage_writer: &mut StorageWriter, current_block_number: BlockNumber) {

Change this to current_block_marker

@noamsp-starkware noamsp-starkware force-pushed the noam.s/apollo_reverts_change_misleading_log branch from 79ac53b to 5826a6a Compare February 20, 2025 12:04
Copy link
Contributor Author

@noamsp-starkware noamsp-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ShahakShama)


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

Previously, ShahakShama wrote…

Change this to current_block_marker

Changed to target_block_marker.

Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware)


a discussion (no related file):
@DvirYo-starkware and @Yael-Starkware should review this as well

@noamsp-starkware noamsp-starkware force-pushed the noam.s/apollo_reverts_change_misleading_log branch from 5826a6a to c605fee Compare February 20, 2025 13:51
Copy link
Contributor

@DvirYo-starkware DvirYo-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @noamsp-starkware, @ShahakShama, and @Yael-Starkware)


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

    info!(
        "Reverting {component_name} from storage height marker {storage_height_marker} to target \
  1. Here, you write marker, and in other logs, you don't explicitly say that
  2. The config value revert_up_to_and_including is not a marker, but the logs are; this a bit weird
  3. if you keep it without change, consider adding a variable that says that explicitly target_block_marker=revert_up_to_and_including,

Code quote:

storage height marker

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

         block in storage is {}.
         Starting eternal pending.",
        revert_up_to_and_including.0 - 1

If revert_up_to_and_including is 0, the number here will be -1.

Code quote:

revert_up_to_and_including.0 - 1

Copy link
Contributor Author

@noamsp-starkware noamsp-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @DvirYo-starkware, @ShahakShama, and @Yael-Starkware)


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

Previously, DvirYo-starkware wrote…
  1. Here, you write marker, and in other logs, you don't explicitly say that
  2. The config value revert_up_to_and_including is not a marker, but the logs are; this a bit weird
  3. if you keep it without change, consider adding a variable that says that explicitly target_block_marker=revert_up_to_and_including,
  1. I tried keeping the messages close to how you wrote them in consensus_manager; see code snippet 1. I'll use both height and marker.
    2/3. I agree it looks a bit weird. I'll change this as you suggested.

Code snippet:

"Batcher height marker {batcher_height_marker} is not larger than the target height marker {target_batcher_height_marker}. No reverts are needed."
"Reverting batcher from block marker {batcher_height_marker} to block marker {target_batcher_height_marker}"
"Reverting batcher block at height {batcher_height}."
"Failed to revert block at height {batcher_height} in the batcher"

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

Previously, DvirYo-starkware wrote…

If revert_up_to_and_including is 0, the number here will be -1.

My bad.
Will handle this case.

@noamsp-starkware noamsp-starkware force-pushed the noam.s/apollo_reverts_change_misleading_log branch from c605fee to 8190613 Compare February 20, 2025 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants