feat(batcher): implement get_proposal_content as part of the validate_proposal flow

[Yael-Starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[Yael-Starkware]

• feat(batcher): add validate support to build_block #1872
• feat(batcher): implement get_proposal_content as part of the validate_proposal flow #1867 👈
• main

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

Join @Yael-Starkware and the rest of your teammates on Graphite

[codecov]

Codecov Report

Attention: Patch coverage is 7.27273% with 51 lines in your changes missing coverage. Please review.

Project coverage is 3.60%. Comparing base (e3165c4) to head (6492d2b).
Report is 239 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/batcher/src/batcher.rs	10.81%	33 Missing ⚠️
crates/batcher/src/proposal_manager.rs	0.00%	18 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #1867       +/-   ##
==========================================
- Coverage   40.10%   3.60%   -36.50%     
==========================================
  Files          26     139      +113     
  Lines        1895   16717    +14822     
  Branches     1895   16717    +14822     
==========================================
- Hits          760     603      -157     
- Misses       1100   16074    +14974     
- Partials       35      40        +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yair-starkware]

Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @alonh5 and @Yael-Starkware)

---

crates/batcher/src/batcher.rs line 49 at r1 (raw file):

struct ValidateProposalContext {
    tx_provider_sender: ValidateTxProviderSender,
}

Consider having an enum + 1 container instead of 2 stucts and 2 containers

Code quote:

struct BuildProposalContext {
    output_tx_stream: OutputStream,
}

struct ValidateProposalContext {
    tx_provider_sender: ValidateTxProviderSender,
}

---

crates/batcher/src/batcher.rs line 122 at r1 (raw file):

    }

    // This function assumes that there is no parallel request handling, otherwise the content could

That requests are received in order.

Perhaps we need to add an order identifier to the request

Code quote:

there is no parallel request handling

---

crates/batcher/src/batcher.rs line 148 at r1 (raw file):

    ) -> BatcherResult<SendProposalContentResponse> {
        match self.proposal_manager.take_proposal_result(proposal_id).await {
            // Block builder is still processing, no errors received.

There is also the possibility that the client sent a non-existing proposal id.
You should use a different function for checking the proposal status.
The take function is for decision_reached IIRC

Code quote:

// Block builder is still processing, no errors received.

---

crates/batcher/src/batcher.rs line 287 at r1 (raw file):

// TODO: Make this work with streams.
type OutputStream = tokio::sync::mpsc::UnboundedReceiver<Transaction>;
type ValidateTxProviderSender = tokio::sync::mpsc::Sender<Transaction>;

Why not InputStream?

Code quote:

ValidateTxProviderSender

---

crates/batcher/src/batcher.rs line 287 at r1 (raw file):

// TODO: Make this work with streams.
type OutputStream = tokio::sync::mpsc::UnboundedReceiver<Transaction>;
type ValidateTxProviderSender = tokio::sync::mpsc::Sender<Transaction>;

Both streams should be bounded or unbounded - I think that unbounded is better in this case.

Code quote:

Sender

---

crates/batcher/src/proposal_manager.rs line 208 at r1 (raw file):

        proposal_id: ProposalId,
    ) -> ProposalResult<ProposalCommitment> {
        self.wait_for_active_proposal_completion().await;

This function can be used to stream proposal content while a different proposal is being executed.
Shouldn't be waiting for the completion

Code quote:

self.wait_for_active_proposal_completion().await;

[yair-starkware]

Reviewed 2 of 3 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @alonh5 and @Yael-Starkware)

[Yael-Starkware]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alonh5 and @yair-starkware)

---

crates/batcher/src/batcher.rs line 49 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Consider having an enum + 1 container instead of 2 stucts and 2 containers

I thought about it , but didn't do it because of the following:
These contexts are used by different functions, there is no place where they are both needed.
If I'd use an enum then each time I'd read the proposalContext, I'll have to verify that it is of the correct type (validate/propose).

---

crates/batcher/src/batcher.rs line 122 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

That requests are received in order.

Perhaps we need to add an order identifier to the request

if they are received in order, why do we need an identifier?

---

crates/batcher/src/batcher.rs line 148 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

There is also the possibility that the client sent a non-existing proposal id.
You should use a different function for checking the proposal status.
The take function is for decision_reached IIRC

I changed it to get_proposal_status, now it is more explicit.
also it protects against a erroneous consensus server that calls send_proposal_content after the proposal was finished.

---

crates/batcher/src/batcher.rs line 287 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Why not InputStream?

done.

---

crates/batcher/src/batcher.rs line 287 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Both streams should be bounded or unbounded - I think that unbounded is better in this case.

I think that since the stream is coming from outside , we need to bound it to prevent a DoS attack.

---

crates/batcher/src/proposal_manager.rs line 208 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

This function can be used to stream proposal content while a different proposal is being executed.
Shouldn't be waiting for the completion

added a proposal_id param.
done.