remote-helper: relax the quorum checks

The rules for checking the quorum of the remote helper, when pushing to the
canonical branch, can prevent the operator from even attempting to try converge
to a state of agreeance.

Instead, the checking of the quorum rules are relaxed. It is still the case that
if the peer is pushing a diverging commit while in a working state they are
warned and the push does not go through. However, there are two cases where it
is fine to force push a change:

1. When pushing a new tip, as a delegate, there is still a canonical tip and the
   new tip still converges with the history, i.e. it is equal to, behind, or
   ahead of another delegate tip.
2. If there is no quorum in the current state, for example there were multiple
   concurrent pushes, then the peer is free to continue to push to their default
   branch in attempt to converge with the other peers.

The example tests ensure that these cases are covered, while also preserving the
old behaviour.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
X-Clacks-Overhead: GNU Terry Pratchett
This commit is contained in:
Fintan Halpenny 2024-06-10 10:32:10 +02:00
parent 54d23545ff
commit 84aaf9efd0
No known key found for this signature in database
GPG Key ID: C93C17467280C75B
7 changed files with 305 additions and 84 deletions

View File

@ -0,0 +1,27 @@
``` ~alice
$ rad id update --title "Add Bob" --delegate did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --repo rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji -q
c036c0d89ce26aef3ad7da402157dba16b5163b4
```
``` ~bob
$ rad sync --fetch
✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6MknSL…StBU8Vi..
✓ Fetched repository from 1 seed(s)
```
``` ~alice
$ git commit -m "New changes" --allow-empty -q
$ git push rad master -o no-sync
```
``` ~alice
$ git commit --amend -m "Neue Änderungen" --allow-empty -q
```
``` ~alice (stderr)
$ git push rad master -f
✓ Canonical head updated to 9170c8795d3a78f0381a0ffafb20ea69fb0f5b6b
✓ Synced with 1 node(s)
To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi
+ fb25886...9170c87 master -> master (forced update)
```

View File

@ -2,11 +2,11 @@ In this scenario we check that we can easily reset our canonical head to the
head of another delegate when there is divergence between the 3 delegates.
First we add our new delegates, Bob & Eve, to our repo, while also setting the
`threshold` to `2`:
`threshold` to `3`:
``` ~alice
$ rad id update --title "Add Bob & Eve" --description "" --delegate did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --delegate did:key:z6Mkux1aUQD2voWWukVb5nNUR7thrHveQG4pDQua8nVhib7Z --threshold 2 --repo rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji -q
73bd6ea3f88eb0687afd13ee13bfb31a9eb0ccd2
$ rad id update --title "Add Bob & Eve" --delegate did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --delegate did:key:z6Mkux1aUQD2voWWukVb5nNUR7thrHveQG4pDQua8nVhib7Z --threshold 3 --repo rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji -q
3143236b2e40338f5574ec04e935a5ab80a6868a
```
Bob and Eve will fetch the changes to ensure they hear about their delegate
@ -37,14 +37,16 @@ f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 refs/heads/master
```
``` ~bob
$ git commit -m "Bob's commit" --allow-empty -q
$ git add README
$ git commit -m "Bob's commit" -q
$ git push rad -o no-sync
$ git ls-remote rad
f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 refs/heads/master
```
``` ~eve
$ git commit -m "Eve's commit" --allow-empty -q
$ git add README
$ git commit -m "Eve's commit" -q
$ git push rad -o no-sync
$ git ls-remote rad
f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 refs/heads/master
@ -74,8 +76,12 @@ remedied by the delegates agreeing upon which way to move forward. In this case,
Alice resets her `master` to `bob/master`:
``` ~alice
$ git reset bob/master --hard
HEAD is now at 0801f02 Bob's commit
$ git merge bob/master
Merge made by the 'ort' strategy.
README | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
$ git merge eve/master -s ours
Merge made by the 'ours' strategy.
```
She can then force push to update the canonical head to the new agreed upon
@ -83,29 +89,54 @@ commit:
``` ~alice (stderr)
$ git push rad -f
✓ Canonical head updated to 0801f020f11a08ec7a96d1710ec24e5e005499d1
✓ Synced with 2 node(s)
warn: no quorum was found for `refs/heads/master`
warn: it is recommended to find a commit to agree upon
✓ Synced with 1 node(s)
To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi
+ d09e634...0801f02 master -> master (forced update)
```
We can convince ourselves that the canonical branch has indeed changed by using
`git ls-remote` for each of the delegates:
``` ~alice
$ git ls-remote rad
0801f020f11a08ec7a96d1710ec24e5e005499d1 refs/heads/master
d09e634..0f9bd80 master -> master
```
``` ~bob
$ git ls-remote rad
0801f020f11a08ec7a96d1710ec24e5e005499d1 refs/heads/master
$ rad remote add did:key:z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --name alice
✓ Follow policy updated for z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi (alice)
✗ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6MknSL…StBU8Vi.. error: no quorum was found
✗ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6Mkux1…nVhib7Z.. error: no quorum was found
✓ Remote alice added
✓ Remote-tracking branch alice/master created for z6MknSL…StBU8Vi
$ git reset --hard alice/master
HEAD is now at 0f9bd80 Merge remote-tracking branch 'eve/master'
```
When we check Bob's `rad` remote, we see that the commit for `refs/heads/master`
has changed. This is actually Eve's commit as part of Alice merging above. Now
that Alice, Bob, and Eve all have this commit as part of their history it has
become the canonical `master`.
``` ~bob (stderr)
$ git push rad
✓ Canonical head updated to 3a75f66dd0020c9a0355cc6ec21f15de989e2001
✓ Synced with 2 node(s)
To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk
2a37862..0f9bd80 master -> master
```
Once Eve also resets to the merge commits, the canonical `master` is set to this tip.
``` ~eve
$ git ls-remote rad
0801f020f11a08ec7a96d1710ec24e5e005499d1 refs/heads/master
$ rad remote add did:key:z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --name alice
✓ Follow policy updated for z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi (alice)
✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6Mkt67…v4N1tRk..
✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6MknSL…StBU8Vi..
✓ Remote alice added
✓ Remote-tracking branch alice/master created for z6MknSL…StBU8Vi
$ git reset --hard alice/master
HEAD is now at 0f9bd80 Merge remote-tracking branch 'eve/master'
```
``` ~eve (stderr)
$ git push rad
✓ Canonical head updated to 0f9bd8035c04b3f73f5408e73e8454879b20800b
✓ Synced with 2 node(s)
To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6Mkux1aUQD2voWWukVb5nNUR7thrHveQG4pDQua8nVhib7Z
3a75f66..0f9bd80 master -> master
```

