-
Notifications
You must be signed in to change notification settings - Fork 784
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Delayed RPC Send Using Tokens #5923
base: unstable
Are you sure you want to change the base?
Changes from 32 commits
0154359
d5fe64e
aab59f5
e00e679
670ec96
c0ae632
7e0c630
6322210
3947bf6
933dc00
b62537f
86cf8fb
8fd37c5
6c1015e
817ce97
7e42568
94c2493
9ad4eb7
de9d943
7adb142
627fd33
b55ffca
73e9879
cdef58d
3190d9a
19fe6b0
5a9237f
4609624
a325438
3b6edab
2ab853c
2621ce8
51247e3
5ed47b7
cbfb2ea
9f6177d
9008d3e
bd9f13c
5dbac58
ae67804
4852b20
156565c
0e1e58b
cb87af0
023c542
14ffeec
5c9e063
3c058b3
a9a675a
5d70573
4e872a0
450326c
14fb84c
636224c
f6fd85b
95f8378
9d2b263
2d7a679
b73a336
dfd092d
810c5de
d46cbe8
540436c
3d39f2c
60c9900
eec6b4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
use crate::rpc::{Protocol, SubstreamId}; | ||
use libp2p::swarm::ConnectionId; | ||
use libp2p::PeerId; | ||
use std::collections::hash_map::Entry; | ||
use std::collections::HashMap; | ||
|
||
/// Restricts more than two inbound requests from running simultaneously on the same protocol per peer. | ||
pub(super) struct ActiveRequestsLimiter { | ||
requests: HashMap<PeerId, Vec<(Protocol, ConnectionId, SubstreamId)>>, | ||
} | ||
|
||
impl ActiveRequestsLimiter { | ||
pub(super) fn new() -> Self { | ||
Self { | ||
requests: HashMap::new(), | ||
} | ||
} | ||
|
||
/// Allows if there is not a request on the same protocol. | ||
pub(super) fn allows( | ||
&mut self, | ||
peer_id: PeerId, | ||
protocol: Protocol, | ||
connection_id: &ConnectionId, | ||
substream_id: &SubstreamId, | ||
) -> bool { | ||
match self.requests.entry(peer_id) { | ||
Entry::Occupied(mut entry) => { | ||
for (p, _cid, _sid) in entry.get_mut().iter_mut() { | ||
// Check if there is a request on the same protocol. | ||
if p == &protocol { | ||
return false; | ||
} | ||
} | ||
|
||
// Request on the same protocol was not found. | ||
entry | ||
.get_mut() | ||
.push((protocol, *connection_id, *substream_id)); | ||
true | ||
} | ||
Entry::Vacant(entry) => { | ||
// No active requests for the peer. | ||
entry.insert(vec![(protocol, *connection_id, *substream_id)]); | ||
true | ||
} | ||
} | ||
} | ||
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. since this function is now just this, wdyt of having this function at the 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. That makes sense. 💡 Thanks! 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. Removed |
||
|
||
/// Removes the request with the given SubstreamId. | ||
ackintosh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pub(super) fn remove_request( | ||
&mut self, | ||
peer_id: PeerId, | ||
connection_id: &ConnectionId, | ||
substream_id: &SubstreamId, | ||
) { | ||
if let Some(requests) = self.requests.get_mut(&peer_id) { | ||
requests.retain(|(_protocol, cid, sid)| cid != connection_id && sid != substream_id); | ||
} | ||
} | ||
|
||
/// Removes the requests with the given PeerId. | ||
pub(super) fn remove_peer(&mut self, peer_id: &PeerId) { | ||
self.requests.remove(peer_id); | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
||
#[test] | ||
fn test_limiter() { | ||
let mut limiter = ActiveRequestsLimiter::new(); | ||
let peer_id = PeerId::random(); | ||
let connection_id = ConnectionId::new_unchecked(1); | ||
let substream_id = SubstreamId::new(1); | ||
|
||
assert!(limiter.allows(peer_id, Protocol::Status, &connection_id, &substream_id)); | ||
// Not allowed since a request for the same protocol is in progress. | ||
assert!(!limiter.allows(peer_id, Protocol::Status, &connection_id, &substream_id)); | ||
// Allowed since there is no BlocksByRange request in the active requests. | ||
assert!(limiter.allows( | ||
peer_id, | ||
Protocol::BlocksByRange, | ||
&connection_id, | ||
&SubstreamId::new(2) | ||
)); | ||
// Allowed since there is no request from the peer in the active requests. | ||
assert!(limiter.allows( | ||
PeerId::random(), | ||
Protocol::Status, | ||
&connection_id, | ||
&substream_id | ||
)); | ||
|
||
// Remove the Status request. | ||
limiter.remove_request(peer_id, &connection_id, &substream_id); | ||
assert!(limiter.allows( | ||
peer_id, | ||
Protocol::Status, | ||
&connection_id, | ||
&SubstreamId::new(3) | ||
)); | ||
|
||
// Remove the peer. | ||
limiter.remove_peer(&peer_id); | ||
assert!(limiter.allows( | ||
peer_id, | ||
Protocol::Status, | ||
&connection_id, | ||
&SubstreamId::new(4) | ||
)); | ||
assert!(limiter.allows( | ||
peer_id, | ||
Protocol::BlocksByRange, | ||
&connection_id, | ||
&SubstreamId::new(5) | ||
)); | ||
} | ||
} |
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.
This seems to be restricting more than one inbound request, not two.
I'm not completely sure about the intuition for allowing 2 concurrent streams, allowing a single stream per protocol makes more sense to me, I have asked in the spec PR ethereum/consensus-specs#3767 (comment)
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.
Fixed in 3c058b3#diff-7269dc8a543f95b719952593e35056791882dbe4c8019e387530ed7d0429bd7cR54-R55