From 3b7cf1a974560f5517d0bfc3ae3c312c1f0632e9 Mon Sep 17 00:00:00 2001 From: Nazar Mokrynskyi Date: Thu, 7 Dec 2023 13:07:12 +0200 Subject: [PATCH] Support querying peer reputation (#2392) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description Trivial change that resolves https://github.com/paritytech/polkadot-sdk/issues/2185. Since there was a mix of `who` and `peer_id` argument names nearby I changed them all to `peer_id`. # Checklist - [x] My PR includes a detailed description as outlined in the "Description" section above - [x] My PR follows the [labeling requirements](CONTRIBUTING.md#Process) of this project (at minimum one label for `T` required) - [x] I have made corresponding changes to the documentation (if applicable) - [ ] I have added tests that prove my fix is effective or that my feature works (if applicable) --------- Co-authored-by: Bastian Köcher --- .../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 | 19 +++++++++------ .../client/network/src/service/traits.rs | 23 +++++++++++-------- .../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, 60 insertions(+), 31 deletions(-) diff --git a/substrate/client/consensus/grandpa/src/communication/tests.rs b/substrate/client/consensus/grandpa/src/communication/tests.rs index 10c4772fc76d..27ca63e7c2c1 100644 --- a/substrate/client/consensus/grandpa/src/communication/tests.rs +++ b/substrate/client/consensus/grandpa/src/communication/tests.rs @@ -76,11 +76,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 6a3790ee2b2b..b9f86a7c8e03 100644 --- a/substrate/client/network-gossip/src/bridge.rs +++ b/substrate/client/network-gossip/src/bridge.rs @@ -372,9 +372,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 4bfb5a7d37f4..28e95831be4c 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 c1df48ad7858..79b7872a26fa 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 @@ -871,12 +873,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 +1201,6 @@ enum ServiceToWorkerMsg { GetValue(KademliaKey), PutValue(KademliaKey, Vec), AddKnownAddress(PeerId, Multiaddr), - ReportPeer(PeerId, ReputationChange), EventStream(out_events::Sender), Request { target: PeerId, @@ -1324,8 +1331,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 bed325ede4a8..d66816c38680 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,16 @@ where T::add_known_address(self, peer_id, addr) } - fn report_peer(&self, who: 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) + fn report_peer(&self, peer_id: PeerId, cost_benefit: ReputationChange) { + 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 885eb1f8da59..0c22e4221a1d 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 c7df5784d329..54f35e637aa8 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 a11ac7d86ecb..61dc16dba6a8 100644 --- a/substrate/client/offchain/src/lib.rs +++ b/substrate/client/offchain/src/lib.rs @@ -371,11 +371,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!(); }