node: Get the failing fetch tests passing

We fix the failing fetch tests by:

1. Not verifying our own refs, unless we're cloning, since we're
   otherwise not fetching our own refs.
2. Always force-fetching `sigrefs` from remotes into the staging copy.
3. Making sure that sigref updates are fast-forward before transfering
   the remote into the production copy.

radicle-cli/tests/commands.rs

@@ -612,7 +612,12 @@ fn test_cob_deletion() {
     let alice_issues = radicle::cob::issue::Issues::open(&alice_repo).unwrap();
     alice_issues.remove(issue_id, &alice.signer).unwrap();
 
-    bob.handle.fetch(rid, alice.id).unwrap();
+    log::debug!(target: "test", "Removing issue..");
+
+    radicle::assert_matches!(
+        bob.handle.fetch(rid, alice.id).unwrap(),
+        radicle::node::FetchResult::Success { .. }
+    );
     let bob_repo = bob.storage.repository(rid).unwrap();
     let bob_issues = radicle::cob::issue::Issues::open(&bob_repo).unwrap();
     assert!(bob_issues.get(issue_id).unwrap().is_none());

radicle-crypto/src/lib.rs

@@ -364,6 +364,13 @@ impl PublicKey {
     pub fn to_namespace(&self) -> git_ref_format::RefString {
         git_ref_format::refname!("refs/namespaces").join(git_ref_format::Component::from(self))
     }
+
+    #[cfg(feature = "git-ref-format")]
+    pub fn from_namespaced(refstr: &git_ref_format::Namespaced) -> Result<Self, PublicKeyError> {
+        let name = refstr.namespace().into_inner();
+
+        Self::from_str(name.deref().as_str())
+    }
 }
 
 impl FromStr for PublicKey {

radicle-node/src/tests/e2e.rs

@@ -736,7 +736,6 @@ fn test_connection_crossing() {
 }
 
 #[test]
-#[should_panic]
 /// Alice is going to try to fetch outdated refs of Bob, from Eve. This is a non-fastfoward fetch
 /// on the sigrefs branch.
 fn test_non_fastforward_sigrefs() {
@@ -791,7 +790,6 @@ fn test_non_fastforward_sigrefs() {
 }
 
 #[test]
-#[should_panic]
 fn test_outdated_sigrefs() {
     logger::init(log::Level::Debug);
 
@@ -850,6 +848,7 @@ fn test_outdated_sigrefs() {
     let eves_refs = repo.remote(&eve.id).unwrap().refs;
 
     // Get the current state of eve's refs in alice's storage
+    log::debug!(target: "test", "Alice fetches from Eve..");
     assert_matches!(
         alice.handle.fetch(rid, eve.id).unwrap(),
         FetchResult::Success { .. }
@@ -860,6 +859,8 @@ fn test_outdated_sigrefs() {
     assert_ne!(eves_refs_expected, old_refs);
     assert_eq!(eves_refs_expected, eves_refs);
 
+    log::debug!(target: "test", "Alice fetches from Bob..");
+
     alice
         .handle
         .track_node(bob.id, Some("bob".to_string()))
@@ -869,7 +870,7 @@ fn test_outdated_sigrefs() {
         FetchResult::Success { .. }
     );
 
-    // Ensure that bob's refs have not changed
+    // Ensure that Eve's refs have not changed after fetching the old refs from Bob.
     let repo = alice.storage.repository(rid).unwrap();
     let eve_remote = repo.remote(&eve.id).unwrap();
     let eves_refs = eve_remote.refs;

radicle-node/src/worker.rs

@@ -229,7 +229,8 @@ impl Worker {
         namespaces: &Namespaces,
         mut channels: Channels,
     ) -> Result<(Vec<RefUpdate>, HashSet<NodeId>), FetchError> {
-        let staging = fetch::StagingPhaseInitial::new(&self.storage, rid, namespaces.clone())?;
+        let staging =
+            fetch::StagingPhaseInitial::new(&self.storage, rid, self.nid, namespaces.clone())?;
         let refs = if staging.repo.is_cloning() {
             match self._fetch(
                 &staging.repo,

radicle-node/src/worker/fetch.rs

@@ -28,6 +28,8 @@ pub struct StagingPhaseInitial<'a> {
     pub(super) repo: StagedRepository,
     /// The original [`Storage`] we are finalising changes into.
     production: &'a Storage,
+    /// The local Node ID.
+    nid: NodeId,
     /// The `Namespaces` passed by the fetching caller.
     pub(super) namespaces: Namespaces,
     _tmp: tempfile::TempDir,
@@ -101,6 +103,8 @@ pub struct StagingPhaseFinal<'a> {
     pub(super) repo: FinalStagedRepository,
     /// The original [`Storage`] we are finalising changes into.
     production: &'a Storage,
+    /// The local Node ID.
+    nid: NodeId,
     _tmp: tempfile::TempDir,
 }
 
@@ -115,6 +119,7 @@ enum VerifiedRemote {
         /// Unsigned refs.
         unsigned: Vec<git::RefString>,
     },
+    UpToDate,
 }
 
 impl<'a> StagingPhaseInitial<'a> {
@@ -123,6 +128,7 @@ impl<'a> StagingPhaseInitial<'a> {
     pub fn new(
         production: &'a Storage,
         rid: Id,
+        nid: NodeId,
         namespaces: Namespaces,
     ) -> Result<Self, error::Init> {
         let tmp = tempfile::TempDir::new()?;
@@ -131,6 +137,7 @@ impl<'a> StagingPhaseInitial<'a> {
         let repo = Self::repository(&staging, production, rid)?;
         Ok(Self {
             repo,
+            nid,
             production,
             namespaces,
             _tmp: tmp,
@@ -194,6 +201,7 @@ impl<'a> StagingPhaseInitial<'a> {
 
         Ok(StagingPhaseFinal {
             repo,
+            nid: self.nid,
             production: self.production,
             _tmp: self._tmp,
         })
@@ -273,27 +281,45 @@ impl<'a> StagingPhaseFinal<'a> {
     /// All references that were updated are returned as a
     /// [`RefUpdate`].
     pub fn transfer(self) -> Result<(Vec<RefUpdate>, HashSet<NodeId>), error::Transfer> {
-        let verifications = self.verify()?;
+        // Nb. we have to verify in a different order when fetching vs. cloning, due to needing
-        let production = match &self.repo {
+        // access to the existing repository in the fetching case.
-            FinalStagedRepository::Cloning { repo, .. } => self.production.create(repo.id)?,
+        let (production, verifications) = match &self.repo {
-            FinalStagedRepository::Fetching { repo, .. } => self.production.repository(repo.id)?,
+            FinalStagedRepository::Cloning { repo, .. } => {
+                let verifications = self.verify::<Repository>(None)?;
+                let prod = self.production.create(repo.id)?;
+
+                (prod, verifications)
+            }
+            FinalStagedRepository::Fetching { repo, .. } => {
+                let prod = self.production.repository(repo.id)?;
+                let verifications = self.verify(Some(&prod))?;
+
+                (prod, verifications)
+            }
         };
         let url = url::File::new(self.repo.path().to_path_buf()).to_string();
         let mut remote = production.backend.remote_anonymous(&url)?;
         let mut updates = Vec::new();
         let mut delete = HashSet::new();
+        let mut skipped = HashSet::new();
 
         let callbacks = ref_updates(&mut updates);
-        let remotes = {
+        let mut remotes = {
             let specs = verifications
                 .into_iter()
                 .flat_map(|(remote, verified)| match verified {
+                    VerifiedRemote::UpToDate => {
+                        log::debug!(target: "worker", "{remote} is up-to-date");
+                        skipped.insert(remote);
+
+                        vec![]
+                    }
                     VerifiedRemote::Failed { reason } => {
                         // TODO: We should include the skipped remotes in the fetch result,
                         // with the reason why they're skipped.
                         log::warn!(
                             target: "worker",
-                            "{remote} failed to verify, will not fetch any further refs: {reason}",
+                            "{remote} failed to verify, ignoring ref updates: {reason}",
                         );
                         vec![]
                     }
@@ -356,11 +382,12 @@ impl<'a> StagingPhaseFinal<'a> {
 
             let (fetching, specs): (HashSet<_>, Vec<_>) = specs.into_iter().unzip();
 
-            if !self
+            if self.repo.is_cloning()
-                .repo
+                && !self
-                .delegates()?
+                    .repo
-                .iter()
+                    .delegates()?
-                .all(|d| fetching.contains(d.as_key()))
+                    .iter()
+                    .all(|d| fetching.contains(d.as_key()))
             {
                 return Err(error::Transfer::NoDelegates);
             }
@@ -408,6 +435,10 @@ impl<'a> StagingPhaseFinal<'a> {
             production.id,
         );
 
+        // Extend the list of remotes we attempted to fetch from with the skipped remotes.
+        // This confirms to the user that the remote was indeed tried.
+        remotes.extend(skipped);
+
         Ok((updates, remotes))
     }
 
@@ -418,17 +449,68 @@ impl<'a> StagingPhaseFinal<'a> {
                     .iter()
                     .filter_map(|remote| self.repo.remote(remote).ok()),
             )),
-            FinalStagedRepository::Fetching { repo, .. } => Ok(Box::new(
+            FinalStagedRepository::Fetching { repo, refs } => {
-                repo.remotes()?.filter_map(|r| r.ok().map(|(_, r)| r)),
+                // Only verify remotes we're fetching refs from.
-            )),
+                let remotes = refs
+                    .iter()
+                    .filter_map(|r| NodeId::from_namespaced(r).ok())
+                    .collect::<HashSet<_>>();
+                let remotes = remotes.into_iter().filter_map(|r| repo.remote(&r).ok());
+
+                Ok(Box::new(remotes))
+            }
         }
     }
 
-    fn verify(&self) -> Result<BTreeMap<RemoteId, VerifiedRemote>, git::raw::Error> {
+    fn verify<R: ReadRepository>(
-        Ok(self
+        &self,
+        local: Option<&R>,
+    ) -> Result<BTreeMap<RemoteId, VerifiedRemote>, git::raw::Error> {
+        let result = self
             .remotes()?
+            .filter(|remote| remote.id != self.nid || self.repo.is_cloning())
             .map(|remote| {
                 let remote_id = remote.id;
+
+                log::debug!(target: "worker", "Verifying remote {remote_id}..");
+
+                // If we have a local copy, ie. we're not cloning, we check that the signed refs
+                // are being fast-forwarded.
+                if let Some(local) = local {
+                    if let (Ok(local), Ok(staging)) = (
+                        local.reference_oid(&remote_id, &git::refs::storage::SIGREFS_BRANCH),
+                        self.repo.reference_oid(&remote_id, &git::refs::storage::SIGREFS_BRANCH),
+                    ) {
+                        if local != staging  {
+                            match self
+                                .repo
+                                .backend
+                                .graph_descendant_of(staging.into(), local.into())
+                            {
+                                Ok(true) => {
+                                    log::debug!(target: "worker", "Signed refs for {remote_id} fast-foward: {local} -> {staging}");
+                                }
+                                Ok(false) => {
+                                    return (
+                                        remote_id,
+                                        VerifiedRemote::Failed {
+                                            reason: "signed refs have diverged".to_owned()
+                                        }
+                                    );
+                                }
+                                Err(e) => {
+                                    return (
+                                        remote_id,
+                                        VerifiedRemote::Failed { reason: e.to_string() },
+                                    );
+                                }
+                            }
+                        } else {
+                            return (remote_id, VerifiedRemote::UpToDate);
+                        }
+                    }
+                }
+
                 let verification = match self.repo.identity_doc_of(&remote_id) {
                     Ok(doc) => match self.repo.validate_remote(&remote) {
                         Ok(unsigned) => VerifiedRemote::Success {
@@ -446,7 +528,9 @@ impl<'a> StagingPhaseFinal<'a> {
                 };
                 (remote_id, verification)
             })
-            .collect())
+            .collect();
+
+        Ok(result)
     }
 }
 

radicle-node/src/worker/fetch/refspecs.rs

@@ -42,18 +42,15 @@ impl AsRefspecs for SpecialRefs {
             Namespaces::All => {
                 let id = NAMESPACES_GLOB.join(&*IDENTITY_BRANCH);
                 let sigrefs = NAMESPACES_GLOB.join(&*SIGREFS_BRANCH);
-                vec![
+
-                    Refspec {
+                [id, sigrefs]
-                        src: id.clone(),
+                    .into_iter()
-                        dst: id,
+                    .map(|spec| Refspec {
-                        force: false,
+                        src: spec.clone(),
-                    },
+                        dst: spec,
-                    Refspec {
+                        force: true,
-                        src: sigrefs.clone(),
+                    })
-                        dst: sigrefs,
+                    .collect()
-                        force: false,
-                    },
-                ]
             }
             Namespaces::Trusted(pks) => pks.iter().flat_map(rad_refs).collect(),
         }
@@ -70,13 +67,13 @@ fn rad_refs(pk: &PublicKey) -> Vec<Refspec<git::PatternString, git::PatternStrin
     let id = Refspec {
         src: id.clone(),
         dst: id,
-        force: false,
+        force: true,
     };
     let sigrefs = git::PatternString::from(ns.join(&*SIGREFS_BRANCH));
     let sigrefs = Refspec {
         src: sigrefs.clone(),
         dst: sigrefs,
-        force: false,
+        force: true,
     };
     vec![id, sigrefs]
 }
@@ -104,7 +101,7 @@ impl AsRefspecs for Namespaces {
             Namespaces::All => vec![Refspec {
                 src: (*storage::git::NAMESPACES_GLOB).clone(),
                 dst: (*storage::git::NAMESPACES_GLOB).clone(),
-                force: false,
+                force: true,
             }],
             Namespaces::Trusted(pks) => pks
                 .iter()
@@ -113,7 +110,7 @@ impl AsRefspecs for Namespaces {
                     Refspec {
                         src: ns.clone(),
                         dst: ns,
-                        force: false,
+                        force: true,
                     }
                 })
                 .collect(),
@@ -163,19 +160,11 @@ impl AsRefspecs for Remote {
 
 impl<'a> AsRefspecs for BTreeSet<git::Namespaced<'a>> {
     fn as_refspecs(&self) -> Vec<Refspec<git::PatternString, git::PatternString>> {
-        let reserved = [(*IDENTITY_BRANCH).clone(), (*SIGREFS_BRANCH).clone()]
-            .into_iter()
-            .collect::<BTreeSet<_>>();
         self.iter()
-            .map(|r| {
+            .map(|r| Refspec {
-                // Only force ordinary refs.
+                src: r.clone().to_ref_string().into(),
-                let suffix = r.strip_namespace();
+                dst: r.clone().to_ref_string().into(),
-                let force = !reserved.contains(&suffix);
+                force: true,
-                Refspec {
-                    src: r.clone().to_ref_string().into(),
-                    dst: r.clone().to_ref_string().into(),
-                    force,
-                }
             })
             .collect()
     }