apollo_batcher: full revert commitment flow

[yoavGrs]

No description provided.

[reviewable-StarkWare]

This change is 

[yoavGrs]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• apollo_batcher: full revert commitment flow #11496 👈 (View in Graphite)
• apollo_batcher: add revert task #11451 : 1 other dependent PR (#11516 )
• apollo_batcher: wait for revert result #11484
• apollo_batcher: perform commitment tasks #11442
• apollo_batcher: add logs to the apollo committer #11533
• main-v0.14.1-committer

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

[amosStarkware]

@amosStarkware reviewed 2 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @yoavGrs).

---

crates/apollo_batcher/src/batcher.rs line 1118 at r3 (raw file):

        self.revert_commitment(height).await;

        self.storage_writer.revert_block(height);

This function should also delete the global root, block hash, etc

Code quote:

 self.storage_writer.revert_block(height);

---

crates/apollo_batcher/src/batcher.rs line 1251 at r3 (raw file):

        self.write_commitment_results_to_storage(commitment_results)
            .await
            // Consider continuing the revert and not panicking here.

This may throw off the task offset - I don't think we want this

Code quote:

// Consider continuing the revert and not panicking here.

---

crates/apollo_batcher/src/batcher.rs line 1255 at r3 (raw file):

        self.validate_revert_task_result(revert_task_result, height).await;
        self.commitment_manager.decrease_commitment_task_offset();

should be done when adding the revert task, IMO - like it's increased when adding commitment tasks.

Code quote:

self.commitment_manager.decrease_commitment_task_offset();

---

crates/apollo_batcher/src/batcher.rs line 1262 at r3 (raw file):

        &self,
        revert_task_output: RevertTaskOutput,
        request_height: BlockNumber,

what does request mean in this context?

Code quote:

request_height

---

crates/apollo_batcher/src/batcher.rs line 1269 at r3 (raw file):

        );

        match revert_task_output.response {

Please log each branch

Code quote:

match revert_task_output.response {

---

crates/apollo_batcher/src/batcher.rs line 1271 at r3 (raw file):

        match revert_task_output.response {
            RevertBlockResponse::RevertedTo(global_root)
            | RevertBlockResponse::AlreadyReverted(global_root) => {

Why does the Committer distinguish between these two responses?
Also - how will it distinguish between AlreadyReverted & Uncommitted, once the state roots and state diff commitment will be deleted from the Committer's storage during a revert?

Code quote:

            RevertBlockResponse::RevertedTo(global_root)
            | RevertBlockResponse::AlreadyReverted(global_root) => {

---

crates/apollo_batcher/src/batcher.rs line 1280 at r3 (raw file):

                    .storage_reader
                    .global_root(new_latest_height)
                    // Consider continuing the revert and not panicking here.

If the global root doesn't exist in storage something is broken - IMO it makes sense to panic.
If you don't agree please put this in a TODO under your name

Code quote:

// Consider continuing the revert and not panicking here.