View File

@ -66,19 +66,14 @@ To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkE
f2de534..f6cff86 master -> master
```
One thing of note is that we can't push an older commit either, by default:
One thing of note is that we can revert to an older commit as long as we are
still ahead of the other delegates.
``` ~alice
$ git reset --hard HEAD^ -q
```
``` ~alice (fail)
$ git push -f
```
We have to use the `allow.rollback` option:
``` ~alice RAD_SOCKET=/dev/null (stderr)
$ git push -f -o allow.rollback
$ git push -f
✓ Canonical head updated to 319a7dc3b195368ded4b099f8c90bbb80addccd3
To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi
+ f6cff86...319a7dc master -> master (forced update)

View File

@ -0,0 +1,60 @@
In this scenario, we will explore being able to rollback to a previous commit.
First we add a second delegate, Bob, to our repo. We also change the threshold
to 2:
``` ~alice
$ rad id update --title "Add Bob" --delegate did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --repo rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji --threshold 2 -q
069e7d58faa9a7473d27f5510d676af33282796f
```
Bob then syncs these changes and adds a new commit:
``` ~bob
$ rad sync --fetch
✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6MknSL…StBU8Vi..
✓ Fetched repository from 1 seed(s)
$ git commit -m "Third commit" --allow-empty -q
$ git push rad
$ git branch -arv
alice@z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi/master f2de534 Second commit
rad/master 319a7dc Third commit
```
Alice merges these changes and pushes them, which updates the canonical head:
``` ~alice
$ rad remote add did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --name bob --fetch --no-sync
✓ Remote bob added
✓ Remote-tracking branch bob/master created for z6Mkt67…v4N1tRk
$ git merge bob/master
Updating f2de534..319a7dc
Fast-forward
```
``` ~alice (stderr)
$ git push rad
✓ Canonical head updated to 319a7dc3b195368ded4b099f8c90bbb80addccd3
✓ Synced with 1 node(s)
To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi
f2de534..319a7dc master -> master
```
Alice decides that she changes her mind about these changes and rolls back to
the previous commit:
``` ~alice
$ git reset --hard f2de534
HEAD is now at f2de534 Second commit
```
Since the canonical head is still decidable from this commit she is allowed to
push and the new canonical head becomes the previous commit again:
``` ~alice (stderr)
$ git push rad -f
✓ Canonical head updated to f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354
✓ Synced with 1 node(s)
To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi
+ 319a7dc...f2de534 master -> master (forced update)
```

View File

