node: fetch trusted peers on clone

When a repository does not yet exist during a fetch, i.e. a clone,
then only delegates are being fetched.

Augment Namespaces::Many to hold the trusted and optional delegate peers
separately. Doing so allows the construction of the variant without
the repository existing, but tracking relationships existing.

This variant can then be used to build refspecs for both the trusted
peers and delegates when doing a cloning fetch.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
X-Clacks-Overhead: GNU Terry Pratchett

radicle-node/src/service.rs

@@ -1004,17 +1004,15 @@ where
             tracking::Scope::Trusted => {
                 match self.tracking.namespaces_for(&self.storage, &message.rid) {
                     Ok(Namespaces::All) => Ok(true),
-                    Ok(Namespaces::Many(nodes)) => {
+                    Ok(Namespaces::Many(mut trusted)) => {
                         // Get the set of trusted nodes except self.
-                        let my_id = self.node_id();
+                        trusted.remove(&self.node_id());
-                        let node_set: HashSet<_> =
-                            nodes.iter().filter(|key| *key != &my_id).collect();
 
                         // Check if there is at least one trusted ref.
                         Ok(message
                             .refs
                             .iter()
-                            .any(|(pub_key, _refs)| node_set.contains(pub_key)))
+                            .any(|(pub_key, _refs)| trusted.contains(pub_key)))
                     }
                     Ok(Namespaces::One(key)) => {
                         Ok(message.refs.iter().any(|(pub_key, _refs)| pub_key == &key))
@@ -1279,8 +1277,8 @@ where
                 // SAFETY: `REF_REMOTE_LIMIT` is greater than 1, thus the bounded vec can hold at least
                 // one remote.
                 .unwrap(),
-            Namespaces::Many(pks) => {
+            Namespaces::Many(trusted) => {
-                for remote_id in pks.into_iter() {
+                for remote_id in trusted.iter() {
                     if refs
                         .push((*remote_id, repo.remote(remote_id)?.refs.unverified()))
                         .is_err()

radicle-node/src/service/tracking.rs

@@ -1,7 +1,7 @@
+use std::collections::HashSet;
 use std::ops;
 
-use log::{error, warn};
+use log::error;
-use nonempty::NonEmpty;
 use thiserror::Error;
 
 use radicle::crypto::PublicKey;
@@ -114,25 +114,25 @@ impl Config {
                     let nodes = self
                         .node_policies()
                         .map_err(|err| FailedNodes { rid: *rid, err })?;
-                    let mut trusted: Vec<_> = nodes
+                    let mut trusted: HashSet<_> = nodes
                         .filter_map(|node| (node.policy == Policy::Track).then_some(node.id))
                         .collect();
 
-                    let ns = if let Ok(repo) = storage.repository(*rid) {
+                    if let Ok(repo) = storage.repository(*rid) {
                         let delegates = repo
                             .delegates()
                             .map_err(|err| FailedDelegates { rid: *rid, err })?
                             .map(PublicKey::from);
                         trusted.extend(delegates);
-                        NonEmpty::from_vec(trusted).map(Namespaces::Many)
-                    } else {
-                        Some(Namespaces::All)
                     };
-
+                    if trusted.is_empty() {
-                    ns.ok_or_else(|| {
+                        // Nb. returning All here because the
-                        warn!(target: "service", "Attempted to fetch repo {rid} with no trusted peers");
+                        // fetching logic will correctly determine
-                        NoTrusted { rid: *rid }
+                        // trusted and delegate remotes.
-                    })
+                        Ok(Namespaces::All)
+                    } else {
+                        Ok(Namespaces::Many(trusted))
+                    }
                 }
             },
         }

radicle-node/src/test/simulator.rs

@@ -671,7 +671,7 @@ fn fetch<W: WriteRepository>(
     let namespace = match namespaces.into() {
         Namespaces::All => None,
         Namespaces::One(ns) => Some(ns),
-        Namespaces::Many(ns) => Some(ns.head),
+        Namespaces::Many(trusted) => trusted.into_iter().next(),
     };
     let mut updates = Vec::new();
     let mut callbacks = git::RemoteCallbacks::new();

radicle-node/src/tests/e2e.rs

@@ -316,20 +316,6 @@ fn test_fetch_trusted_remotes() {
     log::debug!(target: "test", "Fetch complete with {}", bob.id);
 
     let bob_repo = bob.storage.repository(acme).unwrap();
-    let bob_remotes = bob_repo
-        .remote_ids()
-        .unwrap()
-        .collect::<Result<HashSet<_>, _>>()
-        .unwrap();
-    assert_eq!(bob_remotes, Some(alice.id).into_iter().collect());
-
-    // TODO(finto): we have to fetch again to get the other trusted remotes.
-    // At the moment, the existing Namespaces enum does not allow us
-    // to pass on what nodes are tracked, if there is no existing
-    // repository. Thus, the first fetch only attempts to clone the
-    // delegate.
-    bob.handle.fetch(acme, alice.id).unwrap();
-    assert!(result.is_success());
     let bob_remotes = bob_repo
         .remote_ids()
         .unwrap()

radicle-node/src/worker/fetch.rs

@@ -3,11 +3,9 @@ pub use refspecs::{AsRefspecs, Refspec, SpecialRefs};
 
 pub mod error;
 
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashSet};
 use std::ops::Deref;
 
-use nonempty::NonEmpty;
-
 use radicle::crypto::{PublicKey, Unverified, Verified};
 use radicle::git::url;
 use radicle::prelude::{Doc, Id};
@@ -69,9 +67,10 @@ pub struct StagingPhaseFinal<'a> {
     pub(super) repo: StagedRepository,
     /// The original [`Storage`] we are finalising changes into.
     production: &'a Storage,
-    /// The remotes that the fetch is being performed for. These are
+    /// The delegates and tracked remotes that the fetch is being
-    /// discovered after performing the fetch for [`StagingPhaseInitial`].
+    /// performed for. These are passed through from the
-    remotes: NonEmpty<RemoteId>,
+    /// [`StagingPhaseInitial::namespaces`], if the variant is `Many`.
+    trusted: HashSet<RemoteId>,
     _tmp: tempfile::TempDir,
 }
 
@@ -124,14 +123,24 @@ impl<'a> StagingPhaseInitial<'a> {
     /// Convert the [`StagingPhaseInitial`] into [`StagingPhaseFinal`] to continue
     /// the fetch process.
     pub fn into_final(self) -> Result<StagingPhaseFinal<'a>, error::Transition> {
-        let remotes = match &self.repo {
+        let trusted = match &self.repo {
             StagedRepository::Cloning(repo) => {
                 log::debug!(target: "worker", "Loading remotes for clone");
                 let oid = ReadRepository::identity_head(repo)?;
                 log::trace!(target: "worker", "Loading 'rad/id' @ {oid}");
                 let (doc, _) = Doc::<Unverified>::load_at(oid, repo)?;
                 let doc = doc.verified()?;
-                doc.delegates.map(PublicKey::from)
+                let mut trusted = match self.namespaces.clone() {
+                    Namespaces::All => HashSet::new(),
+                    // TODO(finto): this is one of those cases where
+                    // having the `One` variant doesn't make any
+                    // sense.
+                    Namespaces::One(pk) => [pk].into_iter().collect(),
+                    Namespaces::Many(trusted) => trusted,
+                };
+                let delegates = doc.delegates.map(PublicKey::from);
+                trusted.extend(delegates);
+                trusted
             }
             StagedRepository::Fetching(repo) => {
                 log::debug!(target: "worker", "Loading remotes for fetching");
@@ -140,11 +149,12 @@ impl<'a> StagingPhaseInitial<'a> {
                     // namespaces_for so it's safe to just bundle this
                     // with Namespaces::All
                     Namespaces::One(_) | Namespaces::All => {
-                        let mut remotes = repo.delegates()?.map(PublicKey::from);
+                        let mut trusted = repo.remote_ids()?.collect::<Result<HashSet<_>, _>>()?;
-                        remotes.extend(repo.remote_ids()?.collect::<Result<Vec<_>, _>>()?);
+                        trusted.extend(repo.delegates()?.map(PublicKey::from));
-                        remotes
+                        trusted
                     }
-                    Namespaces::Many(remotes) => remotes,
+
+                    Namespaces::Many(trusted) => trusted,
                 }
             }
         };
@@ -152,7 +162,7 @@ impl<'a> StagingPhaseInitial<'a> {
         Ok(StagingPhaseFinal {
             repo: self.repo,
             production: self.production,
-            remotes,
+            trusted,
             _tmp: self._tmp,
         })
     }
@@ -192,7 +202,7 @@ impl<'a> StagingPhaseFinal<'a> {
     /// references.
     pub fn refspecs(&self) -> Vec<Refspec<git::PatternString, git::PatternString>> {
         match self.repo {
-            StagedRepository::Cloning(_) => Namespaces::Many(self.remotes.clone()).as_refspecs(),
+            StagedRepository::Cloning(_) => Namespaces::Many(self.trusted.clone()).as_refspecs(),
             StagedRepository::Fetching(_) => {
                 self.remotes().fold(Vec::new(), |mut specs, remote| {
                     specs.extend(remote.as_refspecs());
@@ -266,7 +276,7 @@ impl<'a> StagingPhaseFinal<'a> {
     }
 
     fn remotes(&self) -> impl Iterator<Item = Remote> + '_ {
-        self.remotes
+        self.trusted
             .iter()
             .filter_map(|remote| match SignedRefs::load(remote, self.repo.deref()) {
                 Ok(refs) => Some(Remote::new(*remote, refs)),
@@ -278,7 +288,7 @@ impl<'a> StagingPhaseFinal<'a> {
     }
 
     fn verify(&self) -> BTreeMap<RemoteId, VerifiedRemote> {
-        self.remotes
+        self.trusted
             .iter()
             .map(|remote| {
                 let verification = match (

radicle/src/storage.rs

@@ -1,7 +1,7 @@
 pub mod git;
 pub mod refs;
 
-use std::collections::hash_map;
+use std::collections::{hash_map, HashSet};
 use std::ops::Deref;
 use std::path::{Path, PathBuf};
 use std::{fmt, io};
@@ -37,36 +37,7 @@ pub enum Namespaces {
     /// A single namespace, by public key.
     One(PublicKey),
     /// Many namespaces, by public keys.
-    Many(NonEmpty<PublicKey>),
+    Many(HashSet<PublicKey>),
-}
-
-impl Namespaces {
-    pub fn remotes<R>(repo: &R) -> Result<Option<Self>, refs::Error>
-    where
-        R: ReadRepository,
-    {
-        Ok(NonEmpty::collect(repo.remotes()?.keys().copied()).map(Self::Many))
-    }
-
-    pub fn delegates<R>(repo: &R) -> Result<Self, IdentityError>
-    where
-        R: ReadRepository,
-    {
-        Ok(Self::Many(repo.delegates()?.map(PublicKey::from)))
-    }
-
-    pub fn as_fetchspecs(&self) -> Vec<String> {
-        match self {
-            Self::All => vec![String::from("refs/namespaces/*:refs/namespaces/*")],
-            Self::One(pk) => vec![format!(
-                "refs/namespaces/{pk}/refs/*:refs/namespaces/{pk}/refs/*"
-            )],
-            Self::Many(pks) => pks
-                .iter()
-                .map(|pk| format!("refs/namespaces/{pk}/refs/*:refs/namespaces/{pk}/refs/*"))
-                .collect(),
-        }
-    }
 }
 
 impl From<PublicKey> for Namespaces {
@@ -75,12 +46,6 @@ impl From<PublicKey> for Namespaces {
     }
 }
 
-impl From<NonEmpty<PublicKey>> for Namespaces {
-    fn from(pks: NonEmpty<PublicKey>) -> Self {
-        Self::Many(pks)
-    }
-}
-
 /// Storage error.
 #[derive(Error, Debug)]
 pub enum Error {