radicle/patch: Use cref rule quorum for merges

To determine whether a patch has been merged to the default branch, we
tally merges based on the default branch quorum: allow set and
threshold. With the introduction of custom branches, the tallying was
not updated to use the quorum rules for the target branch, instead
merging on the identity document threshold and delegate set.

This fix uses the `allow` set and the `threshold` of the most specific
rule that matches the target branch of patch.

Note that a reference that no rule matches never becomes canonical, so a
patch that targets one can never reach a quorum.
This commit is contained in:
Daniel Norman
2026-10-13 14:39:23 +02:00
committed by Lorenz Leutgeb
parent ada90d12ae
commit 3032ba1298
3 changed files with 335 additions and 13 deletions
+10
View File
@@ -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/), 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). 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 ## 1.10.3 - 2026-09-07
### Fixes ### Fixes
+312 -13
View File
@@ -627,10 +627,13 @@ impl Patch {
/// ///
/// If a custom target was specified, it returns that branch. /// If a custom target was specified, it returns that branch.
/// Otherwise, it falls back to the project's default 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, &'a self,
doc: &'a Doc, doc: &'b Doc,
) -> Result<git::fmt::Qualified<'a>, DefaultBranchError> { ) -> Result<git::fmt::Qualified<'b>, DefaultBranchError>
where
'b: 'a,
{
match &self.target { match &self.target {
MergeTarget::Delegates => Ok(doc.default_branch()?), MergeTarget::Delegates => Ok(doc.default_branch()?),
MergeTarget::Branch(branch) => Ok(branch.clone().into_qualified()), MergeTarget::Branch(branch) => Ok(branch.clone().into_qualified()),
@@ -1274,17 +1277,45 @@ impl Patch {
}, },
); );
let mut merges = self.merges.iter().fold( // The quorum for the target branch is defined by the canonical
HashMap::<(RevisionId, git::Oid), usize>::new(), // reference rule that matches it: only merges made by an allowed
|mut acc, (_, merge)| { // DID count, and the rule's threshold must be reached.
*acc.entry((merge.revision, merge.commit)).or_default() += 1; //
acc // A branch that no rule matches never becomes a canonical
}, // reference, so a patch targeting it can never reach a quorum.
); //
// Discard revisions that weren't merged by a threshold of delegates. // As above, a merge we cannot evaluate is skipped rather
merges.retain(|_, count| *count >= identity.threshold()); // 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::<Vec<_>>().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::<Vec<_>>()
.as_slice()
{
[] => { [] => {
// None of the revisions met the quorum. // 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::<Project>(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::<RepoId>(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::<RepoId>(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::<RepoId>(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] #[test]
fn json_serialization() { fn json_serialization() {
let edit = Action::Label { let edit = Action::Label {
@@ -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 { impl Arbitrary for Refs {
fn arbitrary(g: &mut qcheck::Gen) -> Self { fn arbitrary(g: &mut qcheck::Gen) -> Self {
let names = &[ let names = &[