feat(network): add get blocks inbound response logic

[nagmo-starkware]

Pull Request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build-related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

• Yes
• No

Other information

---

This change is 

[nagmo-starkware]

Current dependencies on/for this PR:

• main
  • PR feat(network): add block messages through protobuf (#1116) #1130
    • PR feat(network): protobuf update #1232
      • PR feat(network): add block headers protocol #1248
      • PR chore(network): change the way protobuf types are being imported to improve readability #1233
    • PR feat(network): db_executor #1173
    • PR fix(network): nake behaviour generic #1152
    • PR feat(network): network POC with some of it's components #1147
    • PR feat(network): add get blocks inbound response logic #1131 👈
    • PR feat(network): add get_blocks::Handler with simple test #1123

This comment was auto-generated by Graphite.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (p2p@433fe13). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff           @@
##             p2p    #1131   +/-   ##
======================================
  Coverage       ?   74.02%           
======================================
  Files          ?       78           
  Lines          ?     6873           
  Branches       ?     6873           
======================================
  Hits           ?     5088           
  Misses         ?      934           
  Partials       ?      851           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ShahakShama]

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

---

crates/papyrus_network/src/get_blocks/protocol.rs line 22 at r1 (raw file):

/// Receives a request to get a range of blocks and sends a stream of data on the blocks.
pub struct InboundProtocol {
    request_relay_sender: UnboundedSender<GetBlocks>,

Change type to oneshot channel
https://docs.rs/futures/latest/futures/channel/oneshot/struct.Receiver.html

---

crates/papyrus_network/src/get_blocks/protocol.rs line 23 at r1 (raw file):

pub struct InboundProtocol {
    request_relay_sender: UnboundedSender<GetBlocks>,
    response_relay_receiver: UnboundedReceiver<Option<GetBlocksResponse>>,

response_relay_receiver -> responses_relay_receiver

---

crates/papyrus_network/src/get_blocks/protocol.rs line 23 at r1 (raw file):

pub struct InboundProtocol {
    request_relay_sender: UnboundedSender<GetBlocks>,
    response_relay_receiver: UnboundedReceiver<Option<GetBlocksResponse>>,

explain the meaning of None in comment

---

crates/papyrus_network/src/get_blocks/protocol.rs line 39 at r1 (raw file):

#[derive(thiserror::Error, Debug)]
pub enum InboundProtocolError {

Consider uniting with OutboundProtocolError

---

crates/papyrus_network/src/get_blocks/protocol.rs line 64 at r1 (raw file):

