test: add p2p class sync happy flow test

[noamsp-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[noamsp-starkware]

• test: add p2p class sync fail cases tests #1943
• test: add p2p class sync happy flow test #1942 👈 (View in Graphite)
• refactor: add wait_for_marker to p2p_sync:client test_utils #2147 : 1 other dependent PR (#2154 )
• main

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 26.39%. Comparing base (e3165c4) to head (7329d44).
Report is 599 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1942       +/-   ##
===========================================
- Coverage   40.10%   26.39%   -13.72%     
===========================================
  Files          26      105       +79     
  Lines        1895    12644    +10749     
  Branches     1895    12644    +10749     
===========================================
+ Hits          760     3337     +2577     
- Misses       1100     8720     +7620     
- Partials       35      587      +552     

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

[ShahakShama]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @noamsp-starkware)

---

crates/papyrus_protobuf/src/converters/class.rs line 285 at r1 (raw file):

        });

        // This length check and default option is needed for ContractClass test instances that

For next time: IMO this can be a separate preliminary PR

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 39 at r1 (raw file):

    // Asserting the constants so the test can assume there will be 2 state diff queries for a
    // single header query and the second will be smaller than the first.
    const_assert!(STATE_DIFF_QUERY_LENGTH < HEADER_QUERY_LENGTH);

Since we're not testing state diffs, we don't need this assertion

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 53 at r1 (raw file):

    } = setup();

    let block_hashes_and_signatures =

Please extract all common code between this test and the state diff test
You can use this opportunity to also remove code duplication between state diff test and
Suggestion for the function:

pub fn run_state_diff_sync(
    header_state_diff_lengths: Vec<usize>,
    state_diff_chunks: Vec<usize>
) -> GenericReceiver<ClassTestPayload> {
    ...
}

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 192 at r1 (raw file):

}

