refactor(network): db executor communicates through channels

[ShahakShama]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 63.05732% with 58 lines in your changes missing coverage. Please review.

Project coverage is 65.78%. Comparing base (3675c53) to head (a4415af).

Files	Patch %	Lines
crates/papyrus_node/src/main.rs	28.12%	22 Missing and 1 partial ⚠️
crates/papyrus_network/src/db_executor/mod.rs	64.51%	13 Missing and 9 partials ⚠️
crates/papyrus_network/src/network_manager/mod.rs	79.36%	11 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2120      +/-   ##
==========================================
- Coverage   67.25%   65.78%   -1.48%     
==========================================
  Files         136      136              
  Lines       17889    17900      +11     
  Branches    17889    17900      +11     
==========================================
- Hits        12031    11775     -256     
- Misses       4532     4839     +307     
+ Partials     1326     1286      -40     

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

[ShahakShama]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion

---

crates/papyrus_network/src/network_manager/mod.rs line 3 at r1 (raw file):

mod swarm_trait;

// TODO(shahak): Uncomment

Do not merge until all the PR stack is merged so that this will be uncommented

[eitanm-starkware]

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @ShahakShama)

---

crates/papyrus_network/src/network_manager/mod.rs line 307 at r1 (raw file):

                    Protocol::try_from(protocol_name).expect("Encountered unknown protocol");
                let internal_query = protocol.bytes_query_to_protobuf_request(query);
                match protocol {

let (sender, receiver) = match protocol etc.

to reduce duplication

[ShahakShama]

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

---

crates/papyrus_network/src/network_manager/mod.rs line 307 at r1 (raw file):

Previously, eitanm-starkware wrote…

let (sender, receiver) = match protocol etc.

to reduce duplication

This code is erased in #2122 . Is it ok if I don't fix this now to avoid rebases? or do you prefer I fix it nonetheless?

[eitanm-starkware]

Reviewed 1 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion

---

crates/papyrus_network/src/network_manager/mod.rs line 307 at r1 (raw file):

Previously, ShahakShama wrote…

This code is erased in #2122 . Is it ok if I don't fix this now to avoid rebases? or do you prefer I fix it nonetheless?

yeah

[asmaastarkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @eitanm-starkware)

[matan-starkware]

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

---

-- commits line 2 at r1:
I feel like making it generic is more important (or at least thinking that way made it easier for me to grok the PR)

Suggestion:

- ecb746d: refactor(network): make db executor generic over FetchBlockDataFromDb

This allows us to remove the concrete `Data` type.

---

crates/papyrus_network/src/db_executor/test.rs line 52 at r1 (raw file):

                }
            }
        }

WDYT?

Suggestion:

        mut all_data = data_receiver.collect::<Vec<_>>() => {
            assert_eq!(DataOrFin(None), all_data.pop().unwrap());
            for (i, data) in all_data.into_iter().enumerate() {
                assert_eq!(data.0.unwrap().block_header.block_number.0, i as u64);
            }
        }

---

crates/papyrus_network/src/network_manager/mod.rs line 44 at r1 (raw file):

    header_buffer_size: usize,
    sqmr_inbound_response_receivers:
        StreamHashMap<InboundSessionId, BoxStream<'static, Option<Bytes>>>,

Should we move this stream to utilize the new Stream design we discussed? As in move the concept of streaming entirely out of papyrus_network.

Code quote:

    sqmr_inbound_response_receivers:
        StreamHashMap<InboundSessionId, BoxStream<'static, Option<Bytes>>>,

---

crates/papyrus_network/src/network_manager/mod.rs line 288 at r1 (raw file):

    fn handle_sqmr_behaviour_event(&mut self, event: sqmr::behaviour::ExternalEvent) {
        match event {

This function is nearly 100 lines. Can we have each branch call to a method?

Code quote:

        match event {

---

crates/papyrus_network/src/network_manager/mod.rs line 335 at r1 (raw file):

                    }
                }
            }

Is this going to remain in the future? with explicit handling for each protocol

Code quote:

                match protocol {
                    Protocol::SignedBlockHeader => {
                        let (sender, receiver) = futures::channel::mpsc::channel::<
                            DataOrFin<SignedBlockHeader>,
                        >(self.header_buffer_size);
                        self.db_executor.register_query(internal_query, sender);
                        self.sqmr_inbound_response_receivers.insert(
                            inbound_session_id,
                            receiver
                                .map(|data| Some(Bytes::from(data)))
                                .chain(stream::once(ready(None)))
                                .boxed(),
                        );
                    }
                    Protocol::StateDiff => {
                        let (sender, receiver) = futures::channel::mpsc::channel::<
                            DataOrFin<StateDiffChunk>,
                        >(self.header_buffer_size);
                        self.db_executor.register_query(internal_query, sender);
                        self.sqmr_inbound_response_receivers.insert(
                            inbound_session_id,
                            receiver
                                .map(|data| Some(Bytes::from(data)))
                                .chain(stream::once(ready(None)))
                                .boxed(),
                        );
                    }
                }
            }

