refactor(batcher): create the tx provider in the batcher

[yair-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[yair-starkware]

• feat(batcher): validate l1handler txs in validate_block #1921
• feat(batcher): fetch L1Handler txs in propose #1907
• refactor(batcher): create the tx provider in the batcher #1879 👈
• feat(batcher): add l1-provider client #1870
• chore(l1-provider): use the correct tx type + fix crate package #1791
• refactor: wrap executable tx in account/l1handler enum #1788
• refactor(starknet_api): rename executable tx to account tx #1787
• main

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

Join @yair-starkware and the rest of your teammates on Graphite

[codecov]

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 3.65%. Comparing base (e3165c4) to head (abb381e).
Report is 321 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/batcher/src/batcher.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #1879       +/-   ##
==========================================
- Coverage   40.10%   3.65%   -36.45%     
==========================================
  Files          26     140      +114     
  Lines        1895   17070    +15175     
  Branches     1895   17070    +15175     
==========================================
- Hits          760     624      -136     
- Misses       1100   16402    +15302     
- Partials       35      44        +9     

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

[Yael-Starkware]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5, @dafnamatsry, and @yair-starkware)

---

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

    mock_dependencies.expect_long_build_block(1);

    let tx_provider_0 = propose_tx_provider(&mock_dependencies);

This line is repeated across all tests.

I suggest the create a function that does the following:

1. create the tx_provider
2. create the output channel
3. run proposal_manager.build_block_proposal

(can be in a separate PR).

Code quote:

let tx_provider_0 = propose_tx_provider(&mock_dependencies);

---

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

            )
            .await
            .map_err(BatcherError::from)?;

Suggestion:

        let tx_provider =  ProposeTransactionProvider {
                mempool_client: self.mempool_client.clone(),
                // TODO: use a real L1 provider client.
                l1_provider_client: Arc::new(DummyL1ProviderClient),
            };
        self.proposal_manager
            .build_block_proposal(
                build_proposal_input.proposal_id,
                build_proposal_input.retrospective_block_hash,
                deadline,
                tx_sender,
                tx_provider,
            )
            .await
            .map_err(BatcherError::from)?;

[yair-starkware]

Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @alonh5, @dafnamatsry, and @Yael-Starkware)

---

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

Previously, Yael-Starkware (YaelD) wrote…

This line is repeated across all tests.

I suggest the create a function that does the following:

1. create the tx_provider
2. create the output channel
3. run proposal_manager.build_block_proposal

(can be in a separate PR).

init_proposal_manager consumes mock_dependencies and needs to be called before starting the block proposal

---

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

            )
            .await
            .map_err(BatcherError::from)?;

Done.

[Yael-Starkware]

Reviewable status: 5 of 7 files reviewed, 3 unresolved discussions (waiting on @alonh5, @dafnamatsry, and @yair-starkware)

---

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

Previously, yair-starkware (Yair) wrote…

init_proposal_manager consumes mock_dependencies and needs to be called before starting the block proposal

right, but it actually needs to consume different fields than the tx_provider fields. Indeed requires a small refactor.

---

crates/batcher/src/proposal_manager_test.rs line 45 at r2 (raw file):

    mempool_client: Arc<MockMempoolClient>,
    storage_reader: MockBatcherStorageReaderTrait,
}

Suggestion:

struct MockDependencies {
    block_builder_factory: MockBlockBuilderFactoryTrait,
    tx_provider: Arc<MockTransactionProvider>,
    storage_reader: MockBatcherStorageReaderTrait,
}

---

crates/batcher/src/proposal_manager_test.rs line 90 at r2 (raw file):

        block_builder_factory: MockBlockBuilderFactoryTrait::new(),
        mempool_client: Arc::new(MockMempoolClient::new()),
        storage_reader,

and then you don't need the function propose_tx_provider()

Suggestion:

        l1_provider_client: Arc::new(MockL1ProviderClientTrait::new()),
        mock_transaction_provider: Arc:new(ProposeTransactionProvider {
            mempool_client: MockL1ProviderClientTrait::new(),
            l1_provider_client: MockL1ProviderClientTrait::new(),
        )}
        storage_reader,

[dafnamatsry]

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

---

crates/batcher/src/block_builder.rs line 105 at r2 (raw file):

