Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(batcher): mock BlockBuilder for tests #541

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Conversation

yair-starkware
Copy link
Contributor

@yair-starkware yair-starkware commented Aug 20, 2024

This change is Reviewable

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.529 ms 64.605 ms 64.698 ms]
change: [-8.9088% -5.5558% -2.6580%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
3 (3.00%) high mild
4 (4.00%) high severe

@yair-starkware yair-starkware force-pushed the yair/mock_builder branch 2 times, most recently from 3679914 to 8337ac8 Compare August 21, 2024 09:40
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.536 ms 64.626 ms 64.737 ms]
change: [-10.305% -7.0653% -4.1966%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 71.79487% with 11 lines in your changes missing coverage. Please review.

Project coverage is 5.51%. Comparing base (b0cfe82) to head (11ee294).
Report is 33 commits behind head on main.

Files with missing lines Patch % Lines
crates/batcher/src/proposal_manager.rs 71.79% 10 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (b0cfe82) and HEAD (11ee294). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (b0cfe82) HEAD (11ee294)
3 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #541       +/-   ##
==========================================
- Coverage   74.18%   5.51%   -68.68%     
==========================================
  Files         359      51      -308     
  Lines       36240    3389    -32851     
  Branches    36240    3389    -32851     
==========================================
- Hits        26886     187    -26699     
+ Misses       7220    3187     -4033     
+ Partials     2134      15     -2119     
Flag Coverage Δ
5.51% <71.79%> (-68.68%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.747 ms 64.819 ms 64.902 ms]
change: [-11.088% -7.9695% -5.2144%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.817 ms 64.887 ms 64.971 ms]
change: [-4.2153% -2.7609% -1.6668%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
3 (3.00%) high mild
2 (2.00%) high severe

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.775 ms 64.843 ms 64.922 ms]
change: [-9.4799% -6.0341% -2.9701%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
4 (4.00%) high mild
3 (3.00%) high severe

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.803 ms 64.869 ms 64.944 ms]
change: [-10.003% -6.4370% -3.2586%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
5 (5.00%) high mild
3 (3.00%) high severe

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.819 ms 64.902 ms 64.997 ms]
change: [-8.4988% -5.3723% -2.6652%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
5 (5.00%) high severe

Copy link
Collaborator

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 4 files at r2, all commit messages.
Reviewable status: 2 of 4 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware and @yair-starkware)


crates/batcher/src/proposals_manager.rs line 86 at r2 (raw file):

    /// or validated).
    proposal_in_generation: Arc<Mutex<Option<ProposalId>>>,
    // To be able to mock BlockBuilder in tests.

Suggestion:

 // Use a factory object, to be able to mock BlockBuilder in tests.

crates/batcher/src/proposals_manager.rs line 177 at r2 (raw file):

    pub sender: tokio::sync::mpsc::Sender<Transaction>,
    pub proposal_in_generation: Arc<Mutex<Option<ProposalId>>>,
    pub block_builder_factory: Arc<dyn BlockBuilderFactory>,

Consider holding an Arc<dyn BlockBuilderTrait>.

Code quote:

pub block_builder_factory: Arc<dyn BlockBuilderFactory>,

crates/batcher/src/proposals_manager_test.rs line 25 at r2 (raw file):

#[tokio::test]
async fn proposal_generation_success() {
    let proposals_manager_config = ProposalsManagerConfig::default();

Please add fixtures to create the setup for the tests.
i.e. fixture for the proposal_manager, fixture for the factory, mempool client, etc.

Code quote:

let proposals_manager_config = ProposalsManagerConfig::default();

crates/batcher/src/proposals_manager_test.rs line 26 at r2 (raw file):

async fn proposal_generation_success() {
    let proposals_manager_config = ProposalsManagerConfig::default();
    let mut block_builder_factory = MockBlockBuilderFactory::new();

Why mut?

Code quote:

let mut block_builder_factory 

crates/batcher/src/proposals_manager_test.rs line 66 at r2 (raw file):

    block_builder_factory.expect_create_block_builder().times(2).returning(|| {
        let mut mock_block_builder = MockBlockBuilderTrait::new();
        mock_block_builder.expect_add_txs().once().returning(|_thin_txs| false);

Suggestion:

_txs

crates/batcher/src/proposals_manager_test.rs line 149 at r2 (raw file):

    let mut streamed_txs = vec![];
    while let Some(tx) = tx_stream.next().await {

How do we know that the stream is over?

Code quote:

while let Some(tx) = tx_stream.next().await

Copy link
Contributor Author

@yair-starkware yair-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 5 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware and @dafnamatsry)


crates/batcher/src/proposals_manager.rs line 86 at r2 (raw file):

    /// or validated).
    proposal_in_generation: Arc<Mutex<Option<ProposalId>>>,
    // To be able to mock BlockBuilder in tests.

Done.


crates/batcher/src/proposals_manager.rs line 177 at r2 (raw file):

Previously, dafnamatsry wrote…

Consider holding an Arc<dyn BlockBuilderTrait>.

It makes it muchharder to mock consecutive calls


crates/batcher/src/proposals_manager_test.rs line 25 at r2 (raw file):

Previously, dafnamatsry wrote…

Please add fixtures to create the setup for the tests.
i.e. fixture for the proposal_manager, fixture for the factory, mempool client, etc.

Sort of done, didn't find how to do it completely with adding expectations to the mocks


crates/batcher/src/proposals_manager_test.rs line 26 at r2 (raw file):

Previously, dafnamatsry wrote…

Why mut?

Not relevant after the last change


crates/batcher/src/proposals_manager_test.rs line 66 at r2 (raw file):

    block_builder_factory.expect_create_block_builder().times(2).returning(|| {
        let mut mock_block_builder = MockBlockBuilderTrait::new();
        mock_block_builder.expect_add_txs().once().returning(|_thin_txs| false);

Not relevant after the last change


crates/batcher/src/proposals_manager_test.rs line 149 at r2 (raw file):

Previously, dafnamatsry wrote…

How do we know that the stream is over?

When it returns None

Copy link

github-actions bot commented Sep 1, 2024

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.082 ms 65.172 ms 65.276 ms]
change: [-10.475% -7.0710% -3.9521%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high severe

Copy link

github-actions bot commented Sep 1, 2024

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.693 ms 65.184 ms 65.925 ms]
change: [-8.1912% -4.9365% -2.0805%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
3 (3.00%) high mild
4 (4.00%) high severe

@yair-starkware yair-starkware changed the base branch from yair/refactor_proposal_manager to yair/rename_files September 22, 2024 12:33
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [67.174 ms 67.875 ms 68.739 ms]
change: [-8.3888% -4.9143% -1.8727%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
8 (8.00%) high severe

Copy link
Contributor Author

@yair-starkware yair-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: 1 of 7 files reviewed, 12 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)


crates/batcher/src/proposal_manager.rs line 89 at r11 (raw file):

Previously, dafnamatsry wrote…

Please move next to active_proposal.
Consider even defining a ActiveProposalInfo struct.

Done.


crates/batcher/src/proposal_manager.rs line 195 at r11 (raw file):

Previously, dafnamatsry wrote…

We can do something similar for mempool txs sender/receiver: the the receiving end will create the channel, in this case block_builder, and will return a sender.

But in the future we expect to receive a stream from the mempool and just pass it to the block builder, which is more like the current code.


crates/batcher/src/proposal_manager.rs line 198 at r11 (raw file):

Previously, dafnamatsry wrote…

I still think we should wait only on the block_builder.build_block() future.

In order to do that, the block_builder can expose an abort function that we can call if we fail to feed more mempool txs.

With the current approach, what happens to the builder if feed_more_mempool_txs fails? does it continue the execution of txs it already has? Ending the process explicitly with block_builder.abort() or block_builder.close_block() is safer and more elegant.

Talked about it (added todo in #790)


crates/batcher/src/proposals_manager.rs line 120 at r7 (raw file):

Previously, dafnamatsry wrote…

The builder limitations and requirements (execution resources, chunks size) are not the same as the mempool request limitations (network, etc).
It might be the case that they will eventually have the same values, but I think it makes sense to have different configurations for them.

Done.


crates/batcher/src/proposals_manager.rs line 142 at r8 (raw file):

Previously, dafnamatsry wrote…

See my comment above.

talked

Copy link
Collaborator

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r11, 3 of 3 files at r12, all commit messages.
Reviewable status: 5 of 7 files reviewed, 11 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)


crates/batcher/src/proposal_manager.rs line 32 at r12 (raw file):

        // TODO: Get correct default values.
        Self {
            block_builder_input_stream_bound: 100,

Suggestion:

block_next_txs_buffer_size:

crates/batcher/src/proposal_manager.rs line 250 at r12 (raw file):

            );
            for tx in mempool_txs {
                if let Err(e) = mempool_tx_sender.send(tx).await.map_err(|err| {

IIUC this can only happen if the receiving end of a channel is disconnected.
Maybe panic would be better here...

Code quote:

if let Err(e) = mempool_tx_sender.send(tx).await.map_err(|err| {

crates/batcher/src/proposal_manager.rs line 251 at r12 (raw file):

            for tx in mempool_txs {
                if let Err(e) = mempool_tx_sender.send(tx).await.map_err(|err| {
                    // TODO: should we return the rest of the txs to the mempool?

No, the mempool will be updated on decision_reached with all the txs that were included in the accepted block.

Code quote:

// TODO: should we return the rest of the txs to the mempool?

crates/batcher/src/proposal_manager_test.rs line 65 at r12 (raw file):

) {
    let n_txs = 2 * proposal_manager_config.max_txs_per_mempool_request;
    block_builder_factory.expect_create_block_builder().once().returning(move || {

Will the following work:

    let mut mock_block_builder = Arc::new(MockBlockBuilderTraitWrapper::new());
    block_builder_factory.expect_create_block_builder().once().returning(move || {
        mock_block_builder
    });
    
    mock_block_builder.expect_build_block().return_once(...);

crates/batcher/src/proposal_manager_test.rs line 77 at r12 (raw file):

                .boxed()
            },
        );

Please move to a simulate_build_block(mock_block_builder, ...) function.

Code quote:

        mock_block_builder.expect_build_block().return_once(
            move |deadline, mempool_tx_stream, output_content_sender| {
                simulate_block_builder(
                    deadline,
                    mempool_tx_stream,
                    output_content_sender,
                    Some(n_txs),
                )
                .boxed()
            },
        );

crates/batcher/src/proposal_manager_test.rs line 103 at r12 (raw file):

    assert_matches!(proposal_manager.await_active_proposal().await, Some(Ok(())));
    let v: Vec<_> = stream.collect().await;

(same below)

Suggestion:

proposal_content

crates/batcher/src/proposal_manager_test.rs line 211 at r12 (raw file):

    );
}

Please add tests for: deadline reached, mempool failures, and myabe other possible errors?

Can be done in a separate PR.

Copy link
Contributor Author

@yair-starkware yair-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: 3 of 7 files reviewed, 11 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)


crates/batcher/src/proposal_manager.rs line 32 at r12 (raw file):

        // TODO: Get correct default values.
        Self {
            block_builder_input_stream_bound: 100,

Done.


crates/batcher/src/proposal_manager.rs line 250 at r12 (raw file):

Previously, dafnamatsry wrote…

IIUC this can only happen if the receiving end of a channel is disconnected.
Maybe panic would be better here...

Don't you prefer to abort the proposal without crashing the batcher?


crates/batcher/src/proposal_manager.rs line 251 at r12 (raw file):

Previously, dafnamatsry wrote…

No, the mempool will be updated on decision_reached with all the txs that were included in the accepted block.

Done.


crates/batcher/src/proposal_manager_test.rs line 65 at r12 (raw file):

Previously, dafnamatsry wrote…

Will the following work:

    let mut mock_block_builder = Arc::new(MockBlockBuilderTraitWrapper::new());
    block_builder_factory.expect_create_block_builder().once().returning(move || {
        mock_block_builder
    });
    
    mock_block_builder.expect_build_block().return_once(...);

No, because Arc doesn't work well with mutability.
I think I can find ways to get around this, but I prefer the more explicit.


crates/batcher/src/proposal_manager_test.rs line 77 at r12 (raw file):

Previously, dafnamatsry wrote…

Please move to a simulate_build_block(mock_block_builder, ...) function.

Done.


crates/batcher/src/proposal_manager_test.rs line 103 at r12 (raw file):

Previously, dafnamatsry wrote…

(same below)

Done.


crates/batcher/src/proposal_manager_test.rs line 211 at r12 (raw file):

Previously, dafnamatsry wrote…

Please add tests for: deadline reached, mempool failures, and myabe other possible errors?

Can be done in a separate PR.

Will add in a separate PR

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [66.602 ms 66.681 ms 66.773 ms]
change: [-11.408% -5.9459% -1.4304%] (p = 0.01 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
4 (4.00%) high mild
1 (1.00%) high severe

Copy link
Collaborator

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r13, all commit messages.
Reviewable status: 5 of 7 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)


crates/batcher/src/proposal_manager.rs line 250 at r12 (raw file):

Previously, yair-starkware (Yair) wrote…

Don't you prefer to abort the proposal without crashing the batcher?

When there is a real bug in the code, I personally prefer to crash the batcher.

Copy link
Contributor

@Yael-Starkware Yael-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r12.
Reviewable status: 5 of 7 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @yair-starkware)


crates/batcher/src/proposal_manager_test.rs line 98 at r13 (raw file):

#[rstest]
#[tokio::test]
async fn concecutive_proposal_generations_success(

Suggestion:

consecutive

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [67.090 ms 67.192 ms 67.319 ms]
change: [-8.9037% -5.4487% -2.3515%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe

Copy link
Contributor Author

@yair-starkware yair-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: 1 of 7 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)


crates/batcher/src/proposal_manager.rs line 250 at r12 (raw file):

Previously, dafnamatsry wrote…

When there is a real bug in the code, I personally prefer to crash the batcher.

done


crates/batcher/src/proposal_manager_test.rs line 98 at r13 (raw file):

#[rstest]
#[tokio::test]
async fn concecutive_proposal_generations_success(

Done.

Copy link
Collaborator

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r14, all commit messages.
Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @Yael-Starkware)

Copy link
Collaborator

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@Yael-Starkware Yael-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: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)

Copy link
Contributor

@Yael-Starkware Yael-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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

Copy link
Contributor

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yair-starkware)

@yair-starkware yair-starkware merged commit e2297ba into main Sep 24, 2024
23 checks passed
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