This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Implement request-responses protocols #6634
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
849a623
Implement request-responses protocols
tomaka de1efcc
Add tests
tomaka 2b774e7
Fix sc-cli
tomaka 418bb61
Apply suggestions from code review
tomaka 9083df9
Fix naming
tomaka 44c0203
Fix other issues
tomaka d0d9810
Other naming fix
tomaka 47fa2ac
Fix error logging
tomaka 13ed831
Max sizes to u64
tomaka 3f914b7
Don't kill connections on refusal to process
tomaka 12878bb
Adjust comment
tomaka de492a2
Merge remote-tracking branch 'upstream/master' into request-responses
tomaka 0dd28e7
Merge remote-tracking branch 'upstream/master' into request-responses
tomaka 66e04c1
Merge remote-tracking branch 'upstream/master' into request-responses
tomaka 83977da
Merge remote-tracking branch 'upstream/master' into request-responses
tomaka 43a82d5
Merge remote-tracking branch 'upstream/master' into request-responses
tomaka File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -16,7 +16,7 @@ | |||||
|
||||||
use crate::{ | ||||||
config::{ProtocolId, Role}, block_requests, light_client_handler, finality_requests, | ||||||
peer_info, discovery::{DiscoveryBehaviour, DiscoveryConfig, DiscoveryOut}, | ||||||
peer_info, request_responses, discovery::{DiscoveryBehaviour, DiscoveryConfig, DiscoveryOut}, | ||||||
protocol::{message::{self, Roles}, CustomMessageOutcome, NotificationsSink, Protocol}, | ||||||
ObservedRole, DhtEvent, ExHashT, | ||||||
}; | ||||||
|
@@ -39,6 +39,10 @@ use std::{ | |||||
time::Duration, | ||||||
}; | ||||||
|
||||||
pub use crate::request_responses::{ | ||||||
ResponseFailure, InboundFailure, RequestFailure, OutboundFailure, RequestId, SendRequestError | ||||||
}; | ||||||
|
||||||
/// General behaviour of the network. Combines all protocols together. | ||||||
#[derive(NetworkBehaviour)] | ||||||
#[behaviour(out_event = "BehaviourOut<B>", poll_method = "poll")] | ||||||
|
@@ -50,6 +54,8 @@ pub struct Behaviour<B: BlockT, H: ExHashT> { | |||||
peer_info: peer_info::PeerInfoBehaviour, | ||||||
/// Discovers nodes of the network. | ||||||
discovery: DiscoveryBehaviour, | ||||||
/// Generic request-reponse protocols. | ||||||
request_responses: request_responses::RequestResponsesBehaviour, | ||||||
/// Block request handling. | ||||||
block_requests: block_requests::BlockRequests<B>, | ||||||
/// Finality proof request handling. | ||||||
|
@@ -76,22 +82,40 @@ pub enum BehaviourOut<B: BlockT> { | |||||
RandomKademliaStarted(ProtocolId), | ||||||
|
||||||
/// We have received a request from a peer and answered it. | ||||||
AnsweredRequest { | ||||||
/// | ||||||
/// This event is generated for statistics purposes. | ||||||
InboundRequest { | ||||||
/// Peer which sent us a request. | ||||||
peer: PeerId, | ||||||
/// Protocol name of the request. | ||||||
protocol: String, | ||||||
/// Time it took to build the response. | ||||||
build_time: Duration, | ||||||
protocol: Cow<'static, str>, | ||||||
/// If `Ok`, contains the time elapsed between when we received the request and when we | ||||||
/// sent back the response. If `Err`, the error that happened. | ||||||
result: Result<Duration, ResponseFailure>, | ||||||
}, | ||||||
|
||||||
/// A request initiated using [`Behaviour::send_request`] has succeeded or failed. | ||||||
RequestFinished { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Would be easier for me to understand without having to read the doc comment. |
||||||
/// Request that has succeeded. | ||||||
request_id: RequestId, | ||||||
/// Response sent by the remote or reason for failure. | ||||||
result: Result<Vec<u8>, RequestFailure>, | ||||||
}, | ||||||
|
||||||
/// Started a new request with the given node. | ||||||
RequestStarted { | ||||||
/// | ||||||
/// This event is for statistics purposes only. The request and response handling are entirely | ||||||
/// internal to the behaviour. | ||||||
OpaqueRequestStarted { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Would that not be more descriptive? Would as well match with |
||||||
peer: PeerId, | ||||||
/// Protocol name of the request. | ||||||
protocol: String, | ||||||
}, | ||||||
/// Finished, successfully or not, a previously-started request. | ||||||
RequestFinished { | ||||||
/// | ||||||
/// This event is for statistics purposes only. The request and response handling are entirely | ||||||
/// internal to the behaviour. | ||||||
OpaqueRequestFinished { | ||||||
/// Who we were requesting. | ||||||
peer: PeerId, | ||||||
/// Protocol name of the request. | ||||||
|
@@ -161,17 +185,20 @@ impl<B: BlockT, H: ExHashT> Behaviour<B, H> { | |||||
finality_proof_requests: finality_requests::FinalityProofRequests<B>, | ||||||
light_client_handler: light_client_handler::LightClientHandler<B>, | ||||||
disco_config: DiscoveryConfig, | ||||||
) -> Self { | ||||||
Behaviour { | ||||||
request_response_protocols: Vec<request_responses::ProtocolConfig>, | ||||||
) -> Result<Self, request_responses::RegisterError> { | ||||||
Ok(Behaviour { | ||||||
substrate, | ||||||
peer_info: peer_info::PeerInfoBehaviour::new(user_agent, local_public_key), | ||||||
discovery: disco_config.finish(), | ||||||
request_responses: | ||||||
request_responses::RequestResponsesBehaviour::new(request_response_protocols.into_iter())?, | ||||||
block_requests, | ||||||
finality_proof_requests, | ||||||
light_client_handler, | ||||||
events: VecDeque::new(), | ||||||
role, | ||||||
} | ||||||
}) | ||||||
} | ||||||
|
||||||
/// Returns the list of nodes that we know exist in the network. | ||||||
|
@@ -208,6 +235,16 @@ impl<B: BlockT, H: ExHashT> Behaviour<B, H> { | |||||
self.peer_info.node(peer_id) | ||||||
} | ||||||
|
||||||
/// Initiates sending a request. | ||||||
/// | ||||||
/// An error is returned if we are not connected to the target peer of if the protocol doesn't | ||||||
/// match one that has been registered. | ||||||
pub fn send_request(&mut self, target: &PeerId, protocol: &str, request: Vec<u8>) | ||||||
-> Result<RequestId, SendRequestError> | ||||||
{ | ||||||
self.request_responses.send_request(target, protocol, request) | ||||||
} | ||||||
|
||||||
/// Registers a new notifications protocol. | ||||||
/// | ||||||
/// Please call `event_stream` before registering a protocol, otherwise you may miss events | ||||||
|
@@ -298,18 +335,18 @@ Behaviour<B, H> { | |||||
CustomMessageOutcome::BlockRequest { target, request } => { | ||||||
match self.block_requests.send_request(&target, request) { | ||||||
block_requests::SendRequestOutcome::Ok => { | ||||||
self.events.push_back(BehaviourOut::RequestStarted { | ||||||
self.events.push_back(BehaviourOut::OpaqueRequestStarted { | ||||||
peer: target, | ||||||
protocol: self.block_requests.protocol_name().to_owned(), | ||||||
}); | ||||||
}, | ||||||
block_requests::SendRequestOutcome::Replaced { request_duration, .. } => { | ||||||
self.events.push_back(BehaviourOut::RequestFinished { | ||||||
self.events.push_back(BehaviourOut::OpaqueRequestFinished { | ||||||
peer: target.clone(), | ||||||
protocol: self.block_requests.protocol_name().to_owned(), | ||||||
request_duration, | ||||||
}); | ||||||
self.events.push_back(BehaviourOut::RequestStarted { | ||||||
self.events.push_back(BehaviourOut::OpaqueRequestStarted { | ||||||
peer: target, | ||||||
protocol: self.block_requests.protocol_name().to_owned(), | ||||||
}); | ||||||
|
@@ -358,18 +395,39 @@ Behaviour<B, H> { | |||||
} | ||||||
} | ||||||
|
||||||
impl<B: BlockT, H: ExHashT> NetworkBehaviourEventProcess<request_responses::Event> for Behaviour<B, H> { | ||||||
fn inject_event(&mut self, event: request_responses::Event) { | ||||||
match event { | ||||||
request_responses::Event::InboundRequest { peer, protocol, result } => { | ||||||
self.events.push_back(BehaviourOut::InboundRequest { | ||||||
peer, | ||||||
protocol, | ||||||
result, | ||||||
}); | ||||||
} | ||||||
|
||||||
request_responses::Event::RequestFinished { request_id, result } => { | ||||||
self.events.push_back(BehaviourOut::RequestFinished { | ||||||
request_id, | ||||||
result, | ||||||
}); | ||||||
}, | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
impl<B: BlockT, H: ExHashT> NetworkBehaviourEventProcess<block_requests::Event<B>> for Behaviour<B, H> { | ||||||
fn inject_event(&mut self, event: block_requests::Event<B>) { | ||||||
match event { | ||||||
block_requests::Event::AnsweredRequest { peer, total_handling_time } => { | ||||||
self.events.push_back(BehaviourOut::AnsweredRequest { | ||||||
self.events.push_back(BehaviourOut::InboundRequest { | ||||||
peer, | ||||||
protocol: self.block_requests.protocol_name().to_owned(), | ||||||
build_time: total_handling_time, | ||||||
protocol: self.block_requests.protocol_name().to_owned().into(), | ||||||
result: Ok(total_handling_time), | ||||||
}); | ||||||
}, | ||||||
block_requests::Event::Response { peer, original_request: _, response, request_duration } => { | ||||||
self.events.push_back(BehaviourOut::RequestFinished { | ||||||
self.events.push_back(BehaviourOut::OpaqueRequestFinished { | ||||||
peer: peer.clone(), | ||||||
protocol: self.block_requests.protocol_name().to_owned(), | ||||||
request_duration, | ||||||
|
@@ -381,7 +439,7 @@ impl<B: BlockT, H: ExHashT> NetworkBehaviourEventProcess<block_requests::Event<B | |||||
block_requests::Event::RequestTimeout { peer, request_duration, .. } => { | ||||||
// There doesn't exist any mechanism to report cancellations or timeouts yet, so | ||||||
// we process them by disconnecting the node. | ||||||
self.events.push_back(BehaviourOut::RequestFinished { | ||||||
self.events.push_back(BehaviourOut::OpaqueRequestFinished { | ||||||
peer: peer.clone(), | ||||||
protocol: self.block_requests.protocol_name().to_owned(), | ||||||
request_duration, | ||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do protocol names have to be strings instead of bytes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While libp2p allows bytes, I went for restricting this to strings in Substrate because:
maybe_utf8_bytes_to_string
).