From adf6312a55b7f42f391cc650b5c7fd4aaa769104 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Tue, 28 Mar 2023 17:39:38 +0100 Subject: [PATCH] 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 X-Clacks-Overhead: GNU Terry Pratchett --- radicle-node/src/service.rs | 12 ++++---- radicle-node/src/service/tracking.rs | 24 ++++++++-------- radicle-node/src/test/simulator.rs | 2 +- radicle-node/src/tests/e2e.rs | 14 ---------- radicle-node/src/worker/fetch.rs | 42 +++++++++++++++++----------- radicle/src/storage.rs | 39 ++------------------------ 6 files changed, 46 insertions(+), 87 deletions(-) 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 {