apollo_consensus_orchestrator: remove some panics from the validate proposal flow

[matan-starkware]

Failures here can simply result in the proposal being treated as invalid.

[reviewable-StarkWare]

This change is 

[matan-starkware]

• apollo_consensus_orchestrator: remove some panics from the validate proposal flow #8048 👈 (View in Graphite)
• apollo_consensus_orchestrator: remove some panics from context #8047
• main-v0.14.0

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

[AlonLStarkWare]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)

[AlonLStarkWare]

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

[ShahakShama]

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matan-starkware)

---

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 427 at r2 (raw file):

                ProposalStatus::Finished(id) => id,
                ProposalStatus::InvalidProposal(err) => return HandledProposalPart::Invalid(err),
                status => panic!("Unexpected status: for {proposal_id:?}, {status:?}"),

@dan-starkware WDYT? I think it makes sense to keep this panic since it should never happen. Maybe use unreachable!() instead

---

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 493 at r2 (raw file):

                ProposalStatus::Processing => HandledProposalPart::Continue,
                ProposalStatus::InvalidProposal(err) => HandledProposalPart::Invalid(err),
                status => {

@dan-starkware same here

---

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 528 at r2 (raw file):

        .await
        // This can occur if the batcher already aborted the proposal itself.
        .inspect_err(|e| warn!("Failed to send Abort to batcher: {e:?}"));

You need to check here what the error is. If it's a client error you should retry. Try and see when it's a batcher error if you can analyze if it failed due to already aborted (and then do what you do now) or due to something else (and then panic)

[dan-starkware]

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 427 at r2 (raw file):

Previously, ShahakShama wrote…

@dan-starkware WDYT? I think it makes sense to keep this panic since it should never happen. Maybe use unreachable!() instead

Agree with unreachable. Otherwise, what should we do with this propagated error?

[dan-starkware]

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 493 at r2 (raw file):

Previously, ShahakShama wrote…

@dan-starkware same here

Same as above (unreachable). Otherwise, what should we do with this propagated error?

[AlonLStarkWare]

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

---

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 427 at r2 (raw file):

Previously, dan-starkware wrote…

Agree with unreachable. Otherwise, what should we do with this propagated error?

Done

---

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 493 at r2 (raw file):

Previously, dan-starkware wrote…

Same as above (unreachable). Otherwise, what should we do with this propagated error?

Done

---

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 528 at r2 (raw file):

Previously, ShahakShama wrote…

You need to check here what the error is. If it's a client error you should retry. Try and see when it's a batcher error if you can analyze if it failed due to already aborted (and then do what you do now) or due to something else (and then panic)

Done
However from what I can tell batcher.send_proposal_content will never return a BatcherError::ProposalAborted

[ShahakShama]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @AlonLStarkWare)

---

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 531 at r3 (raw file):

    let input = SendProposalContentInput { proposal_id, content: SendProposalContent::Abort };

    const MAX_CLIENT_RETRIES: usize = 2;

1. I think you should loop eternally
2. If you disagree with 1, panic after max retries
3. If you disagree with 1, make this a config and increase it to around 10

[AlonLStarkWare]

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

---

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 531 at r3 (raw file):

Previously, ShahakShama wrote…

1. I think you should loop eternally
2. If you disagree with 1, panic after max retries
3. If you disagree with 1, make this a config and increase it to around 10

I don't like the idea of an eternal loop here.
Changed to panic after max retries, but I think this is a bit over specific to be a config. Maybe we should consider a broader refactor for client calls, so the retries and max number of retries are universal.

[ShahakShama]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AlonLStarkWare)

---

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 531 at r3 (raw file):

Previously, AlonLStarkWare (Alon-Lukatch-Starkware) wrote…

I don't like the idea of an eternal loop here.
Changed to panic after max retries, but I think this is a bit over specific to be a config. Maybe we should consider a broader refactor for client calls, so the retries and max number of retries are universal.

Yes. Could you add that as a TODO?

[AlonLStarkWare]

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

---

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 531 at r3 (raw file):

Previously, ShahakShama wrote…

Yes. Could you add that as a TODO?

Sure, who should I put this on?

[AlonLStarkWare]

Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)