From ecd99a50c864eee77b8b54c16fd90d947c568bb1 Mon Sep 17 00:00:00 2001 From: Nazar Mokrynskyi Date: Sat, 18 Nov 2023 15:18:23 +0200 Subject: [PATCH 1/3] Support querying peer reputation --- .../grandpa/src/communication/tests.rs | 10 +++++++--- substrate/client/network-gossip/src/bridge.rs | 8 ++++++-- .../network-gossip/src/state_machine.rs | 10 +++++++--- substrate/client/network/src/service.rs | 20 ++++++++++++------- .../client/network/src/service/traits.rs | 19 ++++++++++++------ .../client/network/sync/src/service/mock.rs | 5 +++-- substrate/client/offchain/src/api.rs | 8 ++++++-- substrate/client/offchain/src/lib.rs | 8 ++++++-- 8 files changed, 61 insertions(+), 27 deletions(-) diff --git a/substrate/client/consensus/grandpa/src/communication/tests.rs b/substrate/client/consensus/grandpa/src/communication/tests.rs index 4a869d0f51520..7fadf795288dc 100644 --- a/substrate/client/consensus/grandpa/src/communication/tests.rs +++ b/substrate/client/consensus/grandpa/src/communication/tests.rs @@ -74,11 +74,15 @@ impl NetworkPeers for TestNetwork { unimplemented!(); } - fn report_peer(&self, who: PeerId, cost_benefit: ReputationChange) { - let _ = self.sender.unbounded_send(Event::Report(who, cost_benefit)); + fn report_peer(&self, peer_id: PeerId, cost_benefit: ReputationChange) { + let _ = self.sender.unbounded_send(Event::Report(peer_id, cost_benefit)); } - fn disconnect_peer(&self, _who: PeerId, _protocol: ProtocolName) {} + fn peer_reputation(&self, _peer_id: &PeerId) -> i32 { + unimplemented!() + } + + fn disconnect_peer(&self, _peer_id: PeerId, _protocol: ProtocolName) {} fn accept_unreserved_peers(&self) { unimplemented!(); diff --git a/substrate/client/network-gossip/src/bridge.rs b/substrate/client/network-gossip/src/bridge.rs index 8f7d490757b3e..624326b4a362c 100644 --- a/substrate/client/network-gossip/src/bridge.rs +++ b/substrate/client/network-gossip/src/bridge.rs @@ -373,9 +373,13 @@ mod tests { unimplemented!(); } - fn report_peer(&self, _who: PeerId, _cost_benefit: ReputationChange) {} + fn report_peer(&self, _peer_id: PeerId, _cost_benefit: ReputationChange) {} - fn disconnect_peer(&self, _who: PeerId, _protocol: ProtocolName) { + fn peer_reputation(&self, _peer_id: &PeerId) -> i32 { + unimplemented!() + } + + fn disconnect_peer(&self, _peer_id: PeerId, _protocol: ProtocolName) { unimplemented!(); } diff --git a/substrate/client/network-gossip/src/state_machine.rs b/substrate/client/network-gossip/src/state_machine.rs index 4bfb5a7d37f49..28e95831be4cc 100644 --- a/substrate/client/network-gossip/src/state_machine.rs +++ b/substrate/client/network-gossip/src/state_machine.rs @@ -600,11 +600,15 @@ mod tests { unimplemented!(); } - fn report_peer(&self, who: PeerId, cost_benefit: ReputationChange) { - self.inner.lock().unwrap().peer_reports.push((who, cost_benefit)); + fn report_peer(&self, peer_id: PeerId, cost_benefit: ReputationChange) { + self.inner.lock().unwrap().peer_reports.push((peer_id, cost_benefit)); } - fn disconnect_peer(&self, _who: PeerId, _protocol: ProtocolName) { + fn peer_reputation(&self, _peer_id: &PeerId) -> i32 { + unimplemented!() + } + + fn disconnect_peer(&self, _peer_id: PeerId, _protocol: ProtocolName) { unimplemented!(); } diff --git a/substrate/client/network/src/service.rs b/substrate/client/network/src/service.rs index c1df48ad7858d..8a8aa20e14569 100644 --- a/substrate/client/network/src/service.rs +++ b/substrate/client/network/src/service.rs @@ -116,6 +116,8 @@ pub struct NetworkService { local_identity: Keypair, /// Bandwidth logging system. Can be queried to know the average bandwidth consumed. bandwidth: Arc, + /// Used to query and report reputation changes. + peer_store_handle: PeerStoreHandle, /// Channel that sends messages to the actual worker. to_worker: TracingUnboundedSender, /// For each peer and protocol combination, an object that allows sending notifications to @@ -527,6 +529,7 @@ where sync_protocol_handle, _marker: PhantomData, _block: Default::default(), + peer_store_handle: params.peer_store.clone(), }); Ok(NetworkWorker { @@ -871,12 +874,18 @@ where .unbounded_send(ServiceToWorkerMsg::AddKnownAddress(peer_id, addr)); } - fn report_peer(&self, who: PeerId, cost_benefit: ReputationChange) { - let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::ReportPeer(who, cost_benefit)); + fn report_peer(&self, peer_id: PeerId, cost_benefit: ReputationChange) { + self.peer_store_handle.clone().report_peer(peer_id, cost_benefit); + } + + fn peer_reputation(&self, peer_id: &PeerId) -> i32 { + self.peer_store_handle.peer_reputation(peer_id) } - fn disconnect_peer(&self, who: PeerId, protocol: ProtocolName) { - let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::DisconnectPeer(who, protocol)); + fn disconnect_peer(&self, peer_id: PeerId, protocol: ProtocolName) { + let _ = self + .to_worker + .unbounded_send(ServiceToWorkerMsg::DisconnectPeer(peer_id, protocol)); } fn accept_unreserved_peers(&self) { @@ -1193,7 +1202,6 @@ enum ServiceToWorkerMsg { GetValue(KademliaKey), PutValue(KademliaKey, Vec), AddKnownAddress(PeerId, Multiaddr), - ReportPeer(PeerId, ReputationChange), EventStream(out_events::Sender), Request { target: PeerId, @@ -1324,8 +1332,6 @@ where self.network_service.behaviour_mut().put_value(key, value), ServiceToWorkerMsg::AddKnownAddress(peer_id, addr) => self.network_service.behaviour_mut().add_known_address(peer_id, addr), - ServiceToWorkerMsg::ReportPeer(peer_id, reputation_change) => - self.peer_store_handle.report_peer(peer_id, reputation_change), ServiceToWorkerMsg::EventStream(sender) => self.event_streams.push(sender), ServiceToWorkerMsg::Request { target, diff --git a/substrate/client/network/src/service/traits.rs b/substrate/client/network/src/service/traits.rs index bed325ede4a85..9fcdadaf05533 100644 --- a/substrate/client/network/src/service/traits.rs +++ b/substrate/client/network/src/service/traits.rs @@ -150,12 +150,15 @@ pub trait NetworkPeers { /// Report a given peer as either beneficial (+) or costly (-) according to the /// given scalar. - fn report_peer(&self, who: PeerId, cost_benefit: ReputationChange); + fn report_peer(&self, peer_id: PeerId, cost_benefit: ReputationChange); + + /// Get peer reputation. + fn peer_reputation(&self, peer_id: &PeerId) -> i32; /// Disconnect from a node as soon as possible. /// /// This triggers the same effects as if the connection had closed itself spontaneously. - fn disconnect_peer(&self, who: PeerId, protocol: ProtocolName); + fn disconnect_peer(&self, peer_id: PeerId, protocol: ProtocolName); /// Connect to unreserved peers and allow unreserved peers to connect for syncing purposes. fn accept_unreserved_peers(&self); @@ -241,16 +244,20 @@ where T::add_known_address(self, peer_id, addr) } - fn report_peer(&self, who: PeerId, cost_benefit: ReputationChange) { + fn report_peer(&self, peer_id: PeerId, cost_benefit: ReputationChange) { // TODO: when we get rid of `Peerset`, we'll likely need to add some kind of async // interface to `PeerStore`, otherwise we'll have trouble calling functions accepting // `&mut self` via `Arc`. // See https://github.com/paritytech/substrate/issues/14170. - T::report_peer(self, who, cost_benefit) + T::report_peer(self, peer_id, cost_benefit) + } + + fn peer_reputation(&self, peer_id: &PeerId) -> i32 { + T::peer_reputation(self, peer_id) } - fn disconnect_peer(&self, who: PeerId, protocol: ProtocolName) { - T::disconnect_peer(self, who, protocol) + fn disconnect_peer(&self, peer_id: PeerId, protocol: ProtocolName) { + T::disconnect_peer(self, peer_id, protocol) } fn accept_unreserved_peers(&self) { diff --git a/substrate/client/network/sync/src/service/mock.rs b/substrate/client/network/sync/src/service/mock.rs index 885eb1f8da593..0c22e4221a1dc 100644 --- a/substrate/client/network/sync/src/service/mock.rs +++ b/substrate/client/network/sync/src/service/mock.rs @@ -83,8 +83,9 @@ mockall::mock! { fn set_authorized_peers(&self, peers: HashSet); fn set_authorized_only(&self, reserved_only: bool); fn add_known_address(&self, peer_id: PeerId, addr: Multiaddr); - fn report_peer(&self, who: PeerId, cost_benefit: ReputationChange); - fn disconnect_peer(&self, who: PeerId, protocol: ProtocolName); + fn report_peer(&self, peer_id: PeerId, cost_benefit: ReputationChange); + fn peer_reputation(&self, peer_id: &PeerId) -> i32; + fn disconnect_peer(&self, peer_id: PeerId, protocol: ProtocolName); fn accept_unreserved_peers(&self); fn deny_unreserved_peers(&self); fn add_reserved_peer(&self, peer: MultiaddrWithPeerId) -> Result<(), String>; diff --git a/substrate/client/offchain/src/api.rs b/substrate/client/offchain/src/api.rs index c7df5784d329e..54f35e637aa84 100644 --- a/substrate/client/offchain/src/api.rs +++ b/substrate/client/offchain/src/api.rs @@ -243,11 +243,15 @@ mod tests { unimplemented!(); } - fn report_peer(&self, _who: PeerId, _cost_benefit: ReputationChange) { + fn report_peer(&self, _peer_id: PeerId, _cost_benefit: ReputationChange) { unimplemented!(); } - fn disconnect_peer(&self, _who: PeerId, _protocol: ProtocolName) { + fn peer_reputation(&self, _peer_id: &PeerId) -> i32 { + unimplemented!() + } + + fn disconnect_peer(&self, _peer_id: PeerId, _protocol: ProtocolName) { unimplemented!(); } diff --git a/substrate/client/offchain/src/lib.rs b/substrate/client/offchain/src/lib.rs index 756ab77ff94eb..a27a43b9b1321 100644 --- a/substrate/client/offchain/src/lib.rs +++ b/substrate/client/offchain/src/lib.rs @@ -372,11 +372,15 @@ mod tests { unimplemented!(); } - fn report_peer(&self, _who: PeerId, _cost_benefit: ReputationChange) { + fn report_peer(&self, _peer_id: PeerId, _cost_benefit: ReputationChange) { unimplemented!(); } - fn disconnect_peer(&self, _who: PeerId, _protocol: ProtocolName) { + fn peer_reputation(&self, _peer_id: &PeerId) -> i32 { + unimplemented!() + } + + fn disconnect_peer(&self, _peer_id: PeerId, _protocol: ProtocolName) { unimplemented!(); } From da09a27b3ea95bc68c2bc07594faff9146d27561 Mon Sep 17 00:00:00 2001 From: Nazar Mokrynskyi Date: Mon, 4 Dec 2023 16:13:02 +0200 Subject: [PATCH 2/3] Remove unnecessary comment --- substrate/client/network/src/service/traits.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/substrate/client/network/src/service/traits.rs b/substrate/client/network/src/service/traits.rs index 9fcdadaf05533..d66816c386804 100644 --- a/substrate/client/network/src/service/traits.rs +++ b/substrate/client/network/src/service/traits.rs @@ -245,10 +245,6 @@ where } fn report_peer(&self, peer_id: PeerId, cost_benefit: ReputationChange) { - // TODO: when we get rid of `Peerset`, we'll likely need to add some kind of async - // interface to `PeerStore`, otherwise we'll have trouble calling functions accepting - // `&mut self` via `Arc`. - // See https://github.com/paritytech/substrate/issues/14170. T::report_peer(self, peer_id, cost_benefit) } From bad7f8420b882a3b1cdc9869ddf4784767bfc714 Mon Sep 17 00:00:00 2001 From: Nazar Mokrynskyi Date: Thu, 7 Dec 2023 11:34:28 +0200 Subject: [PATCH 3/3] Remove duplicated `peer_store_handle` struct field --- substrate/client/network/src/service.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/substrate/client/network/src/service.rs b/substrate/client/network/src/service.rs index a8b5f0a733c2c..06db23844d0d9 100644 --- a/substrate/client/network/src/service.rs +++ b/substrate/client/network/src/service.rs @@ -132,8 +132,6 @@ pub struct NetworkService { protocol_handles: Vec, /// Shortcut to sync protocol handle (`protocol_handles[0]`). sync_protocol_handle: protocol_controller::ProtocolHandle, - /// Handle to `PeerStore`. - peer_store_handle: PeerStoreHandle, /// Marker to pin the `H` generic. Serves no purpose except to not break backwards /// compatibility. _marker: PhantomData, @@ -523,7 +521,6 @@ where peer_store_handle: params.peer_store.clone(), _marker: PhantomData, _block: Default::default(), - peer_store_handle: params.peer_store.clone(), }); Ok(NetworkWorker {