From 9a4539fe82f5a0d3afa3659116fbd1cf074ca866 Mon Sep 17 00:00:00 2001 From: Lorenz Leutgeb Date: Sat, 21 Mar 2026 00:24:52 +0100 Subject: [PATCH] radicle/sigrefs: Find first non-replayed commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scenario A₁ ← B₁ ← A₂ ← B₂ would previously have resulted in A₁ being loaded. This is undesriable, since the latest non-replayed commit is B₁. The information regarding the order of duplicates, which is preserved by `seen`, can be used to recover the first commit. --- CHANGELOG.md | 8 ++ .../radicle/src/storage/refs/sigrefs/read.rs | 43 ++++--- .../sigrefs/read/test/signed_refs_reader.rs | 112 ++++++++---------- 3 files changed, 84 insertions(+), 79 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c313a07..1b982ad7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +## Fixed Bugs + +- Fix the signed references reading process by correctly choosing the first, + non-replayed commit. This only occurs if duplicate signatures are found and + the process needs to find the first legitimate commit of the namespace. + ## 1.7.1 ## Fixed Bugs diff --git a/crates/radicle/src/storage/refs/sigrefs/read.rs b/crates/radicle/src/storage/refs/sigrefs/read.rs index d6acfc08..e7ca2ab7 100644 --- a/crates/radicle/src/storage/refs/sigrefs/read.rs +++ b/crates/radicle/src/storage/refs/sigrefs/read.rs @@ -101,16 +101,17 @@ where } } - /// Read a [`VerifiedCommit`] using the [`SignedRefsReader`]. + /// Read a [`VerifiedCommit`] using the [`SignedRefsReader`], from a + /// linear history. /// - /// The [`VerifiedCommit`] will be the first commit, if the commit verifies + /// The [`VerifiedCommit`] will be the latest commit, if the commit verifies /// and contains its parent in its [`Refs`] entry. + /// /// If the commit does not contain a parent, but its signature is not /// repeated, then it is still returned. - /// Otherwise, the commit that is returned is either: - /// - The first commit which has no repeated signatures, i.e. it has no replay attacks. - /// - The first commit which is not a replay commit, i.e. the commit that - /// replay attacks are based on. + /// + /// Otherwise, the latest commit that has no duplicate signatures in its + /// ancestry is returned. /// /// # Replay Attacks /// @@ -145,6 +146,10 @@ where return Ok(head); } + // Note that for all sets of commits which share the same signature, + // the `NonEmpty` in `seen` will be in reverse order of the walk. + // That is, the latest commit in the set will be at the first position, + // and the earliest commit will be at the last position. let seen = iter::Walk::new(*head.id(), self.repository).try_fold( HashMap::>::new(), |mut seen, commit| { @@ -181,7 +186,6 @@ where // The second walk can start from the parent of head. We do not need to // verify head twice, and we already know that the parent exists. - let mut last = None; for commit in iter::Walk::new(parent, self.repository) { let commit = commit .map_err(error::Read::Commit)? @@ -192,19 +196,26 @@ where if commits.len_nonzero() == ONE { return Ok(commit); - } else { + } + + let id = commit.id(); + + if id == commits.last() { + // If this commit is the last element of `commits`, + // then this means it is the earliest of all that share + // its signature. It thus cannot have been replayed. + return Ok(commit); + } + + if id == commits.first() { + // We only log one warning per set of duplicates, and that is + // when we reach the first element of `commits`, which is the + // latest in the history. log::warn!("Duplicate sigrefs found in commits {commits:?}"); - last = Some(commit); } } - // In the extreme case where all commits in the walk contain duplicate - // signatures, return the oldest commit reached — the last one visited - // by the walk, which follows the chain from newest to oldest. - // `last` is always `Some` here because `parent` is guaranteed to exist - // (head had a duplicate signature, so it must have a parent), meaning - // the walk yields at least one commit. - Ok(last.unwrap_or(head)) + unreachable!() } fn resolve_tip(&self) -> Result { diff --git a/crates/radicle/src/storage/refs/sigrefs/read/test/signed_refs_reader.rs b/crates/radicle/src/storage/refs/sigrefs/read/test/signed_refs_reader.rs index 6e530b71..78b7a12a 100644 --- a/crates/radicle/src/storage/refs/sigrefs/read/test/signed_refs_reader.rs +++ b/crates/radicle/src/storage/refs/sigrefs/read/test/signed_refs_reader.rs @@ -165,77 +165,63 @@ fn two_commits() { assert_eq!(*vc.id(), head); } -/// Commit chain: -/// `A <- B <- A` -/// Where A is replayed +/// We test a handful scenarios with replayed commits (or rather, references +/// and signatures within commits). /// -/// Expected Result: B -#[test] -fn head_replays_root() { - const SIGNATURE_A: u8 = 1; - const SIGNATURE_B: u8 = 2; +/// For every test we define: +/// - A history, which is a linear history of commits, +/// where the earliest and leftmost commit is a root commit. +/// - Which commit we expect to be loaded, as a zero based index in the +/// history. +mod replay { + use super::*; - let a1 = mock::oid(1); - let b = mock::oid(2); - let a2 = mock::oid(3); - let repo = mock::setup_chain([ - (a1, SIGNATURE_A, refs_without_parent(mock::oid(10))), - (b, SIGNATURE_B, refs_without_parent(mock::oid(10))), - (a2, SIGNATURE_A, refs_without_parent(mock::oid(10))), - ]); + /// Mocks a chain of commits, where their OID is their zero-based index + /// in `chain` (note that since this is only mocked, it is not an issue + /// that the first commit in the chain, at index zero, is identified by + /// the zero OID). + /// + /// Asserts that the result of [`read`] on the chain is `expected`. + fn replay(chain: impl IntoIterator, expected: u8) { + let refs = refs_without_parent(mock::oid(10)); - let vc = read(a2, repo).unwrap(); - assert_eq!(*vc.id(), b); -} + let chain: Vec<_> = chain.into_iter().collect(); + let mut repo = MockRepository::new(); + let mut parent = None; + for (i, signature) in chain.iter().enumerate() { + let i = mock::oid(i as u8); + repo = repo + .with_commit(i, mock::commit_data(parent)) + .with_refs(i, refs.clone()) + .with_signature(i, *signature); + parent = Some(i); + } -/// Commit chain: -/// `A <- A <- A` -/// Where A is replayed twice -/// -/// Expected Result: first A -#[test] -fn replay_chain() { - const SIGNATURE_A: u8 = 1; - let a1 = mock::oid(1); - let a2 = mock::oid(2); - let a3 = mock::oid(3); - let repo = mock::setup_chain([ - (a1, SIGNATURE_A, refs_without_parent(mock::oid(10))), - (a2, SIGNATURE_A, refs_without_parent(mock::oid(10))), - (a3, SIGNATURE_A, refs_without_parent(mock::oid(10))), - ]); + assert_eq!( + *read(mock::oid((chain.len() - 1) as u8), repo).unwrap().id(), + mock::oid(expected) + ) + } - let vc = read(a3, repo).unwrap(); - assert_eq!(*vc.id(), a1); -} + #[test] + fn root_at_head() { + replay([1, 2, 1], 1) + } -/// Commit chain: -/// `C <- C <- B <- A <- A` -/// Where C and A are replayed twice -/// -/// Expected Result: first A -#[test] -fn multiple_replays() { - const SIGNATURE_A: u8 = 3; - const SIGNATURE_B: u8 = 2; - const SIGNATURE_C: u8 = 1; + #[test] + fn chain() { + replay([1, 1, 1], 0) + } - let c1 = mock::oid(1); - let c2 = mock::oid(2); - let b = mock::oid(3); - let a1 = mock::oid(4); - let a2 = mock::oid(5); - let r = refs_without_parent(mock::oid(10)); - let repo = mock::setup_chain([ - (a2, SIGNATURE_C, r.clone()), - (a1, SIGNATURE_C, r.clone()), - (b, SIGNATURE_B, r.clone()), - (c2, SIGNATURE_A, r.clone()), - (c1, SIGNATURE_A, r), - ]); + #[test] + fn multiple() { + replay([1, 1, 2, 3, 3], 3) + } - let vc = read(c1, repo).unwrap(); - assert_eq!(*vc.id(), b); + #[test] + fn alternating() { + replay([1, 2, 1, 2], 1) + } } #[test]