diff --git a/CHANGELOG.md b/CHANGELOG.md index 76f0731c3..675a94bca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,16 @@ 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 + +## Fixes + +- A patch now merges on the quorum of the canonical reference rule that + matches its target branch, not on the identity document threshold. + Only merges by DIDs in the rule's `allow` set count, and they must + reach the rule's `threshold`. A patch that targets a branch which no + rule matches cannot merge. + ## 1.10.3 - 2026-09-07 ### Fixes diff --git a/crates/radicle/src/cob/patch.rs b/crates/radicle/src/cob/patch.rs index 7b83ca4cf..34abef3b4 100644 --- a/crates/radicle/src/cob/patch.rs +++ b/crates/radicle/src/cob/patch.rs @@ -627,10 +627,13 @@ impl Patch { /// /// If a custom target was specified, it returns that branch. /// Otherwise, it falls back to the project's default branch. - pub fn merge_target_branch<'a>( + pub fn merge_target_branch<'a, 'b>( &'a self, - doc: &'a Doc, - ) -> Result, DefaultBranchError> { + doc: &'b Doc, + ) -> Result, DefaultBranchError> + where + 'b: 'a, + { match &self.target { MergeTarget::Delegates => Ok(doc.default_branch()?), MergeTarget::Branch(branch) => Ok(branch.clone().into_qualified()), @@ -1274,17 +1277,45 @@ impl Patch { }, ); - let mut merges = self.merges.iter().fold( - HashMap::<(RevisionId, git::Oid), usize>::new(), - |mut acc, (_, merge)| { - *acc.entry((merge.revision, merge.commit)).or_default() += 1; - acc - }, - ); - // Discard revisions that weren't merged by a threshold of delegates. - merges.retain(|_, count| *count >= identity.threshold()); + // The quorum for the target branch is defined by the canonical + // reference rule that matches it: only merges made by an allowed + // DID count, and the rule's threshold must be reached. + // + // A branch that no rule matches never becomes a canonical + // reference, so a patch targeting it can never reach a quorum. + // + // As above, a merge we cannot evaluate is skipped rather + // than rejected. + let Ok(crefs) = identity.canonical_refs() else { + return Ok(()); + }; + let Some((_, rule)) = crefs.rules().matches(&expected_branch).next() else { + return Ok(()); + }; - match merges.into_keys().collect::>().as_slice() { + let merges = self + .merges + .iter() + .filter_map(|(actor, merge)| { + rule.allowed().contains(&Did::from(*actor)).then_some(merge) + }) + .fold( + HashMap::<(RevisionId, git::Oid), usize>::new(), + |mut acc, merge| { + *acc.entry((merge.revision, merge.commit)).or_default() += 1; + acc + }, + ); + + match merges + .into_iter() + .filter_map(|(merge, count)| { + // Discard revisions that were not merged by a threshold of allowed DIDs. + (count >= usize::from(*rule.threshold())).then_some(merge) + }) + .collect::>() + .as_slice() + { [] => { // None of the revisions met the quorum. } @@ -3370,6 +3401,274 @@ mod test { ); } + /// Build a patch targeting `refs/heads/accepted`, together with the + /// document and repository needed to apply merges to it. + /// + /// The document has four delegates and a threshold of three, while the + /// rule matching the target branch allows only Alice and Bob, with a + /// threshold of two. The two quorums are distinguishable. + fn merge_quorum_fixture( + signers: &[&SigningKey], + commit: git::Oid, + ) -> (Patch, Doc, MockRepository, RevisionId) { + let base = git::Oid::from_str("cb18e95ada2bb38aadd8e6cef0963ce37a87add3").unwrap(); + let [alice, bob, eve, dave] = signers else { + panic!("merge_quorum_fixture: expected four signers"); + }; + + let mut raw_doc = RawDoc::new( + r#gen::(1), + vec![alice.did(), bob.did(), eve.did(), dave.did()], + 3, + identity::Visibility::Public, + ); + let crefs = serde_json::json!({ + "rules": { + "refs/heads/accepted": { + "allow": [alice.did(), bob.did()], + "threshold": 2 + } + } + }); + raw_doc.payload.insert( + identity::doc::PayloadId::canonical_refs().clone(), + identity::doc::Payload::from(crefs), + ); + let doc = raw_doc.verified().unwrap(); + + // Every signer publishes the merge commit on the target branch, so that + // the merge is not skipped for being unreachable. + let branch = git::fmt::qualified!("refs/heads/accepted"); + let mut repo = MockRepository::new(r#gen::(1), doc.clone()); + for signer in signers { + let mut refs = crate::storage::refs::Refs::new(); + refs.insert(branch.to_ref_string(), commit); + repo.remotes.insert( + *signer.public_key(), + crate::storage::refs::arbitrary::signed_refs(refs, commit, *signer), + ); + } + + let revision_id = RevisionId(arbitrary::entry_id()); + let patch = Patch::new( + cob::Title::new("My Patch").unwrap(), + MergeTarget::Branch(TargetBranch::try_from("accepted").unwrap()), + ( + revision_id, + Revision::new( + revision_id, + Author::new(alice.did()), + String::new(), + base, + commit, + env::local_time().into(), + Default::default(), + ), + ), + ); + + (patch, doc, repo, revision_id) + } + + fn apply_merge( + patch: &mut Patch, + doc: &Doc, + repo: &MockRepository, + revision: RevisionId, + commit: git::Oid, + signer: &SigningKey, + ) { + patch + .action( + Action::Merge { revision, commit }, + arbitrary::entry_id(), + *signer.public_key(), + env::local_time().into(), + &[], + doc, + repo, + ) + .unwrap(); + } + + #[test] + fn test_patch_merge_quorum_uses_rule_threshold() { + let commit = git::Oid::from_str("518d5069f94c03427f694bb494ac1cd7d1339380").unwrap(); + let alice = SigningKey::mock(14); + let bob = SigningKey::mock(23); + let eve = SigningKey::mock(31); + let dave = SigningKey::mock(42); + let signers = [&alice, &bob, &eve, &dave]; + + let (mut patch, doc, repo, revision) = merge_quorum_fixture(&signers, commit); + + apply_merge(&mut patch, &doc, &repo, revision, commit, &alice); + assert!( + !patch.is_merged(), + "one of two allowed merges is not a quorum" + ); + + // Two allowed merges reach the rule's threshold, even though the + // document threshold of three is not met. + apply_merge(&mut patch, &doc, &repo, revision, commit, &bob); + assert_eq!( + *patch.state(), + State::Merged { revision, commit }, + "the rule's threshold of two is reached" + ); + } + + #[test] + fn test_patch_merge_quorum_ignores_disallowed_merges() { + let commit = git::Oid::from_str("518d5069f94c03427f694bb494ac1cd7d1339380").unwrap(); + let alice = SigningKey::mock(14); + let bob = SigningKey::mock(23); + let eve = SigningKey::mock(31); + let dave = SigningKey::mock(42); + let signers = [&alice, &bob, &eve, &dave]; + + let (mut patch, doc, repo, revision) = merge_quorum_fixture(&signers, commit); + + // Eve and Dave are delegates, but the rule does not allow them, so + // their merges never count towards the quorum. + apply_merge(&mut patch, &doc, &repo, revision, commit, &eve); + apply_merge(&mut patch, &doc, &repo, revision, commit, &dave); + apply_merge(&mut patch, &doc, &repo, revision, commit, &alice); + + assert_eq!(patch.merges.len(), 3, "all three merges are recorded"); + assert!( + !patch.is_merged(), + "only Alice's merge counts, which is below the rule's threshold" + ); + } + + #[test] + fn test_patch_merge_quorum_without_matching_rule() { + let base = git::Oid::from_str("cb18e95ada2bb38aadd8e6cef0963ce37a87add3").unwrap(); + let commit = git::Oid::from_str("518d5069f94c03427f694bb494ac1cd7d1339380").unwrap(); + let alice = SigningKey::mock(14); + + // The only rule is the one synthesized for the default branch, so + // nothing matches `refs/heads/staging`. + let project = Project::new( + ProjectName::from_str("unruled").unwrap(), + String::new(), + BranchName::from(git::fmt::RefString::try_from("master").unwrap()), + ) + .unwrap(); + let doc = RawDoc::new(project, vec![alice.did()], 1, identity::Visibility::Public) + .verified() + .unwrap(); + + let branch = git::fmt::qualified!("refs/heads/staging"); + let mut repo = MockRepository::new(r#gen::(1), doc.clone()); + let mut refs = crate::storage::refs::Refs::new(); + refs.insert(branch.to_ref_string(), commit); + repo.remotes.insert( + *alice.public_key(), + crate::storage::refs::arbitrary::signed_refs(refs, commit, &alice), + ); + + let revision = RevisionId(arbitrary::entry_id()); + let mut patch = Patch::new( + cob::Title::new("My Patch").unwrap(), + MergeTarget::Branch(TargetBranch::try_from("staging").unwrap()), + ( + revision, + Revision::new( + revision, + Author::new(alice.did()), + String::new(), + base, + commit, + env::local_time().into(), + Default::default(), + ), + ), + ); + + // Alice is the only delegate, so she meets the document threshold on + // her own, but no rule makes `refs/heads/staging` canonical. + apply_merge(&mut patch, &doc, &repo, revision, commit, &alice); + + assert_eq!(patch.merges.len(), 1, "the merge is recorded"); + assert!( + !patch.is_merged(), + "a branch that no rule matches can never reach a quorum" + ); + } + + /// The default branch keeps the quorum it had before rules were consulted. + /// + /// Without a `xyz.radicle.crefs` payload, the rule for the default branch + /// is synthesized as `delegates` at the document threshold, so the tally + /// must behave exactly as it did when it used the document directly. + #[test] + fn test_patch_merge_quorum_of_default_branch() { + let base = git::Oid::from_str("cb18e95ada2bb38aadd8e6cef0963ce37a87add3").unwrap(); + let commit = git::Oid::from_str("518d5069f94c03427f694bb494ac1cd7d1339380").unwrap(); + let alice = SigningKey::mock(14); + let bob = SigningKey::mock(23); + let eve = SigningKey::mock(31); + + let project = Project::new( + ProjectName::from_str("unruled").unwrap(), + String::new(), + BranchName::from(git::fmt::RefString::try_from("master").unwrap()), + ) + .unwrap(); + let doc = RawDoc::new( + project, + vec![alice.did(), bob.did(), eve.did()], + 2, + identity::Visibility::Public, + ) + .verified() + .unwrap(); + + let branch = git::fmt::qualified!("refs/heads/master"); + let mut repo = MockRepository::new(r#gen::(1), doc.clone()); + for signer in [&alice, &bob, &eve] { + let mut refs = crate::storage::refs::Refs::new(); + refs.insert(branch.to_ref_string(), commit); + repo.remotes.insert( + *signer.public_key(), + crate::storage::refs::arbitrary::signed_refs(refs, commit, signer), + ); + } + + let revision = RevisionId(arbitrary::entry_id()); + let mut patch = Patch::new( + cob::Title::new("My Patch").unwrap(), + MergeTarget::Delegates, + ( + revision, + Revision::new( + revision, + Author::new(alice.did()), + String::new(), + base, + commit, + env::local_time().into(), + Default::default(), + ), + ), + ); + + apply_merge(&mut patch, &doc, &repo, revision, commit, &alice); + assert!( + !patch.is_merged(), + "one of three delegates is below the document threshold" + ); + + apply_merge(&mut patch, &doc, &repo, revision, commit, &bob); + assert_eq!( + *patch.state(), + State::Merged { revision, commit }, + "the synthesized rule resolves to the document threshold of two" + ); + } + #[test] fn json_serialization() { let edit = Action::Label { diff --git a/crates/radicle/src/storage/refs/arbitrary.rs b/crates/radicle/src/storage/refs/arbitrary.rs index 67f07d0de..e137a09b6 100644 --- a/crates/radicle/src/storage/refs/arbitrary.rs +++ b/crates/radicle/src/storage/refs/arbitrary.rs @@ -27,6 +27,19 @@ pub fn signed_refs_at(g: &mut qcheck::Gen, root: Oid, signer: &impl crypto::Sign } } +/// Sign an exact set of `refs`, for tests that need specific reference names. +pub fn signed_refs(refs: Refs, at: Oid, signer: &impl crypto::Signer) -> SignedRefs { + let signature = crypto::signature::Signer::sign(signer, &refs.canonical()); + SignedRefs { + refs, + signature, + id: *signer.public_key(), + level: FeatureLevel::default(), + parent: None, + at, + } +} + impl Arbitrary for Refs { fn arbitrary(g: &mut qcheck::Gen) -> Self { let names = &[