From 119d48bbbca6c5b1001b85cf1c986934a6721009 Mon Sep 17 00:00:00 2001
From: Fintan Halpenny <fintan.halpenny@gmail.com>
Date: Fri, 5 Jan 2024 17:28:56 +0000
Subject: [PATCH] fetch: always fetch canonical rad/id

The following scenario was noticed:

If a node has a `rad/id` that is behind the one it is fetching from,
where a delegate was added, then verification will fail since it will
not fetch the new delegate -- if the scope is set to `Followed`.

Irregardless if the `rad/id` is ahead or behind, the reference should
be fetched to use as the delegates anchor since it will tell the
fetching side which delegates in can fetch from the remote. If any of
those delegates are missing then the remote is indeed lying.

This is safe since the `refs/rad/id` reference is only updated by
computing the COB rather than setting the OID, so it will not update
through the fetch protocol itself.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
X-Clacks-Overhead: GNU Terry Pratchett
---
 radicle-fetch/src/lib.rs   | 16 +++-------------
 radicle-fetch/src/state.rs | 12 ++++++++++++
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/radicle-fetch/src/lib.rs b/radicle-fetch/src/lib.rs
index 41b07d54..376e2bb2 100644
--- a/radicle-fetch/src/lib.rs
+++ b/radicle-fetch/src/lib.rs
@@ -59,11 +59,12 @@ where
     if local == remote {
         return Err(Error::ReplicateSelf);
     }
-    let state = FetchState::default();
     let handshake = handle
         .transport
         .handshake()
         .map_err(|err| Error::Handshake { err })?;
+    let state = FetchState::default();
+
     // N.b. ensure that we ignore the local peer's key.
     handle.blocked.extend([local]);
     state
@@ -90,18 +91,7 @@ where
         .transport
         .handshake()
         .map_err(|err| Error::Handshake { err })?;
-    let mut state = FetchState::default();
-    state
-        .run_stage(
-            handle,
-            &handshake,
-            &stage::CanonicalId {
-                remote,
-                limit: limit.special,
-            },
-        )
-        .map_err(|e| Error::from(state::error::Protocol::from(e)))?;
-
+    let state = FetchState::default();
     state
         .run(handle, &handshake, limit, remote, None)
         .map_err(Error::Protocol)
diff --git a/radicle-fetch/src/state.rs b/radicle-fetch/src/state.rs
index 6495d9c9..218bcc95 100644
--- a/radicle-fetch/src/state.rs
+++ b/radicle-fetch/src/state.rs
@@ -378,6 +378,18 @@ impl FetchState {
     where
         S: transport::ConnectionStream,
     {
+        // N.b. we always fetch the `rad/id` since our delegate set
+        // might be further ahead than theirs, e.g. we are the
+        // deciding vote on adding a delegate.
+        self.run_stage(
+            handle,
+            &handshake,
+            &stage::CanonicalId {
+                remote,
+                limit: limit.special,
+            },
+        )?;
+
         // N.b. The error case here should not happen. In the case of
         // a `clone` we have asked for refs/rad/id and ensured it was
         // fetched. In the case of `pull` the repository should have