-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix packet count when talking with PAR2 peers #8555
Changes from 7 commits
d629b1b
06285ea
c6eca6d
87560bb
eca6cfe
a2ab110
7b744c9
4dc10be
12b06d6
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 |
---|---|---|
|
@@ -75,14 +75,17 @@ const RECALCULATE_COSTS_INTERVAL: Duration = Duration::from_secs(60 * 60); | |
// minimum interval between updates. | ||
const UPDATE_INTERVAL: Duration = Duration::from_millis(5000); | ||
|
||
/// Packet count for PIP. | ||
const PACKET_COUNT_V1: u8 = 9; | ||
|
||
/// Supported protocol versions. | ||
pub const PROTOCOL_VERSIONS: &'static [u8] = &[1]; | ||
pub const PROTOCOL_VERSIONS: &'static [(u8, u8)] = &[ | ||
(1, PACKET_COUNT_V1), | ||
]; | ||
|
||
/// Max protocol version. | ||
pub const MAX_PROTOCOL_VERSION: u8 = 1; | ||
|
||
/// Packet count for PIP. | ||
pub const PACKET_COUNT: u8 = 9; | ||
|
||
// packet ID definitions. | ||
mod packet { | ||
|
@@ -111,9 +114,9 @@ mod packet { | |
mod timeout { | ||
use std::time::Duration; | ||
|
||
pub const HANDSHAKE: Duration = Duration::from_millis(2500); | ||
pub const ACKNOWLEDGE_UPDATE: Duration = Duration::from_millis(5000); | ||
pub const BASE: u64 = 1500; // base timeout for packet. | ||
pub const HANDSHAKE: Duration = Duration::from_millis(4_000); | ||
pub const ACKNOWLEDGE_UPDATE: Duration = Duration::from_millis(5_000); | ||
pub const BASE: u64 = 2500; // base timeout for packet. | ||
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. I think number format should be consistent there. Either change this to 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. While we're in here, perhaps s/BASE/BASE_TIMEOUT/? 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. @dvdplm we use it always including module name, so it's 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. fair enough |
||
|
||
// timeouts per request within packet. | ||
pub const HEADERS: u64 = 250; // per header? | ||
|
@@ -688,7 +691,7 @@ impl LightProtocol { | |
Err(e) => { punish(*peer, io, e); return } | ||
}; | ||
|
||
if PROTOCOL_VERSIONS.iter().find(|x| **x == proto_version).is_none() { | ||
if PROTOCOL_VERSIONS.iter().find(|x| x.0 == proto_version).is_none() { | ||
punish(*peer, io, Error::UnsupportedProtocolVersion(proto_version)); | ||
return; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,7 @@ use super::{ | |
BlockSet, | ||
ChainSync, | ||
PeerAsking, | ||
ETH_PACKET_COUNT, | ||
ETH_PROTOCOL_VERSION_63, | ||
GET_BLOCK_BODIES_PACKET, | ||
GET_BLOCK_HEADERS_PACKET, | ||
GET_RECEIPTS_PACKET, | ||
|
@@ -140,7 +140,8 @@ impl SyncRequester { | |
} | ||
peer.asking = asking; | ||
peer.ask_time = Instant::now(); | ||
let result = if packet_id >= ETH_PACKET_COUNT { | ||
// TODO [ToDr] This seems quite fragile. Be careful when protocol is updated. | ||
let result = if packet_id >= ETH_PROTOCOL_VERSION_63.1 { | ||
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. Perhaps a test here to help our future selves? 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. Logged a separate issue. |
||
io.send_protocol(WARP_SYNC_PROTOCOL_ID, peer_id, packet_id, packet) | ||
} else { | ||
io.send(peer_id, packet_id, packet) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,7 +79,9 @@ const NODE_TABLE_TIMEOUT: Duration = Duration::from_secs(300); | |
#[derive(Debug, PartialEq, Eq)] | ||
/// Protocol info | ||
pub struct CapabilityInfo { | ||
/// Protocol ID | ||
pub protocol: ProtocolId, | ||
/// Protocol version | ||
pub version: u8, | ||
/// Total number of packet IDs this protocol support. | ||
pub packet_count: u8, | ||
|
@@ -161,6 +163,7 @@ impl<'s> NetworkContextTrait for NetworkContext<'s> { | |
} | ||
|
||
fn disconnect_peer(&self, peer: PeerId) { | ||
trace!(target: "network", "Peer disconnect requested: {}", peer); | ||
self.io.message(NetworkIoMessage::Disconnect(peer)) | ||
.unwrap_or_else(|e| warn!("Error sending network IO message: {:?}", e)); | ||
} | ||
|
@@ -722,11 +725,24 @@ impl Host { | |
// Outgoing connections are allowed as long as their count is <= min_peers | ||
// Incoming connections are allowed to take all of the max_peers reserve, or at most half of the slots. | ||
let max_ingress = max(max_peers - min_peers, min_peers / 2); | ||
if reserved_only || | ||
(s.info.originated && egress_count > min_peers) || | ||
(!s.info.originated && ingress_count > max_ingress) { | ||
// only proceed if the connecting peer is reserved. | ||
if !self.reserved_nodes.read().contains(&id) { | ||
if reserved_only | ||
|| (s.info.originated && egress_count > min_peers) | ||
|| (!s.info.originated && ingress_count > max_ingress) | ||
{ | ||
// We didn't start the connection, but the node is known to us | ||
// So eventually we will attempt to connect to it as well. | ||
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. Have we checked that this is not subject to a variant on the connection monopolization and table poisoning attack? Let's say if there are N nodes controlled by the attacker in a client's node table. For outgoing connections, not all of those N nodes will be tried immediately. However, with this change, in incoming connections, the attacker can flood the client using all those N nodes, and controls a much larger portion of the ongoing connections. Let's say incoming maximum is I, outgoing maximum is O. In the case where I < N, total maximum number of connections is greater than N, and the attacker has perfect connectivity to the victim, we have:
|
||
let is_incoming_but_known = !s.info.originated && self.nodes.read().contains(&id); | ||
|
||
if is_incoming_but_known { | ||
warn!(target: "network", "Allowing incoming connection from a known node."); | ||
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. s/warn!/info!/ perhaps? |
||
} | ||
// only proceed if the connecting peer is reserved or is known | ||
if !is_incoming_but_known && !self.reserved_nodes.read().contains(&id) { | ||
trace!( | ||
target: "network", | ||
"Rejected {} session: TooManyPeers", | ||
if s.info.originated { "outbound" } else { "inbound" } | ||
); | ||
s.disconnect(io, DisconnectReason::TooManyPeers); | ||
kill = true; | ||
break; | ||
|
@@ -990,7 +1006,6 @@ impl IoHandler<NetworkIoMessage> for Host { | |
ref handler, | ||
ref protocol, | ||
ref versions, | ||
ref packet_count, | ||
} => { | ||
let h = handler.clone(); | ||
let reserved = self.reserved_nodes.read(); | ||
|
@@ -1000,8 +1015,12 @@ impl IoHandler<NetworkIoMessage> for Host { | |
); | ||
self.handlers.write().insert(*protocol, h); | ||
let mut info = self.info.write(); | ||
for v in versions { | ||
info.capabilities.push(CapabilityInfo { protocol: *protocol, version: *v, packet_count: *packet_count }); | ||
for &(version, packet_count) in versions { | ||
info.capabilities.push(CapabilityInfo { | ||
protocol: *protocol, | ||
version, | ||
packet_count, | ||
}); | ||
} | ||
}, | ||
NetworkIoMessage::AddTimer { | ||
|
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.
Extra space after
=
.