@ -2443,6 +2443,8 @@ fn git_push_diverge() {
#[test]
fn git_push_converge() {
use std::fs;
let mut environment = Environment::new();
let alice = environment.node(Config::test(Alias::new("alice")));
let bob = environment.node(Config::test(Alias::new("bob")));
@ -2471,6 +2473,17 @@ fn git_push_converge() {
alice.has_remote_of(&acme, &bob.id);
alice.has_remote_of(&acme, &eve.id);
fs::write(
working.join("bob").join("heartwood").join("README"),
"Hello\n",
)
.unwrap();
fs::write(
working.join("eve").join("heartwood").join("README"),
"Hello, world!\n",
)
.unwrap();
formula(&environment.tmp(), "examples/git/git-push-converge.md")
.unwrap()
.home(
@ -2492,6 +2505,88 @@ fn git_push_converge() {
.unwrap();
}
#[test]
fn git_push_amend() {
let mut environment = Environment::new();
let alice = environment.node(Config::test(Alias::new("alice")));
let bob = environment.node(Config::test(Alias::new("bob")));
let working = environment.tmp().join("working");
let acme = RepoId::from_str("z42hL2jL4XNk6K8oHQaSWfMgCL7ji").unwrap();
fixtures::repository(working.join("alice"));
test(
"examples/rad-init.md",
working.join("alice"),
Some(&alice.home),
[],
)
.unwrap();
let alice = alice.spawn();
let mut bob = bob.spawn();
bob.connect(&alice).converge([&alice]);
bob.fork(acme, working.join("bob")).unwrap();
alice.has_remote_of(&acme, &bob.id);
formula(&environment.tmp(), "examples/git/git-push-amend.md")
.unwrap()
.home(
"alice",
working.join("alice"),
[("RAD_HOME", alice.home.path().display())],
)
.home(
"bob",
working.join("bob").join("heartwood"),
[("RAD_HOME", bob.home.path().display())],
)
.run()
.unwrap();
}
#[test]
fn git_push_rollback() {
let mut environment = Environment::new();
let alice = environment.node(Config::test(Alias::new("alice")));
let bob = environment.node(Config::test(Alias::new("bob")));
let working = environment.tmp().join("working");
let acme = RepoId::from_str("z42hL2jL4XNk6K8oHQaSWfMgCL7ji").unwrap();
fixtures::repository(working.join("alice"));
test(
"examples/rad-init.md",
working.join("alice"),
Some(&alice.home),
[],
)
.unwrap();
let alice = alice.spawn();
let mut bob = bob.spawn();
bob.connect(&alice).converge([&alice]);
bob.fork(acme, working.join("bob")).unwrap();
alice.has_remote_of(&acme, &bob.id);
formula(&environment.tmp(), "examples/git/git-push-rollback.md")
.unwrap()
.home(
"alice",
working.join("alice"),
[("RAD_HOME", alice.home.path().display())],
)
.home(
"bob",
working.join("bob").join("heartwood"),
[("RAD_HOME", bob.home.path().display())],
)
.run()
.unwrap();
}
#[test]
fn rad_push_and_pull_patches() {
let mut environment = Environment::new();

View File

@ -68,11 +68,6 @@ pub enum Error {
List(#[from] list::Error),
}
#[derive(Debug, Default, Clone)]
pub struct Allow {
rollback: bool,
}
#[derive(Debug, Default, Clone)]
pub struct Options {
/// Don't sync after push.
@ -87,8 +82,6 @@ pub struct Options {
base: Option<Rev>,
/// Patch message.
message: cli::patch::Message,
/// Operations allowed.
allow: Allow,
}
/// Run the radicle remote helper using the given profile.
@ -215,7 +208,6 @@ fn push_option(args: &[&str], opts: &mut Options) -> Result<(), Error> {
["sync.debug"] => opts.sync_debug = true,
["no-sync"] => opts.no_sync = true,
["patch.draft"] => opts.draft = true,
["allow.rollback"] => opts.allow.rollback = true,
_ => {
let args = args.join(" ");
@ -259,3 +251,8 @@ pub(crate) fn read_line<'a>(stdin: &io::Stdin, line: &'a mut String) -> io::Resu
pub(crate) fn hint(s: impl fmt::Display) {
eprintln!("{}", cli::format::hint(format!("hint: {s}")));
}
/// Write a warning to the user.
pub(crate) fn warn(s: impl fmt::Display) {
eprintln!("{}", cli::format::hint(format!("warn: {s}")));
}

View File

@ -13,6 +13,8 @@ use radicle::cob::patch;
use radicle::cob::patch::cache::Patches as _;
use radicle::crypto::Signer;
use radicle::explorer::ExplorerResource;
use radicle::git::canonical;
use radicle::git::canonical::Canonical;
use radicle::identity::Did;
use radicle::node;
use radicle::node::{Handle, NodeId};
@ -24,7 +26,7 @@ use radicle::{git, rad};
use radicle_cli as cli;
use radicle_cli::terminal as term;
use crate::{hint, read_line, Options};
use crate::{hint, read_line, warn, Options};
#[derive(Debug, Error)]
pub enum Error {
@ -191,7 +193,6 @@ pub fn run(
_ => return Err(Error::InvalidCommand(line.trim().to_owned())),
}
}
let canonical = stored.head()?;
let delegates = stored.delegates()?;
// For each refspec, push a ref or delete a ref.
@ -203,7 +204,7 @@ pub fn run(
Command::Delete(dst) => {
// Delete refs.
let refname = nid.to_namespace().join(dst);
let (canonical_ref, _) = &canonical;
let (canonical_ref, _) = &stored.head()?;
if *dst == canonical_ref.to_ref_string() && delegates.contains(&Did::from(nid)) {
return Err(Error::DeleteForbidden(dst.clone()));
@ -249,7 +250,10 @@ pub fn run(
opts.clone(),
)
} else {
let (canonical_ref, canonical_oid) = &canonical;
let identity = stored.identity()?;
let project = identity.project()?;
let canonical_ref = git::refs::branch(project.default_branch());
let me = Did::from(nid);
// If we're trying to update the canonical head, make sure
// we don't diverge from the current head. This only applies
@ -257,41 +261,51 @@ pub fn run(
//
// Note that we *do* allow rolling back to a previous commit on the
// canonical branch.
if dst == *canonical_ref
&& delegates.contains(&Did::from(nid))
&& delegates.len() > 1
{
if let Err(e) = working.find_commit(**canonical_oid) {
return if git::ext::is_not_found_err(&e) {
Err(Error::MissingCanonicalHead(*canonical_oid))
} else {
Err(e.into())
};
}
if dst == canonical_ref && delegates.contains(&me) && delegates.len() > 1 {
let head = working.find_reference(src.as_str())?;
let head = head.peel_to_commit()?.id();
// Rollback is allowed and head is an ancestor of the canonical head.
let rollback = opts.allow.rollback
&& working.graph_descendant_of(**canonical_oid, head)?;
if head != **canonical_oid
// Canonical head is *not* an ancestor of head.
&& !working.graph_descendant_of(head, **canonical_oid)?
// Not a rollback.
&& !rollback
{
if hints {
hint(
"you are attempting to push a commit that would cause \
your upstream to diverge from the canonical head",
);
hint(
"to integrate the remote changes, run `git pull --rebase` \
and try again",
);
}
return Err(Error::HeadsDiverge(head.into(), *canonical_oid));
let mut canonical =
Canonical::default_branch(stored, &project, &identity.delegates)?;
let converges = canonical::converges(
canonical
.tips()
.filter_map(|(did, tip)| (*did != me).then_some(tip)),
head.into(),
&working,
)?;
if converges {
canonical.modify_vote(me, head.into());
}
match canonical.quorum(identity.threshold, &working) {
Ok(canonical_oid) => {
// Canonical head is an ancestor of head.
let is_ff = head == *canonical_oid
|| working.graph_descendant_of(head, *canonical_oid)?;
if !is_ff && !converges {
if hints {
hint(
"you are attempting to push a commit that would cause \
your upstream to diverge from the canonical head",
);
hint(
"to integrate the remote changes, run `git pull --rebase` \
and try again",
);
}
return Err(Error::HeadsDiverge(
head.into(),
canonical_oid,
));
}
}
Err(canonical::QuorumError::NoQuorum) => {
warn(format!("no quorum was found for `{canonical_ref}`"));
warn("it is recommended to find a commit to agree upon");
}
Err(e) => return Err(e.into()),
};
}
push(
src,
@ -322,15 +336,17 @@ pub fn run(
// Sign refs and sync if at least one ref pushed successfully.
if !ok.is_empty() {
let _ = stored.sign_refs(&signer)?;
let head = stored.set_head()?;
if head.is_updated() {
eprintln!(
"{} Canonical head updated to {}",
term::format::positive(""),
term::format::secondary(head.new),
);
}
// N.b. if an error occurs then there may be no quorum
if let Ok(head) = stored.set_head() {
if head.is_updated() {
eprintln!(
"{} Canonical head updated to {}",
term::format::positive(""),
term::format::secondary(head.new),
);
}
};
if !opts.no_sync {
if profile.policies()?.is_seeding(&stored.id)? {