diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs index 3056619b..515b7a85 100644 --- a/radicle-node/src/service.rs +++ b/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(); - let node_set: HashSet<_> = - nodes.iter().filter(|key| *key != &my_id).collect(); + trusted.remove(&self.node_id()); // 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) => { - for remote_id in pks.into_iter() { + Namespaces::Many(trusted) => { + for remote_id in trusted.iter() { if refs .push((*remote_id, repo.remote(remote_id)?.refs.unverified())) .is_err() diff --git a/radicle-node/src/service/tracking.rs b/radicle-node/src/service/tracking.rs index 4903da3f..a26f4a0f 100644 --- a/radicle-node/src/service/tracking.rs +++ b/radicle-node/src/service/tracking.rs @@ -1,7 +1,7 @@ +use std::collections::HashSet; use std::ops; -use log::{error, warn}; -use nonempty::NonEmpty; +use log::error; 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) }; - - ns.ok_or_else(|| { - warn!(target: "service", "Attempted to fetch repo {rid} with no trusted peers"); - NoTrusted { rid: *rid } - }) + if trusted.is_empty() { + // Nb. returning All here because the + // fetching logic will correctly determine + // trusted and delegate remotes. + Ok(Namespaces::All) + } else { + Ok(Namespaces::Many(trusted)) + } } }, } diff --git a/radicle-node/src/test/simulator.rs b/radicle-node/src/test/simulator.rs index d68db88a..c06388d4 100644 --- a/radicle-node/src/test/simulator.rs +++ b/radicle-node/src/test/simulator.rs @@ -671,7 +671,7 @@ fn fetch( 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(); diff --git a/radicle-node/src/tests/e2e.rs b/radicle-node/src/tests/e2e.rs index 323014ab..bb6e62e0 100644 --- a/radicle-node/src/tests/e2e.rs +++ b/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::, _>>() - .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() diff --git a/radicle-node/src/worker/fetch.rs b/radicle-node/src/worker/fetch.rs index 0499fc34..2f17c0e3 100644 --- a/radicle-node/src/worker/fetch.rs +++ b/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 - /// discovered after performing the fetch for [`StagingPhaseInitial`]. - remotes: NonEmpty, + /// The delegates and tracked remotes that the fetch is being + /// performed for. These are passed through from the + /// [`StagingPhaseInitial::namespaces`], if the variant is `Many`. + trusted: HashSet, _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, 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::::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); - remotes.extend(repo.remote_ids()?.collect::, _>>()?); - remotes + let mut trusted = repo.remote_ids()?.collect::, _>>()?; + trusted.extend(repo.delegates()?.map(PublicKey::from)); + 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> { 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 + '_ { - 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 { - self.remotes + self.trusted .iter() .map(|remote| { let verification = match ( diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs index e5e0ca31..c1180f40 100644 --- a/radicle/src/storage.rs +++ b/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), -} - -impl Namespaces { - pub fn remotes(repo: &R) -> Result, refs::Error> - where - R: ReadRepository, - { - Ok(NonEmpty::collect(repo.remotes()?.keys().copied()).map(Self::Many)) - } - - pub fn delegates(repo: &R) -> Result - where - R: ReadRepository, - { - Ok(Self::Many(repo.delegates()?.map(PublicKey::from))) - } - - pub fn as_fetchspecs(&self) -> Vec { - 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(), - } - } + Many(HashSet), } impl From for Namespaces { @@ -75,12 +46,6 @@ impl From for Namespaces { } } -impl From> for Namespaces { - fn from(pks: NonEmpty) -> Self { - Self::Many(pks) - } -} - /// Storage error. #[derive(Error, Debug)] pub enum Error {