chore(sync): receive statediffchunk from network #2118

eitanm-starkware · 2024-06-18T12:49:04Z

This change is

ShahakShama

Reviewed 3 of 8 files at r1, all commit messages.
Reviewable status: 3 of 8 files reviewed, all discussions resolved

codecov · 2024-06-19T11:05:05Z

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 66.00%. Comparing base (cd6f096) to head (94340b3).

Files	Patch %	Lines
crates/papyrus_p2p_sync/src/client/state_diff.rs	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2118      +/-   ##
==========================================
+ Coverage   65.86%   66.00%   +0.14%     
==========================================
  Files         136      136              
  Lines       18113    18102      -11     
  Branches    18113    18102      -11     
==========================================
+ Hits        11930    11949      +19     
+ Misses       4888     4858      -30     
  Partials     1295     1295

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

ShahakShama

Reviewed 4 of 8 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @eitanm-starkware)

crates/papyrus_p2p_sync/src/state_diff.rs line 48 at r3 (raw file):

    #[latency_histogram("p2p_sync_state_diff_parse_data_for_block_latency_seconds", true)]
    fn parse_data_for_block<'a>(
        state_diff_chunks_receiver: &'a mut DataReceiver,

This rename is unnecessary. in the sync server we call it just state_diff
Whatever you prefer

crates/papyrus_p2p_sync/src/state_diff.rs line 124 at r3 (raw file):

                chunk_deployed_contract.insert(contract_diff.contract_address, class_hash);
            }
            unite_state_diffs_field(&mut state_diff.deployed_contracts, chunk_deployed_contract)?;

The function unite_state_diffs_field was only useful in the old code. Now it will be cleaner to remove this function and just call state_diff.deployed_contracts.insert(...) and return error if the return value is some
Alternatively, you can create a function like unite_state_diffs_field that receives only 1 entry from the chunk

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

        create_block_hashes_and_signatures(HEADER_QUERY_LENGTH.try_into().unwrap());
    let mut rng = get_rng();
    let state_diffs = (0..HEADER_QUERY_LENGTH)

This means just one state diff per header. Add a 3rd constant for NUM_CHUNKS_PER_BLOCK and have each element in the vector be a vector of chunks

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

                assert_eq!(block_number.unchecked_next(), txn.get_state_marker().unwrap());

                let expected_state_diff = txn.get_state_diff(block_number).unwrap().unwrap();

Swap the names. This is state_diff/actual_state_diff, and the one below is expected_state_diff

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

                let expected_state_diff = txn.get_state_diff(block_number).unwrap().unwrap();
                let state_diff = match state_diff_chunk {

Consider moving to hardcoded state diff chunks and expected state diff instead

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

}

async fn validate_state_diff_fails(

Move this and the function below it to either the bottom of the file or the top of the file

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

}

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

remove pub

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

#[tokio::test]
async fn state_diff_not_splitted_correctly() {

Why did you erase this test? You can do it by having a ContractDiff of size 2

ShahakShama

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

crates/papyrus_p2p_sync/src/client/state_diff.rs line 121 at r4 (raw file):

            }
            unite_state_diffs_field(&mut state_diff.nonces, chunk_nonce)?;
            match state_diff.storage_diffs.get_mut(&contract_diff.contract_address) {

Do this only if !contract_diff.storage_diff.is_empty()

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

                assert_eq!(block_number.unchecked_next(), txn.get_state_marker().unwrap());

                let expected_state_diff = txn.get_state_diff(block_number).unwrap().unwrap();

Replace names. See comment below in the old file

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

                let expected_state_diff = txn.get_state_diff(block_number).unwrap().unwrap();
                let state_diff = match state_diff_chunk {

Add a comment that each chunk is a separate state diff, and add a TODO to test the positive flow of multiple chunks per block

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

#[tokio::test]
async fn state_diff_not_splitted_correctly() {

Why did you remove this test? You can change the 2nd chunk to be a contract diff with class_hash and nonce

crates/papyrus_p2p_sync/src/state_diff.rs line 124 at r3 (raw file):