Skip to content

Commit

Permalink
fix: ensure that features are set when syncing peers (#3745)
Browse files Browse the repository at this point in the history
Description
---

- updates features in peer sync when a signature is valid
- invalidates the identity signature of a peer if addresses/features are changed

Motivation and Context
---
If a peer somehow is created with incorrect peer features (say, by adding a wallet to peer_seeds list), and that same peer is later synced with a valid signature but without updating the erroneous features in the existing peer, the peer will have an invalid state and signature in its database causing it to be banned by other nodes.

How Has This Been Tested?
---
Manually, added a wallet to my peer seeds and didnt get banned
  • Loading branch information
sdbondi committed Jan 25, 2022
1 parent d041c70 commit 8efd2e4
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 8 deletions.
6 changes: 3 additions & 3 deletions base_layer/p2p/src/peer_seeds.rs
Expand Up @@ -81,7 +81,7 @@ pub struct SeedPeer {

impl SeedPeer {
#[inline]
pub fn get_node_id(&self) -> NodeId {
pub fn derive_node_id(&self) -> NodeId {
NodeId::from_public_key(&self.public_key)
}
}
Expand All @@ -105,8 +105,8 @@ impl FromStr for SeedPeer {

impl From<SeedPeer> for Peer {
fn from(seed: SeedPeer) -> Self {
let node_id = seed.get_node_id();
Self::new(
let node_id = seed.derive_node_id();
Peer::new(
seed.public_key,
node_id,
seed.addresses.into(),
Expand Down
10 changes: 7 additions & 3 deletions comms/dht/src/peer_validator.rs
Expand Up @@ -103,9 +103,13 @@ impl<'a> PeerValidator<'a> {
}

debug!(target: LOG_TARGET, "Updating peer `{}`", new_peer.node_id);
current_peer.addresses.update_addresses(new_peer.addresses.into_vec());
current_peer.identity_signature = new_peer.identity_signature;
current_peer.set_offline(false);
current_peer
.update_addresses(new_peer.addresses.into_vec())
.set_features(new_peer.features)
.set_offline(false);
if let Some(sig) = new_peer.identity_signature {
current_peer.set_valid_identity_signature(sig);
}
self.peer_manager.add_peer(current_peer).await?;

Ok(false)
Expand Down
27 changes: 25 additions & 2 deletions comms/src/peer_manager/peer.rs
Expand Up @@ -266,13 +266,14 @@ impl Peer {
self.banned_until.as_ref().filter(|dt| *dt > &Utc::now().naive_utc())
}

/// Marks the peer as offline
pub fn set_offline(&mut self, is_offline: bool) {
/// Marks the peer as offline if true, or not offline if false
pub fn set_offline(&mut self, is_offline: bool) -> &mut Self {
if is_offline {
self.offline_at = Some(Utc::now().naive_utc());
} else {
self.offline_at = None;
}
self
}

/// This will store metadata inside of the metadata field in the peer.
Expand All @@ -286,6 +287,28 @@ impl Peer {
self.metadata.get(&key)
}

/// Set the identity signature of the peer. WARNING: It is up to the caller to ensure that the signature is valid.
pub fn set_valid_identity_signature(&mut self, signature: IdentitySignature) -> &mut Self {
self.identity_signature = Some(signature);
self
}

/// Update the peer's addresses. This call will invalidate the identity signature.
pub fn update_addresses(&mut self, addresses: Vec<Multiaddr>) -> &mut Self {
self.addresses.update_addresses(addresses);
self.identity_signature = None;
self
}

/// Update the peer's features. This call will invalidate the identity signature if the features differ.
pub fn set_features(&mut self, features: PeerFeatures) -> &mut Self {
if self.features != features {
self.features = features;
self.identity_signature = None;
}
self
}

pub fn to_short_string(&self) -> String {
format!(
"{}::{}",
Expand Down

0 comments on commit 8efd2e4

Please sign in to comment.