From 703a6d80250d7cac6584811fc4553746b4e46bd7 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Tue, 7 Nov 2023 14:35:03 +0000 Subject: [PATCH] fetch: Ignore sigrefs that are behind Previously, if a delegate's namespace was fetched and its rad/sigrefs was behind the current state, then it would fail to perform the fetch. Instead, when the ancestry path is checked the outcome can be used during the update: 1. If the tip is equal, then the update can be marked as skipped. 2. If the tip is behind, then the update can be rejected, iff the policy does not allow it. 3. If the tip is ahead, then the update is a fast-forward. 4. If the tip is diverged, then the update results in a failure, iff the policy does not allow it. The protocol also prunes any delegates's updates when the `rad/sigrefs` is behind and returns an error if they are diverged. For any non-delegates, it will simply prune them. Signed-off-by: Fintan Halpenny --- radicle-fetch/src/git/repository.rs | 162 ++++++++++++++-------- radicle-fetch/src/git/repository/error.rs | 4 +- radicle-fetch/src/state.rs | 39 ++++++ radicle-node/src/tests/e2e.rs | 92 +++++++++++- 4 files changed, 237 insertions(+), 60 deletions(-) diff --git a/radicle-fetch/src/git/repository.rs b/radicle-fetch/src/git/repository.rs index bdc6f83f..a219a820 100644 --- a/radicle-fetch/src/git/repository.rs +++ b/radicle-fetch/src/git/repository.rs @@ -1,15 +1,36 @@ pub mod error; -use either::{ - Either, - Either::{Left, Right}, -}; +use either::Either; use radicle::git::{Namespaced, Oid, Qualified}; use radicle::storage::git::Repository; -use radicle::storage::ReadRepository; use super::refs::{Applied, Policy, RefUpdate, Update}; +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Ancestry { + Equal, + Ahead, + Behind, + Diverged, +} + +pub enum Updated<'a> { + Accepted(RefUpdate), + Rejected(Update<'a>), +} + +impl<'a> From for Updated<'a> { + fn from(up: RefUpdate) -> Self { + Updated::Accepted(up) + } +} + +impl<'a> From> for Updated<'a> { + fn from(up: Update<'a>) -> Self { + Updated::Rejected(up) + } +} + pub fn contains(repo: &Repository, oid: Oid) -> Result { repo.backend .odb() @@ -17,17 +38,27 @@ pub fn contains(repo: &Repository, oid: Oid) -> Result { .map_err(error::Contains) } -pub fn is_in_ancestry_path(repo: &Repository, old: Oid, new: Oid) -> Result { +pub fn ancestry(repo: &Repository, old: Oid, new: Oid) -> Result { if !contains(repo, old)? || !contains(repo, new)? { - return Ok(false); + return Err(error::Ancestry::Missing { a: old, b: new }); } if old == new { - return Ok(true); + return Ok(Ancestry::Equal); } - repo.is_ancestor_of(old, new) - .map_err(|err| error::Ancestry::Check { old, new, err }) + let (ahead, behind) = repo + .backend + .graph_ahead_behind(new.into(), old.into()) + .map_err(|err| error::Ancestry::Check { old, new, err })?; + + if ahead > 0 && behind == 0 { + Ok(Ancestry::Ahead) + } else if ahead == 0 && behind > 0 { + Ok(Ancestry::Behind) + } else { + Ok(Ancestry::Diverged) + } } pub fn refname_to_id<'a, N>(repo: &Repository, refname: N) -> Result, error::Resolve> @@ -59,12 +90,12 @@ where target, no_ff, } => match direct(repo, name, target, no_ff)? { - Left(r) => applied.rejected.push(r), - Right(u) => applied.updated.push(u), + Updated::Rejected(r) => applied.rejected.push(r), + Updated::Accepted(u) => applied.updated.push(u), }, Update::Prune { name, prev } => match prune(repo, name, prev)? { - Left(r) => applied.rejected.push(r), - Right(u) => applied.updated.push(u), + Updated::Rejected(r) => applied.rejected.push(r), + Updated::Accepted(u) => applied.updated.push(u), }, } } @@ -77,49 +108,62 @@ fn direct<'a>( name: Namespaced<'a>, target: Oid, no_ff: Policy, -) -> Result, RefUpdate>, error::Update> { +) -> Result, error::Update> { let tip = refname_to_id(repo, name.clone())?; match tip { Some(prev) => { - let is_ff = is_in_ancestry_path(repo, prev, target)?; - if !is_ff { - match no_ff { - Policy::Abort => { - return Err(error::Update::NonFF { - name: name.to_owned(), - new: target, - cur: prev, - }) - } - Policy::Reject => Ok(Left(Update::Direct { - name, - target, - no_ff, - })), - Policy::Allow => { - // N.b. the update is a non-fast-forward but - // we allow it, so we pass `force: true`. - repo.backend - .reference(name.as_ref(), target.into(), true, "radicle: update") - .map_err(|err| error::Update::Create { - name: name.to_owned(), - target, - err, - })?; - Ok(Right(RefUpdate::from(name.to_ref_string(), prev, target))) - } + let ancestry = ancestry(repo, prev, target)?; + + match ancestry { + Ancestry::Equal => Ok(RefUpdate::Skipped { + name: name.to_ref_string(), + oid: target, } - } else { - // N.b. the update is a fast-forward so we can safely - // pass `force: true`. - repo.backend - .reference(name.as_ref(), target.into(), true, "radicle: update") - .map_err(|err| error::Update::Create { + .into()), + Ancestry::Ahead => { + // N.b. the update is a fast-forward so we can safely + // pass `force: true`. + repo.backend + .reference(name.as_ref(), target.into(), true, "radicle: update") + .map_err(|err| error::Update::Create { + name: name.to_owned(), + target, + err, + })?; + Ok(RefUpdate::from(name.to_ref_string(), prev, target).into()) + } + Ancestry::Behind | Ancestry::Diverged if matches!(no_ff, Policy::Allow) => { + // N.b. the update is a non-fast-forward but + // we allow it, so we pass `force: true`. + repo.backend + .reference(name.as_ref(), target.into(), true, "radicle: update") + .map_err(|err| error::Update::Create { + name: name.to_owned(), + target, + err, + })?; + Ok(RefUpdate::from(name.to_ref_string(), prev, target).into()) + } + // N.b. if the target is behind, we simply reject the update + Ancestry::Behind => Ok(Update::Direct { + name, + target, + no_ff, + } + .into()), + Ancestry::Diverged if matches!(no_ff, Policy::Reject) => Ok(Update::Direct { + name, + target, + no_ff, + } + .into()), + Ancestry::Diverged => { + return Err(error::Update::NonFF { name: name.to_owned(), - target, - err, - })?; - Ok(Right(RefUpdate::from(name.to_ref_string(), prev, target))) + new: target, + cur: prev, + }) + } } } None => { @@ -132,10 +176,11 @@ fn direct<'a>( target, err, })?; - Ok(Right(RefUpdate::Created { + Ok(RefUpdate::Created { name: name.to_ref_string(), oid: target, - })) + } + .into()) } } } @@ -144,7 +189,7 @@ fn prune<'a>( repo: &Repository, name: Namespaced<'a>, prev: Either>, -) -> Result, RefUpdate>, error::Update> { +) -> Result, error::Update> { use radicle::git::raw::ObjectType; match find(repo, &name)? { @@ -160,12 +205,13 @@ fn prune<'a>( name: name.to_owned(), err, })?; - Ok(Right(RefUpdate::Deleted { + Ok(RefUpdate::Deleted { name: name.to_ref_string(), oid: prev, - })) + } + .into()) } - None => Ok(Left(Update::Prune { name, prev })), + None => Ok(Update::Prune { name, prev }.into()), } } diff --git a/radicle-fetch/src/git/repository/error.rs b/radicle-fetch/src/git/repository/error.rs index effb6ca9..12525ce3 100644 --- a/radicle-fetch/src/git/repository/error.rs +++ b/radicle-fetch/src/git/repository/error.rs @@ -7,6 +7,8 @@ pub struct Contains(#[source] pub raw::Error); #[derive(Debug, Error)] pub enum Ancestry { + #[error("missing one of {a} or {b} while checking ancestry")] + Missing { a: Oid, b: Oid }, #[error(transparent)] Contains(#[from] Contains), #[error("failed to check ancestry for {old} and {new}")] @@ -14,7 +16,7 @@ pub enum Ancestry { old: Oid, new: Oid, #[source] - err: ext::Error, + err: raw::Error, }, } diff --git a/radicle-fetch/src/state.rs b/radicle-fetch/src/state.rs index 874b1c08..90f95a7b 100644 --- a/radicle-fetch/src/state.rs +++ b/radicle-fetch/src/state.rs @@ -29,6 +29,8 @@ pub const DEFAULT_FETCH_DATA_REFS_LIMIT: u64 = 1024 * 1024 * 1024 * 5; pub mod error { use std::io; + use radicle::git::Oid; + use radicle::prelude::PublicKey; use thiserror::Error; use crate::{git, git::repository, handle, sigrefs, stage}; @@ -47,8 +49,16 @@ pub mod error { #[derive(Debug, Error)] pub enum Protocol { + #[error(transparent)] + Ancestry(#[from] repository::error::Ancestry), #[error(transparent)] Canonical(#[from] Canonical), + #[error("delegate '{remote}' has diverged 'rad/sigrefs': {current} -> {received}")] + Diverged { + remote: PublicKey, + current: Oid, + received: Oid, + }, #[error(transparent)] Io(#[from] io::Error), #[error("canonical 'refs/rad/id' is missing")] @@ -371,6 +381,20 @@ impl FetchState { remote, data: Some(sigrefs), } => { + if let Some(SignedRefsAt { at, .. }) = SignedRefsAt::load(remote, &handle.repo)? + { + // Prune non-delegates if they're behind or + // diverged. A diverged case is non-fatal for + // delegates. + if matches!( + repository::ancestry(&handle.repo, at, sigrefs.at)?, + repository::Ancestry::Behind | repository::Ancestry::Diverged + ) { + self.prune(&remote); + continue; + } + } + let cache = self.as_cached(handle); if let Some(warns) = sigrefs::validate(&cache, sigrefs)?.as_mut() { log::debug!( @@ -385,6 +409,21 @@ impl FetchState { remote, data: Some(sigrefs), } => { + if let Some(SignedRefsAt { at, .. }) = SignedRefsAt::load(remote, &handle.repo)? + { + let ancestry = repository::ancestry(&handle.repo, at, sigrefs.at)?; + if matches!(ancestry, repository::Ancestry::Behind) { + self.prune(&remote); + continue; + } else if matches!(ancestry, repository::Ancestry::Diverged) { + return Err(error::Protocol::Diverged { + remote, + current: at, + received: sigrefs.at, + }); + } + } + let cache = self.as_cached(handle); if let Some(fails) = sigrefs::validate(&cache, sigrefs)?.as_mut() { log::warn!(target: "fetch", "Pruning delegate {remote} tips, due to validation failures"); diff --git a/radicle-node/src/tests/e2e.rs b/radicle-node/src/tests/e2e.rs index 6c12f58b..4639d87e 100644 --- a/radicle-node/src/tests/e2e.rs +++ b/radicle-node/src/tests/e2e.rs @@ -855,7 +855,7 @@ fn test_non_fastforward_sigrefs() { // Eve has old refs. assert_matches!( alice.handle.fetch(rid, eve.id, DEFAULT_TIMEOUT).unwrap(), - FetchResult::Failed { .. } + FetchResult::Success { updated, .. } if updated.is_empty() ); } @@ -949,3 +949,93 @@ fn test_outdated_sigrefs() { assert_ne!(eves_refs, old_refs); assert_eq!(eves_refs_expected, eves_refs); } + +#[test] +fn test_outdated_delegate_sigrefs() { + logger::init(log::Level::Debug); + + let tmp = tempfile::tempdir().unwrap(); + + let mut alice = Node::init(tmp.path(), Config::test(Alias::new("alice"))); + let bob = Node::init(tmp.path(), Config::test(Alias::new("bob"))); + let eve = Node::init(tmp.path(), Config::test(Alias::new("eve"))); + + let rid = alice.project("acme", ""); + + let mut alice = alice.spawn(); + let mut bob = bob.spawn(); + let mut eve = eve.spawn(); + + bob.handle.track_repo(rid, Scope::All).unwrap(); + eve.handle.track_repo(rid, Scope::All).unwrap(); + alice.connect(&bob); + bob.connect(&eve); + eve.connect(&alice); + converge([&alice, &bob, &eve]); + + bob.handle.fetch(rid, alice.id, DEFAULT_TIMEOUT).unwrap(); + assert!(bob.storage.contains(&rid).unwrap()); + rad::fork(rid, &bob.signer, &bob.storage).unwrap(); + + eve.handle.fetch(rid, alice.id, DEFAULT_TIMEOUT).unwrap(); + assert!(eve.storage.contains(&rid).unwrap()); + rad::fork(rid, &eve.signer, &eve.storage).unwrap(); + + alice + .handle + .track_node(eve.id, Some(Alias::new("eve"))) + .unwrap(); + alice.handle.fetch(rid, eve.id, DEFAULT_TIMEOUT).unwrap(); + let repo = alice.storage.repository(rid).unwrap(); + assert!(repo.remote(&eve.id).is_ok()); + + log::debug!(target: "test", "Bob fetches from Eve.."); + assert_matches!( + bob.handle.fetch(rid, eve.id, DEFAULT_TIMEOUT).unwrap(), + FetchResult::Success { .. } + ); + let repo = bob.storage.repository(rid).unwrap(); + let alice_remote = repo.remote(&alice.id).unwrap(); + let old_refs = alice_remote.refs; + + // At this stage, Alice and Bob have Eve's fork and Eve does not + // have Bob's fork + + alice.issue( + rid, + "Outdated Sigrefs", + "Outdated sigrefs are harshing my vibes", + ); + let repo = alice.storage.repository(rid).unwrap(); + let alice_refs = repo.remote(&alice.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!( + eve.handle.fetch(rid, alice.id, DEFAULT_TIMEOUT).unwrap(), + FetchResult::Success { .. } + ); + let repo = eve.storage.repository(rid).unwrap(); + let alice_remote = repo.remote(&alice.id).unwrap(); + let alice_refs_expected = alice_remote.refs; + assert_ne!(alice_refs_expected, old_refs); + assert_eq!(alice_refs_expected, alice_refs); + + log::debug!(target: "test", "Alice fetches from Bob.."); + + eve.handle + .track_node(bob.id, Some(Alias::new("bob"))) + .unwrap(); + assert_matches!( + eve.handle.fetch(rid, bob.id, DEFAULT_TIMEOUT).unwrap(), + FetchResult::Success { .. } + ); + + // Ensure that Eve's refs have not changed after fetching the old refs from Bob. + let repo = eve.storage.repository(rid).unwrap(); + let alice_remote = repo.remote(&alice.id).unwrap(); + let alice_refs = alice_remote.refs; + + assert_ne!(alice_refs, old_refs); + assert_eq!(alice_refs_expected, alice_refs); +}