Skip to content

Comments

apollo_propeller: add PreReconstruction phase and shard collection#12008

Closed
sirandreww-starkware wants to merge 1 commit into01-26-apollo_propeller_add_reconstruction_types_and_spawn_taskfrom
01-26-apollo_propeller_add_prereconstruction_phase_and_shard_collection
Closed

apollo_propeller: add PreReconstruction phase and shard collection#12008
sirandreww-starkware wants to merge 1 commit into01-26-apollo_propeller_add_reconstruction_types_and_spawn_taskfrom
01-26-apollo_propeller_add_prereconstruction_phase_and_shard_collection

Conversation

@sirandreww-starkware
Copy link
Contributor

@sirandreww-starkware sirandreww-starkware commented Jan 26, 2026

Note

Medium Risk
Introduces new state and concurrency around shard collection and reconstruction triggering, which could cause subtle lifecycle/ordering bugs even though the reconstruction result is not yet consumed.

Overview
Adds an explicit ReconstructionPhase::PreReconstruction state to MessageProcessor to collect validated shard units, track whether the node’s own shard index has been seen, and capture the message signature.

Once tree_manager.should_build() is satisfied, the processor now kicks off a background reconstruction via spawn_reconstruction_task, storing a pending_reconstruction oneshot receiver (result handling is not yet implemented).

Written by Cursor Bugbot for commit 12476cd. This will update automatically on new commits. Configure here.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

sirandreww-starkware commented Jan 26, 2026

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

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

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

@sirandreww-starkware sirandreww-starkware force-pushed the 01-26-apollo_propeller_add_reconstruction_types_and_spawn_task branch from f8d5a40 to fc5b531 Compare February 9, 2026 14:27
@sirandreww-starkware sirandreww-starkware force-pushed the 01-26-apollo_propeller_add_prereconstruction_phase_and_shard_collection branch from e1000a3 to 06997e2 Compare February 9, 2026 14:27
Copy link
Contributor

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

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

@guy-starkware reviewed all commit messages and made 3 comments.
Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @noamsp-starkware, @ShahakShama, and @sirandreww-starkware).


crates/apollo_propeller/src/message_processor.rs line 102 at r1 (raw file):

        let mut pending_reconstruction: Option<oneshot::Receiver<ReconstructionResult>> = None;

        // State machine: PreReconstruction -> PostReconstruction

This comment doesn't look like it fits the code under it. Maybe I'm missing something?


crates/apollo_propeller/src/message_processor.rs line 224 at r1 (raw file):

    ) {
        // Store the signature from the first unit we receive
        if signature.is_none() && !unit.signature().is_empty() {

What happens if there are units without signatures? (either one such unit or all of them)


crates/apollo_propeller/src/message_processor.rs line 239 at r1 (raw file):

        tracing::trace!("[MSG_PROC] Starting reconstruction with {} shards", received_shards.len());

        let shards = received_shards.clone();

why not send the shards in by value? Why clone?

Copy link
Contributor

@guy-starkware guy-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, 5 unresolved discussions (waiting on noamsp-starkware, ShahakShama, and sirandreww-starkware).

Copy link
Contributor

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

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

@guy-starkware reviewed 1 file.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on noamsp-starkware, ShahakShama, and sirandreww-starkware).

@sirandreww-starkware sirandreww-starkware changed the base branch from 01-26-apollo_propeller_add_reconstruction_types_and_spawn_task to graphite-base/12008 February 22, 2026 07:51
@sirandreww-starkware sirandreww-starkware force-pushed the 01-26-apollo_propeller_add_prereconstruction_phase_and_shard_collection branch from 06997e2 to 12476cd Compare February 22, 2026 09:15
Copy link
Contributor Author

@sirandreww-starkware sirandreww-starkware left a comment

Choose a reason for hiding this comment

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

@sirandreww-starkware made 5 comments and resolved 2 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on guy-starkware, noamsp-starkware, and ShahakShama).


crates/apollo_propeller/src/message_processor.rs line 102 at r1 (raw file):

Previously, guy-starkware wrote…

This comment doesn't look like it fits the code under it. Maybe I'm missing something?

You're not, mistake in split, will be solved in the next PR


crates/apollo_propeller/src/message_processor.rs line 224 at r1 (raw file):

Previously, guy-starkware wrote…

What happens if there are units without signatures? (either one such unit or all of them)

Added a comment to explain this


crates/apollo_propeller/src/message_processor.rs line 239 at r1 (raw file):

Previously, guy-starkware wrote…

why not send the shards in by value? Why clone?

you're right, using std::mem::take here

@sirandreww-starkware sirandreww-starkware changed the base branch from graphite-base/12008 to 01-26-apollo_propeller_add_reconstruction_types_and_spawn_task February 22, 2026 09:15
Copy link
Contributor

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

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

@guy-starkware reviewed 1 file and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on noamsp-starkware, ShahakShama, and sirandreww-starkware).


crates/apollo_propeller/src/message_processor.rs line 224 at r1 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

Added a comment to explain this

Is there a check somewhere that all signatures are the same for all units?

Copy link
Collaborator

@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.

@ShahakShama made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on noamsp-starkware and sirandreww-starkware).


crates/apollo_propeller/src/message_processor.rs line 136 at r2 (raw file):

                // the validator permanently lost.
                Ok(result) = async {
                    pending_validation.as_mut().unwrap().await

instead of unwrap and if is_some, do here unwrap_or(pending())

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