impl BlockBuilderTrait for BlockBuilder {
    async fn build_block(
        &mut self,

Why mut?
tx_provider.get_txs is expecting &self .

Code quote:

&mut self,

---

crates/batcher/src/proposal_manager.rs line 75 at r2 (raw file):

        deadline: tokio::time::Instant,
        tx_sender: tokio::sync::mpsc::UnboundedSender<Transaction>,
        tx_provider: ProposeTransactionProvider,

Does the proposal manager needs to know that this is a ProposeTransactionProvider?
Consider using dyn TransactionProvider and than you can also use MockTransactionProvider in the tests.

Code quote:

ProposeTransactionProvider

---

crates/batcher/src/transaction_provider.rs line 74 at r2 (raw file):

pub trait L1ProviderClientTrait: Send + Sync {
    #[allow(dead_code)]
    fn get_txs(&mut self, n_txs: usize) -> Vec<L1HandlerTransaction>;

why?

Code quote:

&mut 

[yair-starkware]

Reviewable status: 5 of 7 files reviewed, 6 unresolved discussions (waiting on @alonh5, @dafnamatsry, and @Yael-Starkware)

---

crates/batcher/src/block_builder.rs line 105 at r2 (raw file):

Previously, dafnamatsry wrote…

Why mut?
tx_provider.get_txs is expecting &self .

The tx provider needs to remember how many l1handler txs were already fetched

---

crates/batcher/src/proposal_manager.rs line 75 at r2 (raw file):

Previously, dafnamatsry wrote…

Does the proposal manager needs to know that this is a ProposeTransactionProvider?
Consider using dyn TransactionProvider and than you can also use MockTransactionProvider in the tests.

I want to force the user to provide the correct TX provider.

---

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

Previously, Yael-Starkware (YaelD) wrote…

right, but it actually needs to consume different fields than the tx_provider fields. Indeed requires a small refactor.

Let's discuss and implement in a separate PR

---

crates/batcher/src/proposal_manager_test.rs line 90 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

and then you don't need the function propose_tx_provider()

Answered in previous comments

---

crates/batcher/src/transaction_provider.rs line 74 at r2 (raw file):

Previously, dafnamatsry wrote…

why?

mistake, reverted

---

crates/batcher/src/proposal_manager_test.rs line 45 at r2 (raw file):

    mempool_client: Arc<MockMempoolClient>,
    storage_reader: MockBatcherStorageReaderTrait,
}

I need to create specifically ProposeTransactionProvider, not a mock

[Yael-Starkware]

Reviewed 2 of 2 files at r2, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alonh5, @dafnamatsry, and @yair-starkware)

---

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

Previously, yair-starkware (Yair) wrote…

Let's discuss and implement in a separate PR

Sure, marked as non blocking

---

crates/batcher/src/proposal_manager_test.rs line 45 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

I need to create specifically ProposeTransactionProvider, not a mock

Ok, but still this can be computed in advance and not for every test separately.
again, can be refactored later, non-blocking.

---

crates/batcher/src/proposal_manager_test.rs line 90 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

Answered in previous comments

ack

[yair-starkware]

Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @alonh5, @dafnamatsry, and @Yael-Starkware)

---

crates/batcher/src/proposal_manager_test.rs line 45 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Ok, but still this can be computed in advance and not for every test separately.
again, can be refactored later, non-blocking.

We need a new provider for each call to build_block_proposal so I don't see how you can escape it

[dafnamatsry]

Reviewed 1 of 7 files at r1, 1 of 2 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5, @Yael-Starkware, and @yair-starkware)

---

crates/batcher/src/proposal_manager.rs line 75 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

I want to force the user to provide the correct TX provider.

Not sure there is a justification for distinguishing between propose and validate in the proposal manager anymore, now that we moved the tx_provider responsibility to the batcher.

This is non-blocking, but worth consideration IMO.

---

crates/batcher/src/proposal_manager_test.rs line 45 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

We need a new provider for each call to build_block_proposal so I don't see how you can escape it

Maybe you can create them inside build_and_await_block_proposal

[yair-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5, @dafnamatsry, and @Yael-Starkware)

---

crates/batcher/src/proposal_manager.rs line 75 at r2 (raw file):

Previously, dafnamatsry wrote…

Not sure there is a justification for distinguishing between propose and validate in the proposal manager anymore, now that we moved the tx_provider responsibility to the batcher.

This is non-blocking, but worth consideration IMO.

In my opinion, in general, it is better to restrict inputs to only the valid types.

---

crates/batcher/src/proposal_manager_test.rs line 45 at r2 (raw file):

Previously, dafnamatsry wrote…

Maybe you can create them inside build_and_await_block_proposal

There are places where we need tx_provider but don't call build_and_await_block_proposal

[alonh5]

Reviewed 2 of 7 files at r1, 2 of 2 files at r2, 1 of 2 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @Yael-Starkware)

[alonh5]

Reviewed 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @Yael-Starkware)

[dafnamatsry]

Reviewed 6 of 6 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

[yair-starkware]

Merge activity

• Nov 12, 5:54 AM EST: A user merged this pull request with Graphite.