[ShahakShama]

Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on @asmaastarkware, @eitanm-starkware, and @matan-starkware)

---

-- commits line 2 at r1:

Previously, matan-starkware wrote…

I feel like making it generic is more important (or at least thinking that way made it easier for me to grok the PR)

I didn't understand

---

crates/papyrus_network/src/network_manager/mod.rs line 44 at r1 (raw file):

Previously, matan-starkware wrote…

Should we move this stream to utilize the new Stream design we discussed? As in move the concept of streaming entirely out of papyrus_network.

No. SG is based on a specific protobuf message that groups messages to streams. Also, here the responses are tied to some specific query in the networking level

---

crates/papyrus_network/src/network_manager/mod.rs line 288 at r1 (raw file):

Previously, matan-starkware wrote…

This function is nearly 100 lines. Can we have each branch call to a method?

Added a TODO for now because I'm afraid of this conflicting with the future PRs

---

crates/papyrus_network/src/network_manager/mod.rs line 335 at r1 (raw file):

Previously, matan-starkware wrote…

Is this going to remain in the future? with explicit handling for each protocol

No

[matan-starkware]

Reviewed 1 of 1 files at r2.
Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @asmaastarkware, @eitanm-starkware, and @ShahakShama)

---

-- commits line 2 at r1:

Previously, ShahakShama wrote…

I didn't understand

I don't think deleting Data is the most important thing. I think making db executor generic is.

---

crates/papyrus_network/src/network_manager/mod.rs line 44 at r1 (raw file):

Previously, ShahakShama wrote…

No. SG is based on a specific protobuf message that groups messages to streams. Also, here the responses are tied to some specific query in the networking level

Doesn't block the PR but I don't understand why papyrus_network sometimes handles streams and sometimes doesn't. It would make more sense to me if the networking layer either has the generic capacity to handle a stream or no capacity and we always use SG.

---

crates/papyrus_protobuf/src/converters/state_diff.rs line 94 at r2 (raw file):

auto_impl_into_and_try_from_vec_u8!(DataOrFin<StateDiffChunk>, protobuf::StateDiffsResponse);

auto_impl_into_and_try_from_vec_u8!(DataOrFin<StateDiffChunk>, protobuf::StateDiffsResponse);

Suggestion:

auto_impl_into_and_try_from_vec_u8!(DataOrFin<StateDiffChunk>, protobuf::StateDiffsResponse);

[ShahakShama]

Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @asmaastarkware, @eitanm-starkware, and @matan-starkware)

---

-- commits line 2 at r1:

Previously, matan-starkware wrote…

I don't think deleting Data is the most important thing. I think making db executor generic is.

Oh. Changed the PR title. I prefer not touching the commits themselves and they will dissappear when we merge anyway

---

crates/papyrus_network/src/network_manager/mod.rs line 44 at r1 (raw file):

Previously, matan-starkware wrote…

Doesn't block the PR but I don't understand why papyrus_network sometimes handles streams and sometimes doesn't. It would make more sense to me if the networking layer either has the generic capacity to handle a stream or no capacity and we always use SG.

I'll be happy to explain in huddle :)

---

crates/papyrus_protobuf/src/converters/state_diff.rs line 94 at r2 (raw file):

auto_impl_into_and_try_from_vec_u8!(DataOrFin<StateDiffChunk>, protobuf::StateDiffsResponse);

auto_impl_into_and_try_from_vec_u8!(DataOrFin<StateDiffChunk>, protobuf::StateDiffsResponse);

Yes, I fixed it already. This was what caused the tests to fail

[matan-starkware]

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

---

crates/papyrus_network/src/network_manager/mod.rs line 44 at r1 (raw file):

Previously, ShahakShama wrote…

I'll be happy to explain in huddle :)

Honestly I think I need this written out. You've tried to explain this to me a couple times already

[matan-starkware]

Reviewed 1 of 3 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @eitanm-starkware)

[asmaastarkware]

Reviewable status: 1 of 9 files reviewed, 1 unresolved discussion (waiting on @eitanm-starkware and @matan-starkware)

[eitanm-starkware]

Reviewable status: 1 of 9 files reviewed, 1 unresolved discussion