feat(sequencing): add cende module and main types

[DvirYo-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[Yael-Starkware]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/lib.rs line 12 at r1 (raw file):

/// Centralized and decentralized communication types and functionallity.
// TODO(dvir): delte this when using the types in consensus.

Suggestion:

delete

---

crates/sequencing/papyrus_consensus_orchestrator/src/lib.rs line 12 at r1 (raw file):

/// Centralized and decentralized communication types and functionallity.
// TODO(dvir): delte this when using the types in consensus.

not sure I understand,
so you mean when transforming to fully decentralized?

Code quote:

when using the types in consensus.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 11 at r1 (raw file):

/// A chunk of all the data to write to Aersopike.
#[derive(Clone, Debug)]

why clone?

Code quote:

Clone,

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 20 at r1 (raw file):

    /// Write the previous height blob to Aerospike. Returns a cell with an inner boolean indicating
    /// whether the write was successful.
    fn write_prev_height_blob(&self) -> Arc<OnceLock<bool>>;

is this variable intended to communicate when the writing to AS is done?
if so, consider using a oneshot channel

Code quote:

Arc<OnceLock<bool>>

[Yael-Starkware]

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 53 at r1 (raw file):

                debug!("Blob writing to AS completed.");
            } else {
                debug!("No blob to write to AS.");

doesn't this prevent from being a proposer?
doesn't this indicate an internal error that should cause panic? I assume the outer context takes care of it when receiving false?

Code quote:

debug!("No blob to write to AS.");

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 60 at r1 (raw file):

    }

    async fn prepare_prev_height_blob(&self, nfb: NeededForBlob) {

are these the BlockExecutionArtifacts?

Code quote:

NeededForBlob

[DvirYo-starkware]

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, and @Yael-Starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/lib.rs line 12 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

not sure I understand,
so you mean when transforming to fully decentralized?

In the next PR I will use those types in consensus so will not get cllipy unused types warning

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 11 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

why clone?

removed

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 20 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

is this variable intended to communicate when the writing to AS is done?
if so, consider using a oneshot channel

This is a bit more complex without any advantages. If the task is done and the sender is dropped, when trying to read from the receiver, an error will be returned (at least, this is what I understand from the documentation).

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 53 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

doesn't this prevent from being a proposer?
doesn't this indicate an internal error that should cause panic? I assume the outer context takes care of it when receiving false?

This prevents from being the proposer. The context will see the cell has a false value and will not send ProposalFin.
In case the node restarts, this value will be None, and only at the next height the node will have a valid value. It is fine.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 60 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

are these the BlockExecutionArtifacts?

This and more structs. see line 69

---

crates/sequencing/papyrus_consensus_orchestrator/src/lib.rs line 12 at r1 (raw file):

/// Centralized and decentralized communication types and functionallity.
// TODO(dvir): delte this when using the types in consensus.

done

[dafnamatsry]

Reviewable status: 0 of 2 files reviewed, 9 unresolved discussions (waiting on @dan-starkware, @DvirYo-starkware, and @Yael-Starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 23 at r2 (raw file):

    // Prepares the previous height blob that will be written in the next height.
    async fn prepare_prev_height_blob(&self, nfb: NeededForBlob);

If it is going to be written in the next height, maybe call it prepare_blob_for_next_height?

Code quote:

    // Prepares the previous height blob that will be written in the next height.
    async fn prepare_prev_height_blob(&self, nfb: NeededForBlob);

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 42 at r2 (raw file):

#[async_trait]
impl CendeContext for CendeAmbassador {
    fn write_prev_height_blob(&self) -> Arc<OnceLock<bool>> {

How is the caller is expected to use this API?
Why not make it a blocking function, and spawn it in the caller only if needed?

Code quote:

write_prev_height_blob(&self) -> Arc<OnceLock<bool>>

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 55 at r2 (raw file):

            debug!("Writing blob to Aerospike.");
            cell.set(true).expect("Cell should be empty");
            debug!("Blob writing to Aerospike completed.");

Set self.prev_height_blob to None if the write was successful?

[DvirYo-starkware]

Reviewable status: 0 of 2 files reviewed, 9 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, and @Yael-Starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 23 at r2 (raw file):

Previously, dafnamatsry wrote…

If it is going to be written in the next height, maybe call it prepare_blob_for_next_height?

Done.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 42 at r2 (raw file):

Previously, dafnamatsry wrote…

How is the caller is expected to use this API?
Why not make it a blocking function, and spawn it in the caller only if needed?

The next_height_blob is in the CendeAmbassador, so there is a need to get a copy of this information from the CendeAmbassador before calling the inner task. In addition, there is some additional preparation that I think should be transparent for the caller

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 55 at r2 (raw file):

Previously, dafnamatsry wrote…

Set self.prev_height_blob to None if the write was successful?

I added a to-do for that. It can be a problem if we should propose a block again at the same height for some reason (I am not sure this is possible consensus-wise). For now, I keep it like that.

[Yael-Starkware]

Reviewable status: 0 of 2 files reviewed, 9 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 20 at r1 (raw file):

Previously, DvirYo-starkware wrote…

This is a bit more complex without any advantages. If the task is done and the sender is dropped, when trying to read from the receiver, an error will be returned (at least, this is what I understand from the documentation).

Afaiu. The receiver will first consume the message , and then get an error.
see example in the docs: https://doc.servo.org/tokio/sync/oneshot/index.html

I think the code will be simpler and more self explanatory with a channel.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 52 at r2 (raw file):

                return;
            };
            // TODO(dvir): write blob to AS.

this is a blocking operation, right?

Code quote:

// TODO(dvir): write blob to AS.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 61 at r2 (raw file):

    }

    async fn prepare_prev_height_blob(&self, nfb: NeededForBlob) {

This is being called during the previous height, right? so during this time it is the current height

Suggestion:

prepare_blob_for_central_write

[Yael-Starkware]

Reviewable status: 0 of 2 files reviewed, 9 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 52 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

this is a blocking operation, right?

maybe state it clearly in the TODO so we won't forget

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 61 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

This is being called during the previous height, right? so during this time it is the current height

oh never mind, I see that Dafna already commented on this.

[Yael-Starkware]

Reviewable status: 0 of 2 files reviewed, 9 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 70 at r3 (raw file):

#[derive(Clone, Debug, Default)]
pub(crate) struct NeededForBlob {

Suggestion:

BlobInput

[Yael-Starkware]

Reviewable status: 0 of 2 files reviewed, 9 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 42 at r2 (raw file):

Previously, DvirYo-starkware wrote…

The next_height_blob is in the CendeAmbassador, so there is a need to get a copy of this information from the CendeAmbassador before calling the inner task. In addition, there is some additional preparation that I think should be transparent for the caller

can we consume the CendeContext when calling this spawn, or should it be available for retries in case of failure?

[DvirYo-starkware]

Reviewable status: 0 of 2 files reviewed, 9 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, and @Yael-Starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 20 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Afaiu. The receiver will first consume the message , and then get an error.
see example in the docs: https://doc.servo.org/tokio/sync/oneshot/index.html

I think the code will be simpler and more self explanatory with a channel.

Done

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 42 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

can we consume the CendeContext when calling this spawn, or should it be available for retries in case of failure?

We can't consume. We should be able to retry.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 52 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

maybe state it clearly in the TODO so we won't forget

This fine.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 61 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

This is being called during the previous height, right? so during this time it is the current height

changed to Dafna's suggestion prepare\_blob\_for\_next\_height

[Yael-Starkware]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, and @DvirYo-starkware)

[Yael-Starkware]

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 76 at r4 (raw file):

impl From<NeededForBlob> for AerospikeBlob {
    fn from(_nfb: NeededForBlob) -> Self {

blob_input

Code quote:

nfb