fn create_random_state_diff_chunk(rng: &mut ChaCha8Rng) -> StateDiffChunk {

Unite these two functions into create_random_state_diff_chunk_with_class. This way, you'll get rid of the unreachable section

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 194 at r1 (raw file):

fn create_random_state_diff_chunk(rng: &mut ChaCha8Rng) -> StateDiffChunk {
    let class_hash = ClassHash(rng.next_u64().into());
    if rng.next_u32() % 2 == 0 {

use rng.gen_bool() instead (from the Rng trait)

[ShahakShama]

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

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 53 at r1 (raw file):

Previously, ShahakShama wrote…

Please extract all common code between this test and the state diff test
You can use this opportunity to also remove code duplication between state diff test and
Suggestion for the function:

pub fn run_state_diff_sync(
    header_state_diff_lengths: Vec<usize>,
    state_diff_chunks: Vec<usize>
) -> GenericReceiver<ClassTestPayload> {
    ...
}

*between state diff positive flow test and negative flow test

[noamsp-starkware]

Reviewable status: 2 of 5 files reviewed, 5 unresolved discussions (waiting on @ShahakShama)

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 39 at r1 (raw file):

Previously, ShahakShama wrote…

Since we're not testing state diffs, we don't need this assertion

Done.

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 53 at r1 (raw file):

Previously, ShahakShama wrote…

*between state diff positive flow test and negative flow test

Done.

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 192 at r1 (raw file):

Previously, ShahakShama wrote…

Unite these two functions into create_random_state_diff_chunk_with_class. This way, you'll get rid of the unreachable section

Done.

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 194 at r1 (raw file):

Previously, ShahakShama wrote…

use rng.gen_bool() instead (from the Rng trait)

Done.

---

crates/papyrus_protobuf/src/converters/class.rs line 285 at r1 (raw file):

Previously, ShahakShama wrote…

For next time: IMO this can be a separate preliminary PR

I agree. will do next time.

[ShahakShama]

Reviewed 1 of 3 files at r2.
Reviewable status: 3 of 5 files reviewed, 20 unresolved discussions (waiting on @noamsp-starkware)

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 76 at r3 (raw file):

    // Create a future that will receive queries, send responses and validate the results.
    let parse_queries_future = async move {

Rename it to test_future

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 84 at r3 (raw file):

        tokio::join! {
            run_state_diff_sync_custom(

Put it inside the select in the end of the test (have 3 futures in that select)

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 290 at r3 (raw file):

async fn validate_state_diff_fails(
    header_state_diff_lengths: Vec<usize>,
    state_diffs: Vec<Option<StateDiffChunk>>,

Keep the name state_diff_chunks

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 338 at r3 (raw file):

}

async fn run_state_diff_sync_custom(

add doc

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 338 at r3 (raw file):

}

async fn run_state_diff_sync_custom(

change the word custom to through_channel

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 342 at r3 (raw file):

    mock_state_diff_response_manager: &mut GenericReceiver<StateDiffTestPayload>,
    header_state_diff_lengths: Vec<usize>,
    state_diff_chunk_receiver: &mut mpsc::Receiver<Option<StateDiffChunk>>,

You can import mpsc::Receiver directly and remove the mpsc:: part. If you saw other cases in the code that reference Receiver with mpsc:: it's because there's also a different type or Receiver in the same file.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 343 at r3 (raw file):

    header_state_diff_lengths: Vec<usize>,
    state_diff_chunk_receiver: &mut mpsc::Receiver<Option<StateDiffChunk>>,
) -> Option<StateDiffTestPayload> {

Remove the return value and instead have a bool argument called should_assert_reported

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 344 at r3 (raw file):

    state_diff_chunk_receiver: &mut mpsc::Receiver<Option<StateDiffChunk>>,
) -> Option<StateDiffTestPayload> {
    let n_headers = header_state_diff_lengths.len();

n_headers -> num_headers

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 350 at r3 (raw file):

    let mut mock_header_responses_manager = mock_header_response_manager.next().await.unwrap();

    // Send headers for entire query.

Either document as assumption that the query length is >= num_headers, assert that, or perform queries in a loop until the sum of query sizes reaches num_headers

I think I prefer the latter. It can also be quite easy if you keep the current loop structure and add a condition that if
i == sum_query_sizes then you receive a new query and reset the mock_header_responses_manager

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 354 at r3 (raw file):

        block_hashes_and_signatures.iter().zip(header_state_diff_lengths.iter()).enumerate()
    {
        // Send responses

Send header responses

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 402 at r3 (raw file):

                step: 1,
            })),
            "If the limit of the query is too low, try to increase \

Remove the sleep and instead wait until header marker reaches where it should reach as we discussed be-al pe. You can do this in a separate PR (I think you should) and add a TODO instead

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 407 at r3 (raw file):

        let mut current_state_diff_length = 0;
        let destination_state_diff_length = (start_block_number..start_block_number + limit)

Try instead header_state_diff_lengths[start_block_number..start_block_number + limit].sum() or if that doesn't work maybe header_state_diff_lengths[start_block_number..start_block_number + limit].iter().sum()

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 426 at r3 (raw file):

            }

            // return if sent an empty/none state diff chunk

Why do you return here and not continue sending for the next query?

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 431 at r3 (raw file):

        // return if sent too many state diffs
        if current_state_diff_length != destination_state_diff_length {

Why do you return here and not send more state diffs

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 436 at r3 (raw file):

        assert_eq!(current_state_diff_length, destination_state_diff_length);
        mock_state_diff_responses_manager.send_response(DataOrFin(None)).await.unwrap();

Remove this and have the None come from the caller of the function

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 450 at r3 (raw file):

) {
    let (state_diff_sender, mut state_diff_receiver) = mpsc::channel(config.buffer_size);
    tokio::join! {

Use select instead of join

[noamsp-starkware]

Reviewable status: 2 of 5 files reviewed, 18 unresolved discussions (waiting on @ShahakShama)

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 76 at r3 (raw file):

Previously, ShahakShama wrote…

Rename it to test_future

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 84 at r3 (raw file):

Previously, ShahakShama wrote…

Put it inside the select in the end of the test (have 3 futures in that select)

spoken offline, decided to keep join but move it into the select since we want to verify that both functions terminate successfully.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 290 at r3 (raw file):

Previously, ShahakShama wrote…

Keep the name state_diff_chunks

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 338 at r3 (raw file):

Previously, ShahakShama wrote…

change the word custom to through_channel

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 338 at r3 (raw file):

Previously, ShahakShama wrote…

add doc

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 342 at r3 (raw file):

Previously, ShahakShama wrote…

You can import mpsc::Receiver directly and remove the mpsc:: part. If you saw other cases in the code that reference Receiver with mpsc:: it's because there's also a different type or Receiver in the same file.

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 343 at r3 (raw file):

Previously, ShahakShama wrote…

Remove the return value and instead have a bool argument called should_assert_reported

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 344 at r3 (raw file):

Previously, ShahakShama wrote…

n_headers -> num_headers

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 350 at r3 (raw file):

Previously, ShahakShama wrote…

Either document as assumption that the query length is >= num_headers, assert that, or perform queries in a loop until the sum of query sizes reaches num_headers

I think I prefer the latter. It can also be quite easy if you keep the current loop structure and add a condition that if
i == sum_query_sizes then you receive a new query and reset the mock_header_responses_manager

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 354 at r3 (raw file):

Previously, ShahakShama wrote…

Send header responses

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 407 at r3 (raw file):

Previously, ShahakShama wrote…

Try instead header_state_diff_lengths[start_block_number..start_block_number + limit].sum() or if that doesn't work maybe header_state_diff_lengths[start_block_number..start_block_number + limit].iter().sum()

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 426 at r3 (raw file):

Previously, ShahakShama wrote…

Why do you return here and not continue sending for the next query?

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 431 at r3 (raw file):

Previously, ShahakShama wrote…

Why do you return here and not send more state diffs

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 436 at r3 (raw file):

Previously, ShahakShama wrote…

Remove this and have the None come from the caller of the function

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 450 at r3 (raw file):

Previously, ShahakShama wrote…

Use select instead of join

Spoken offline, decided to keep this join

[ShahakShama]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @noamsp-starkware)

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 50 at r4 (raw file):

    let mut rng = get_rng();
    let (class_state_diffs, api_contract_classes): (Vec<_>, Vec<_>) = (0..HEADER_QUERY_LENGTH)

add TODO to add multiple state diffs per header

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 104 at r4 (raw file):

                let block_number = BlockNumber(block_number);

                // Check that before we've sent all parts the state diff wasn't written yet

update comment to fit into class test

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 123 at r4 (raw file):

                let txn = storage_reader.begin_ro_txn().unwrap();
                let expected_class = match api_contract_class {

rename api_contract_class to expected_class and rename expected_class to actual_class

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 149 at r4 (raw file):

}

trait GetClassHash {

Why do you declare this as a trait? you can simply write impl StateDiffChunk {...

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 353 at r4 (raw file):

    // split the headers into queries of size HEADER_QUERY_LENGTH and send headers for each query
    for header_querie in block_hashes_and_signatures

header_querie -> headers_for_current_query

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 354 at r4 (raw file):

    // split the headers into queries of size HEADER_QUERY_LENGTH and send headers for each query
    for header_querie in block_hashes_and_signatures
        .iter()

use into_iter and it will save you all the reference mess

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 360 at r4 (raw file):

        .chunks(HEADER_QUERY_LENGTH.try_into().unwrap())
    {
        let mut mock_header_responses_manager = mock_header_response_manager.next().await.unwrap();

Until we improve the naming here (it's a task that I plan for Alon to do), please add a comment that you're receiving the next query from sync

[noamsp-starkware]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ShahakShama)

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 50 at r4 (raw file):

Previously, ShahakShama wrote…

add TODO to add multiple state diffs per header

Ack

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 104 at r4 (raw file):

Previously, ShahakShama wrote…

update comment to fit into class test

Ack

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 123 at r4 (raw file):

Previously, ShahakShama wrote…

rename api_contract_class to expected_class and rename expected_class to actual_class

Ack

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 149 at r4 (raw file):

Previously, ShahakShama wrote…

Why do you declare this as a trait? you can simply write impl StateDiffChunk {...

I wanted this function only locally for this test. I can't do it as you suggested in this file since StateDiffChunk is not defined here. We can use this function by either using it how I wrote it or defining it at StateDiffChunk's impl at papyrus_protobuf/src/sync.rs.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 353 at r4 (raw file):

Previously, ShahakShama wrote…

header_querie -> headers_for_current_query

Done.

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 354 at r4 (raw file):

Previously, ShahakShama wrote…

use into_iter and it will save you all the reference mess

The reference mess occurs because I'm using chunk() splitting. into_iter will only help the header_state_diff_lengths vector, which I need to use later in the test, so I'll need to clone it before using into_iter on it. I think keeping it as before is simpler and more cost-effective. What do you think?

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 360 at r4 (raw file):

Previously, ShahakShama wrote…

Until we improve the naming here (it's a task that I plan for Alon to do), please add a comment that you're receiving the next query from sync

Done.

[ShahakShama]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @noamsp-starkware)

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 149 at r4 (raw file):

Previously, noamsp-starkware wrote…

I wanted this function only locally for this test. I can't do it as you suggested in this file since StateDiffChunk is not defined here. We can use this function by either using it how I wrote it or defining it at StateDiffChunk's impl at papyrus_protobuf/src/sync.rs.

Very well. Write down a comment saying that (that you define a new trait here in order to just add a method to GetClassHash because it comes from another crate)

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 354 at r4 (raw file):

Previously, noamsp-starkware wrote…

The reference mess occurs because I'm using chunk() splitting. into_iter will only help the header_state_diff_lengths vector, which I need to use later in the test, so I'll need to clone it before using into_iter on it. I think keeping it as before is simpler and more cost-effective. What do you think?

cost is not a concern here. The main concern is readability.
How about .cloned() at the end?

[noamsp-starkware]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ShahakShama)

---

crates/papyrus_p2p_sync/src/client/class_test.rs line 149 at r4 (raw file):

Previously, ShahakShama wrote…

Very well. Write down a comment saying that (that you define a new trait here in order to just add a method to GetClassHash because it comes from another crate)

Ack

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 354 at r4 (raw file):

Previously, ShahakShama wrote…

cost is not a concern here. The main concern is readability.
How about .cloned() at the end?

Cloned() doesn't work easily because it requires type annotations. This workaround reduces all reference levels.

Code snippet:

for headers_for_current_query in block_hashes_and_signatures
        .into_iter()
        .zip(header_state_diff_lengths.clone().into_iter())
        .enumerate()
        .collect::<Vec<_>>()
        .chunks(HEADER_QUERY_LENGTH.try_into().unwrap())
        .map(Vec::from)

[noamsp-starkware]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ShahakShama)

---

crates/papyrus_p2p_sync/src/client/state_diff_test.rs line 354 at r4 (raw file):

Previously, noamsp-starkware wrote…

Cloned() doesn't work easily because it requires type annotations. This workaround reduces all reference levels.

If the map(Vec::from) at the end looks weird to you, I can just remove the ref level like this:

for &(i, ((block_hash, block_signature), header_state_diff_length)) in

headers_for_current_query

[ShahakShama]

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

[ShahakShama]

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