fix: prevent seed peer address from being overwritten unless newer (#…

…4085)

Description
---
- Prevents seed peer addresses from being overwritten unless:
  -  The seed peer signs a peer update after the node starts up,
  - directly connected to the seed peer.
- Adds PeerValidator unit tests.

Motivation and Context
---
This is only a problem if the base node address has changed recently enough that it is not known to the rest of the network. The user has put the correct new address. The _unsigned_ base node peer is added to the peer DB.

Soon after, the DHT syncs peers, since the previous address was signed, the base node peer is overwritten with the previous address.

How Has This Been Tested?
---
Unit test for PeerValidator tests this behaviour

applications/tari_base_node/src/commands/command/get_peer.rs

@@ -90,6 +90,7 @@ impl CommandContext {
         });
         println!("User agent: {}", peer.user_agent);
         println!("Features: {:?}", peer.features);
+        println!("Flags: {:?}", peer.flags);
         println!("Supported protocols:");
         peer.supported_protocols.iter().for_each(|p| {
             println!("- {}", String::from_utf8_lossy(p));

applications/tari_base_node/src/commands/command/list_peers.rs

@@ -63,7 +63,9 @@ impl CommandContext {
         for peer in peers {
             let info_str = {
                 let mut s = vec![];
-
+                if peer.is_seed() {
+                    s.push("SEED".to_string());
+                }
                 if peer.is_offline() {
                     if !peer.is_banned() {
                         s.push("OFFLINE".to_string());

base_layer/p2p/src/initialization.rs

@@ -39,7 +39,7 @@ use rand::{distributions::Alphanumeric, thread_rng, Rng};
 use tari_common::configuration::Network;
 use tari_comms::{
     backoff::ConstantBackoff,
-    peer_manager::{NodeIdentity, Peer, PeerFeatures, PeerManagerError},
+    peer_manager::{NodeIdentity, Peer, PeerFeatures, PeerFlags, PeerManagerError},
     pipeline,
     protocol::{
         messaging::{MessagingEventSender, MessagingProtocolExtension},
@@ -143,7 +143,7 @@ pub async fn initialize_local_test_comms<P: AsRef<Path>>(
         .with_shutdown_signal(shutdown_signal)
         .build()?;
 
-    add_all_peers(&comms.peer_manager(), &comms.node_identity(), seed_peers).await?;
+    add_seed_peers(&comms.peer_manager(), &comms.node_identity(), seed_peers).await?;
 
     // Create outbound channel
     let (outbound_tx, outbound_rx) = mpsc::channel(10);
@@ -384,19 +384,20 @@ fn acquire_exclusive_file_lock(db_path: &Path) -> Result<File, CommsInitializati
 ///
 /// ## Returns
 /// A Result to determine if the call was successful or not, string will indicate the reason on error
-async fn add_all_peers(
+async fn add_seed_peers(
     peer_manager: &PeerManager,
     node_identity: &NodeIdentity,
     peers: Vec<Peer>,
 ) -> Result<(), CommsInitializationError> {
-    for peer in peers {
+    for mut peer in peers {
         if &peer.public_key == node_identity.public_key() {
             debug!(
                 target: LOG_TARGET,
                 "Attempting to add yourself [{}] as a seed peer to comms layer, ignoring request", peer
             );
             continue;
         }
+        peer.add_flags(PeerFlags::SEED);
 
         debug!(target: LOG_TARGET, "Adding seed peer [{}]", peer);
         peer_manager
@@ -544,12 +545,12 @@ impl ServiceInitializer for P2pInitializer {
                 Vec::new()
             },
         };
-        add_all_peers(&peer_manager, &node_identity, peers).await?;
+        add_seed_peers(&peer_manager, &node_identity, peers).await?;
 
         // TODO: Use serde
         let peers = Self::try_parse_seed_peers(&self.seed_config.peer_seeds)?;
 
-        add_all_peers(&peer_manager, &node_identity, peers).await?;
+        add_seed_peers(&peer_manager, &node_identity, peers).await?;
 
         context.register_handle(comms.connectivity());
         context.register_handle(peer_manager);

comms/core/Cargo.toml

@@ -21,7 +21,7 @@ async-trait = "0.1.36"
 bitflags = "1.0.4"
 blake2 = "0.9.0"
 bytes = { version = "1", features = ["serde"] }
-chrono = { version = "0.4.19", default-features = false, features = ["serde"] }
+chrono = { version = "0.4.19", default-features = false, features = ["serde", "clock"] }
 cidr = "0.1.0"
 clear_on_drop = "=0.2.4"
 data-encoding = "2.2.0"

comms/core/src/peer_manager/peer.rs

@@ -53,6 +53,7 @@ bitflags! {
     #[derive(Default, Deserialize, Serialize)]
     pub struct PeerFlags: u8 {
         const NONE = 0x00;
+        const SEED = 0x01;
     }
 }
 
@@ -159,11 +160,6 @@ impl Peer {
             .map(|since| Duration::from_millis(u64::try_from(since.num_milliseconds()).unwrap()))
     }
 
-    /// TODO: Remove once we don't have to sync wallet and base node db
-    pub fn unset_id(&mut self) {
-        self.id = None;
-    }
-
     pub(super) fn set_id(&mut self, id: PeerId) {
         self.id = Some(id);
     }
@@ -174,7 +170,6 @@ impl Peer {
     }
 
     #[allow(clippy::option_option)]
-
     pub fn update(
         &mut self,
         net_addresses: Option<Vec<Multiaddr>>,
@@ -305,6 +300,15 @@ impl Peer {
         self
     }
 
+    pub fn add_flags(&mut self, flags: PeerFlags) -> &mut Self {
+        self.flags |= flags;
+        self
+    }
+
+    pub fn is_seed(&self) -> bool {
+        self.flags.contains(PeerFlags::SEED)
+    }
+
     pub fn to_short_string(&self) -> String {
         format!(
             "{}::{}",

comms/dht/src/peer_validator.rs

@@ -88,11 +88,14 @@ impl<'a> PeerValidator<'a> {
                         .map(|i| i.updated_at())
                         .expect("unreachable panic");
 
-                    // Update if new_peer has newer timestamp than current_peer
+                    // Update if new_peer has newer timestamp than current_peer, and if the newer timestamp is after the
+                    // added date
                     current_peer
                         .identity_signature
                         .as_ref()
-                        .map(|i| i.updated_at() < new_dt)
+                        .map(|i| i.updated_at() < new_dt && (
+                            !current_peer.is_seed() ||
+                            current_peer.added_at < new_dt.naive_utc()))
                         // If None, update to peer with valid signature
                         .unwrap_or(true)
                 };
@@ -134,3 +137,114 @@ fn validate_node_id(public_key: &CommsPublicKey, node_id: &NodeId) -> Result<Nod
         Err(PeerValidatorError::InvalidNodeId { peer: node_id.clone() })
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use chrono::Utc;
+    use tari_comms::{net_address::MultiaddressesWithStats, peer_manager::PeerFlags};
+    use tari_test_utils::unpack_enum;
+
+    use super::*;
+    use crate::test_utils::{build_peer_manager, make_node_identity};
+
+    #[tokio::test]
+    async fn it_adds_a_valid_unsigned_peer() {
+        let peer_manager = build_peer_manager();
+        let config = DhtConfig::default_local_test();
+        let node_identity = make_node_identity();
+        let mut peer = node_identity.to_peer();
+        peer.identity_signature = None;
+        let validator = PeerValidator::new(&peer_manager, &config);
+        let is_new = validator.validate_and_add_peer(peer.clone()).await.unwrap();
+        assert!(is_new);
+        assert!(peer_manager.exists(&peer.public_key).await);
+    }
+
+    #[tokio::test]
+    async fn it_does_not_add_an_invalid_peer() {
+        let peer_manager = build_peer_manager();
+        let config = DhtConfig::default_local_test();
+        let node_identity = make_node_identity();
+        let mut peer = node_identity.to_peer();
+        // Peer MUST provide at least one address
+        peer.addresses = MultiaddressesWithStats::new(vec![]);
+        let validator = PeerValidator::new(&peer_manager, &config);
+        let err = validator.validate_and_add_peer(peer.clone()).await.unwrap_err();
+        unpack_enum!(PeerValidatorError::InvalidPeerAddresses { .. } = err);
+        assert!(!peer_manager.exists(&peer.public_key).await);
+    }
+
+    #[tokio::test]
+    async fn it_updates_a_newer_signed_peer() {
+        let peer_manager = build_peer_manager();
+        let config = DhtConfig::default_local_test();
+        let validator = PeerValidator::new(&peer_manager, &config);
+
+        let node_identity = make_node_identity();
+        let peer = node_identity.to_peer();
+        peer_manager.add_peer(peer).await.unwrap();
+
+        node_identity.set_public_address("/dns4/updated.com/tcp/1234".parse().unwrap());
+        node_identity.sign();
+        let peer = node_identity.to_peer();
+
+        let is_new = validator.validate_and_add_peer(peer.clone()).await.unwrap();
+        assert!(!is_new);
+        let peer = peer_manager
+            .find_by_public_key(&peer.public_key)
+            .await
+            .unwrap()
+            .unwrap();
+        assert_eq!(peer.addresses[0].address.to_string(), "/dns4/updated.com/tcp/1234");
+    }
+
+    #[tokio::test]
+    async fn it_does_not_update_a_valid_unsigned_peer() {
+        let peer_manager = build_peer_manager();
+        let config = DhtConfig::default_local_test();
+        let validator = PeerValidator::new(&peer_manager, &config);
+
+        let node_identity = make_node_identity();
+        let prev_addr = node_identity.public_address();
+        let mut peer = node_identity.to_peer();
+        peer_manager.add_peer(peer.clone()).await.unwrap();
+
+        peer.identity_signature = None;
+        peer.update_addresses(vec!["/dns4/updated.com/tcp/1234".parse().unwrap()]);
+
+        let is_new = validator.validate_and_add_peer(peer.clone()).await.unwrap();
+        assert!(!is_new);
+        let peer = peer_manager
+            .find_by_public_key(&peer.public_key)
+            .await
+            .unwrap()
+            .unwrap();
+        assert_eq!(peer.addresses[0].address, prev_addr);
+    }
+
+    #[tokio::test]
+    async fn it_does_not_add_a_seed_peer_if_added_more_recently_than_update() {
+        let peer_manager = build_peer_manager();
+        let config = DhtConfig::default_local_test();
+        let validator = PeerValidator::new(&peer_manager, &config);
+
+        let node_identity = make_node_identity();
+        let mut peer = node_identity.to_peer();
+        peer.add_flags(PeerFlags::SEED);
+        peer.added_at = (Utc::now() - chrono::Duration::minutes(10)).naive_utc();
+        peer_manager.add_peer(peer).await.unwrap();
+
+        node_identity.set_public_address("/dns4/updated.com/tcp/1234".parse().unwrap());
+        node_identity.sign();
+
+        let peer = node_identity.to_peer();
+        let is_new = validator.validate_and_add_peer(peer.clone()).await.unwrap();
+        assert!(!is_new);
+        let peer = peer_manager
+            .find_by_public_key(&peer.public_key)
+            .await
+            .unwrap()
+            .unwrap();
+        assert_eq!(peer.addresses[0].address, node_identity.public_address());
+    }
+}

integration_tests/features/Sync.feature

@@ -46,15 +46,14 @@ Feature: Block Sync
     Then all nodes are at height 20
 
   @critical
-  Scenario: When a new node joins the network, it should receive all peers
+  Scenario: When a new node joins the network, it receives all peers
     Given I have 10 seed nodes
     And I have a base node NODE1 connected to all seed nodes
     # additional peer seeds are being included from config.toml [common]
-    Then NODE1 should have at least 10 peers
+    Then NODE1 has at least 10 peers
     Given I have a base node NODE2 connected to node NODE1
-    Then NODE1 should have at least 11 peers
-    Then NODE2 should have at least 11 peers
-
+    Then NODE1 has at least 11 peers
+    And NODE2 has at least 11 peers
 
   Scenario: Pruned mode sync test
     Given I have a seed node SEED

integration_tests/features/support/steps.js

@@ -407,15 +407,13 @@ Then(/(.*) should have (\d+) peers/, async function (nodeName, peerCount) {
   expect(peers.length).to.equal(peerCount);
 });
 
-Then(
-  /(.*) should have at least (\d+) peers/,
-  async function (nodeName, peerCount) {
-    await sleep(500);
-    const client = this.getClient(nodeName);
+Then(/(.*) has at least (\d+) peers/, async function (nodeName, peerCount) {
+  const client = this.getClient(nodeName);
+  await waitForPredicate(async () => {
     const peers = await client.getPeers();
-    expect(peers.length).to.be.greaterThanOrEqual(peerCount);
-  }
-);
+    return peers.length >= peerCount;
+  }, 10 * 1000);
+});
 
 When("I print the world", function () {
   console.log(this);