    fn upgrade_inbound(mut self, mut io: Stream, _: Self::Info) -> Self::Future {
        Box::pin(

Why did you pin it? explain in a comment

---

crates/papyrus_network/src/get_blocks/protocol.rs line 69 at r1 (raw file):

                    self.request_relay_sender.unbounded_send(get_blocks_msg)?;
                }
                let mut expect_end_of_stream = false;

This can be a lot more simpler if you return when you receive Some(None) and you return error when you receive None. In that case, you don't need this variable

---

crates/papyrus_network/src/get_blocks/protocol.rs line 72 at r1 (raw file):

                loop {
                    match self.response_relay_receiver.next().await {
                        Some(response) => match response {

If response_relay_receiver.next() didn't return None, assert that expect_end_of_stream is false (return error if it isn't)
(Assuming you didn't erase expect_end_of_stream as suggested above)

---

crates/papyrus_network/src/get_blocks/protocol.rs line 73 at r1 (raw file):

                    match self.response_relay_receiver.next().await {
                        Some(response) => match response {
                            Some(res) => write_message(res, &mut io).await?,

flatten into one match with the following arms instead
Some(Some(response))
Some(None)
None

---

crates/papyrus_network/src/get_blocks/protocol.rs line 167 at r1 (raw file):

}

use crate::messages::block::BlockHeader;

Move this to protocol_test (or test_utils file if you use it somewhere else

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 60 at r1 (raw file):

    let (outbound_protocol, mut responses_receiver) = OutboundProtocol::new(Default::default());
    let (inbound_protocol, (mut request_relay_receiver, response_relay_sender)) =

Consider prefixing each channel with outbound/inbound

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 73 at r1 (raw file):

                .unwrap();
        },
        // plays the role of the network and DB handlers

We're still not sure about the names of these yet. Write something more generic like
// Sends responses to the inbound protocol and receives responses from the outbound protocol

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 75 at r1 (raw file):

        // plays the role of the network and DB handlers
        async move {
            // ignore block query for now, just send hardcoded responses

Why ignore? Assert that the received request is the request that the outbound sent

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 76 at r1 (raw file):

        async move {
            // ignore block query for now, just send hardcoded responses
            let _blocks_query = request_relay_receiver.next().await.unwrap();

Rename to _request

[ShahakShama]

Reviewed 1 of 3 files at r1.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @nagmo-starkware)

---

crates/papyrus_network/src/get_blocks/handler.rs line 120 at r1 (raw file):

    fn listen_protocol(&self) -> SubstreamProtocol<Self::InboundProtocol, Self::InboundOpenInfo> {
        let (inbound_protocol, _) = InboundProtocol::new();

Add TODO to do something with _

[nagmo-starkware]

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

---

crates/papyrus_network/src/get_blocks/handler.rs line 120 at r1 (raw file):

Previously, ShahakShama wrote…

Add TODO to do something with _

Done.

---

crates/papyrus_network/src/get_blocks/protocol.rs line 22 at r1 (raw file):

Previously, ShahakShama wrote…

Change type to oneshot channel
https://docs.rs/futures/latest/futures/channel/oneshot/struct.Receiver.html

Done.

---

crates/papyrus_network/src/get_blocks/protocol.rs line 23 at r1 (raw file):

Previously, ShahakShama wrote…

response_relay_receiver -> responses_relay_receiver

Done.

---

crates/papyrus_network/src/get_blocks/protocol.rs line 23 at r1 (raw file):

Previously, ShahakShama wrote…

explain the meaning of None in comment

what none?

---

crates/papyrus_network/src/get_blocks/protocol.rs line 39 at r1 (raw file):

Previously, ShahakShama wrote…

Consider uniting with OutboundProtocolError

I'm not sure about it. I say keep it like that for now. adding a todo to not forget about it

---

crates/papyrus_network/src/get_blocks/protocol.rs line 64 at r1 (raw file):

Previously, ShahakShama wrote…

Why did you pin it? explain in a comment

compiler said so :)

---

crates/papyrus_network/src/get_blocks/protocol.rs line 69 at r1 (raw file):

Previously, ShahakShama wrote…

This can be a lot more simpler if you return when you receive Some(None) and you return error when you receive None. In that case, you don't need this variable

Done.

---

crates/papyrus_network/src/get_blocks/protocol.rs line 72 at r1 (raw file):

Previously, ShahakShama wrote…

If response_relay_receiver.next() didn't return None, assert that expect_end_of_stream is false (return error if it isn't)
(Assuming you didn't erase expect_end_of_stream as suggested above)

Done.

---

crates/papyrus_network/src/get_blocks/protocol.rs line 73 at r1 (raw file):

Previously, ShahakShama wrote…

flatten into one match with the following arms instead
Some(Some(response))
Some(None)
None

Done.

---

crates/papyrus_network/src/get_blocks/protocol.rs line 167 at r1 (raw file):

Previously, ShahakShama wrote…

Move this to protocol_test (or test_utils file if you use it somewhere else

I prefer to leave it like that for now. next we'll move the channel to the handler and then the behavior and we'll move it in the end to the DB handler.

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 73 at r1 (raw file):

Previously, ShahakShama wrote…

We're still not sure about the names of these yet. Write something more generic like
// Sends responses to the inbound protocol and receives responses from the outbound protocol

I don't think it conveys the same meaning. I want the reader to understand that this part is the part of the p2p handler (network handler) and the DB reader.
I think that it's fine even if we rename it later.

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 75 at r1 (raw file):

Previously, ShahakShama wrote…

Why ignore? Assert that the received request is the request that the outbound sent

Done.

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 76 at r1 (raw file):

Previously, ShahakShama wrote…

Rename to _request

Done.

[ShahakShama]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @nagmo-starkware)

---

crates/papyrus_network/src/get_blocks/protocol.rs line 23 at r1 (raw file):

Previously, nagmo-starkware wrote…

what none?

What does it mean when response_relay_receiver received None

---

crates/papyrus_network/src/get_blocks/protocol.rs line 39 at r1 (raw file):

Previously, nagmo-starkware wrote…

I'm not sure about it. I say keep it like that for now. adding a todo to not forget about it

Very well, then add the TODO

---

crates/papyrus_network/src/get_blocks/protocol.rs line 64 at r1 (raw file):

Previously, nagmo-starkware wrote…

compiler said so :)

Understand what the compiler said and explain that in a comment

---

crates/papyrus_network/src/get_blocks/protocol.rs line 167 at r1 (raw file):

Previously, nagmo-starkware wrote…

I prefer to leave it like that for now. next we'll move the channel to the handler and then the behavior and we'll move it in the end to the DB handler.

I think that it's better to remove it and if we'll want it in the DB handler then we'll take care of that. but I'm ok with leaving it if that's what you think

---

crates/papyrus_network/src/get_blocks/protocol.rs line 122 at r2 (raw file):

    #[error(transparent)]
    IOError(#[from] io::Error),
    #[error(transparent)]

Change here the error message in the same way you did with InboundProtocolError

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 76 at r1 (raw file):

Previously, nagmo-starkware wrote…

Done.

We'll might change this protocol to GetHeaders so IMO it's better to simply call this request, but whatever you prefer

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 60 at r2 (raw file):

    let (outbound_protocol, mut outbound_responses_relay_receiver) =
        OutboundProtocol::new(Default::default());

Instead of calling default here, create a variable for the request and set the variable to default and use it here and in the assertion below

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 204 at r2 (raw file):

        // plays the role of the network handler
        async move {
            let _blocks_query = inbound_request_relay_receiver.await.unwrap();

Rename to _request or _blocks_request

[nagmo-starkware]

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

---

crates/papyrus_network/src/get_blocks/protocol.rs line 23 at r1 (raw file):

Previously, ShahakShama wrote…

What does it mean when response_relay_receiver received None

oh got it. I think it's very obvious from the usage. look at write_message(

---

crates/papyrus_network/src/get_blocks/protocol.rs line 39 at r1 (raw file):

Previously, ShahakShama wrote…

Very well, then add the TODO

here: // TODO(nevo): consider consolidating with InboundProtocolError

---

crates/papyrus_network/src/get_blocks/protocol.rs line 64 at r1 (raw file):

Previously, ShahakShama wrote…

Understand what the compiler said and explain that in a comment

due to recent changes it's unneeded anymore

---

crates/papyrus_network/src/get_blocks/protocol.rs line 122 at r2 (raw file):

Previously, ShahakShama wrote…

Change here the error message in the same way you did with InboundProtocolError

Done.

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 76 at r1 (raw file):

Previously, ShahakShama wrote…

We'll might change this protocol to GetHeaders so IMO it's better to simply call this request, but whatever you prefer

I think we need to call it data since we'll eventually generalize it to be used to more the just the blocks :)
I think let's not waste effort in trying to make everything perfect right away. we'll modify as needed as we progress

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 60 at r2 (raw file):

Previously, ShahakShama wrote…

Instead of calling default here, create a variable for the request and set the variable to default and use it here and in the assertion below

ok I'm fine with it. but since it's a work in progress and will definitely change soon I think it's nitpicking

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 204 at r2 (raw file):

Previously, ShahakShama wrote…

Rename to _request or _blocks_request

Done.

[ShahakShama]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nagmo-starkware)

---

crates/papyrus_network/src/get_blocks/protocol.rs line 23 at r1 (raw file):

Previously, nagmo-starkware wrote…

oh got it. I think it's very obvious from the usage. look at write_message(

I get it, but right now we're not at the usage. I think it will be clearer to document it here.
Now that I think of it, the correct way to document it is to write documentation for the new method and describe the sender end
As we agreed on slack, you can do this in a later PR

---

crates/papyrus_network/src/get_blocks/protocol_test.rs line 76 at r1 (raw file):

Previously, nagmo-starkware wrote…

I think we need to call it data since we'll eventually generalize it to be used to more the just the blocks :)
I think let's not waste effort in trying to make everything perfect right away. we'll modify as needed as we progress

Ok, but I think that request/response are good names even when we generalize

[github-actions]

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale.
This PR will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!