feat(papyrus_p2p_sync): handle internal block in datastreambuilder

[eitanm-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[ShahakShama]

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

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 107 at r1 (raw file):

                        })
                    );
                if internal_block_receiver.is_some() {

Do this before you send the query. not after

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 107 at r1 (raw file):

                        })
                    );
                if internal_block_receiver.is_some() {

Try to extract all of this to a function. It can be named get_internal_block and the argument should be the hashmap and the block number

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 108 at r1 (raw file):

                    );
                if internal_block_receiver.is_some() {
                    while let Some((block_number, block_data)) = internal_block_receiver.as_mut().unwrap().next().await {

You do not want to await here. You want to use now_or_never and if it returns None to go back to sending the query

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 117 at r1 (raw file):

                let end_block_number = min(end_block_number, stop_sync_at_block_number.map(|block_number| block_number.0).unwrap_or(end_block_number));
                while current_block_number.0 < end_block_number {
                    if let Some(block_data) = internal_blocks_received.remove(&current_block_number) {

My suggestion above is to not send the query if the block exists as an internal block. This means that the code here should be separate from the code that handles the response manager

[codecov]

Codecov Report

Attention: Patch coverage is 60.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 77.38%. Comparing base (e3165c4) to head (520c76b).
Report is 823 commits behind head on main.

Files with missing lines	Patch %	Lines
...ates/papyrus_p2p_sync/src/client/stream_builder.rs	58.53%	11 Missing and 6 partials ⚠️
crates/papyrus_node/src/run.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2589       +/-   ##
===========================================
+ Coverage   40.10%   77.38%   +37.27%     
===========================================
  Files          26      395      +369     
  Lines        1895    42680    +40785     
  Branches     1895    42680    +40785     
===========================================
+ Hits          760    33026    +32266     
- Misses       1100     7355     +6255     
- Partials       35     2299     +2264     

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

[ShahakShama]

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

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 117 at r1 (raw file):

Previously, ShahakShama wrote…

My suggestion above is to not send the query if the block exists as an internal block. This means that the code here should be separate from the code that handles the response manager

I think that if you got an internal block you should yield it and continue the outer loop

[eitanm-starkware]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ShahakShama)

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 107 at r1 (raw file):

Previously, ShahakShama wrote…

Do this before you send the query. not after

Done.

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 107 at r1 (raw file):

Previously, ShahakShama wrote…

Try to extract all of this to a function. It can be named get_internal_block and the argument should be the hashmap and the block number

Done.. look for load_internal_blocks

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 108 at r1 (raw file):

Previously, ShahakShama wrote…

You do not want to await here. You want to use now_or_never and if it returns None to go back to sending the query

Done.

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 117 at r1 (raw file):

Previously, ShahakShama wrote…

I think that if you got an internal block you should yield it and continue the outer loop

Done. we discussed the design in person

[ShahakShama]

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

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 60 at r3 (raw file):

        internal_blocks_received: &mut HashMap<BlockNumber, Self::Output>,
        internal_block_receiver: &mut Option<Receiver<(BlockNumber, Self::Output)>>,
        current_block_number: &BlockNumber,

Receive by value as BlockNumber derives Copy

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 64 at r3 (raw file):

        if let Some(internal_block_receiver) = internal_block_receiver {
            while let Some((block_number, block_data)) =
                internal_block_receiver.next().now_or_never().flatten()

You should expect (unwrap) the inner option instead of flattening

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 67 at r3 (raw file):

            {
                if &block_number >= current_block_number {
                    internal_blocks_received.insert(block_number, block_data);

The reason I originally wrote get_internal_block and not load_internal_blocks is that I want to stop the function if block_number is equal to current_block_number

You can still do it here and not load everything, but that makes the function unclear
Plus, IMO it's best if all the hashmap logic sits here so that if we decide to use something else we only need to change this function and maybe the initialization of the map

---

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

                    if let Some(block_data) = internal_blocks_received.remove(&current_block_number) {
                        yield Ok(Box::<dyn BlockData>::from(Box::new(block_data)));
                        current_block_number = current_block_number.unchecked_next();

As discussed, let's keep this check just before each query and continue the outer loop

[eitanm-starkware]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ShahakShama)

---

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

Previously, ShahakShama wrote…

As discussed, let's keep this check just before each query and continue the outer loop

Done.

[eitanm-starkware]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ShahakShama)

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 60 at r3 (raw file):

Previously, ShahakShama wrote…

Receive by value as BlockNumber derives Copy

Done.

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 64 at r3 (raw file):

Previously, ShahakShama wrote…

You should expect (unwrap) the inner option instead of flattening

Done.

---

crates/papyrus_p2p_sync/src/client/stream_builder.rs line 67 at r3 (raw file):

Previously, ShahakShama wrote…

The reason I originally wrote get_internal_block and not load_internal_blocks is that I want to stop the function if block_number is equal to current_block_number

You can still do it here and not load everything, but that makes the function unclear
Plus, IMO it's best if all the hashmap logic sits here so that if we decide to use something else we only need to change this function and maybe the initialization of the map

Done.

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-15T08:01:34Z
• Started at 2024-12-17T09:35:38Z