From e10cc762d61a9d99b2aabdd2284a6860ef35c81d Mon Sep 17 00:00:00 2001 From: CleverWild Date: Mon, 7 Sep 2026 15:51:43 +0200 Subject: [PATCH] fix(proposal): count recovery operators only in electorates they can vote in --- .../src/actors/proposal_manager.rs | 19 ++- .../src/actors/proposal_manager/tests.rs | 151 ++++++++++++++++-- .../arbiter-server/src/db/proposal/mod.rs | 6 + 3 files changed, 155 insertions(+), 21 deletions(-) diff --git a/server/crates/arbiter-server/src/actors/proposal_manager.rs b/server/crates/arbiter-server/src/actors/proposal_manager.rs index c153d0b..8f8156e 100644 --- a/server/crates/arbiter-server/src/actors/proposal_manager.rs +++ b/server/crates/arbiter-server/src/actors/proposal_manager.rs @@ -159,10 +159,7 @@ impl ProposalManager { .await?; let mut tally = self.store.tally(proposal_id).await?; - // §3.5: recovery operators only join the electorate once they are awake. - if !self.store.is_recovery_active().await? { - tally.total_recovery = 0; - } + self.narrow_electorate(&proposal, &mut tally).await?; self.settle(&proposal, &tally).await } @@ -240,7 +237,9 @@ impl ProposalManager { }) .await?; - let tally = self.store.tally(proposal_id).await?; + let mut tally = self.store.tally(proposal_id).await?; + self.narrow_electorate(&proposal, &mut tally).await?; + self.settle(&proposal, &tally).await } } @@ -257,6 +256,16 @@ impl ProposalManager { Ok(()) } + /// §3.5/§3.6: recovery operators join the electorate only for the kinds they may vote on, + /// and only once the wake-up window has elapsed. Counting them anywhere else makes the + /// rejection threshold unreachable and, for full-quorum kinds, approval unreachable too. + async fn narrow_electorate(&self, proposal: &Proposal, tally: &mut Tally) -> Result<(), Error> { + if !proposal.kind.recovery_may_vote() || !self.store.is_recovery_active().await? { + tally.total_recovery = 0; + } + Ok(()) + } + /// Pure quorum arithmetic — no I/O, so the rules can be tested directly (§3.3). /// /// A proposal is rejected once approval has become unreachable: even if every voter diff --git a/server/crates/arbiter-server/src/actors/proposal_manager/tests.rs b/server/crates/arbiter-server/src/actors/proposal_manager/tests.rs index 7a8a607..1b50763 100644 --- a/server/crates/arbiter-server/src/actors/proposal_manager/tests.rs +++ b/server/crates/arbiter-server/src/actors/proposal_manager/tests.rs @@ -180,11 +180,24 @@ async fn a_vote_short_of_quorum_does_not_touch_the_status() { assert_eq!(outcome, VoteOutcome::Pending); } -/// A sleeping recovery electorate must not raise the bar for an ordinary proposal. -#[tokio::test] -async fn sleeping_recovery_operators_do_not_count_towards_quorum() { - let id = ProposalId::from_raw(9); - let voter = OperatorIdentityId::from_raw(3); +/// Drives one `cast_vote` on a proposal of the given `kind` through a mocked store and +/// returns the outcome. `recovery_active` decides what `is_recovery_active` reports; +/// `expected_status` is the status a settled outcome must be persisted under. +/// +/// `set_status` carries an argument matcher but no `.times()`: whichever outcome a caller +/// asserts is either `Approved` or `Rejected` (never `Pending`), so the write must happen +/// with the right status if it happens at all, but leaving the count unconstrained means a +/// regression that turns the outcome into `Pending` still fails on the caller's own +/// `assert_eq!` -- a readable diff -- rather than on a mockall cardinality panic that hides +/// what the actor actually computed. +async fn settle_vote_with( + kind: ProposalKindTag, + tally: Tally, + recovery_active: bool, + expected_status: ProposalStatus, +) -> VoteOutcome { + let id = ProposalId::from_raw(11); + let voter = OperatorIdentityId::from_raw(1); let key = SigningKey::generate(); let signature = key .sign_message(&vote_message(id, true), SigningContext::GovernanceVote) @@ -194,29 +207,135 @@ async fn sleeping_recovery_operators_do_not_count_towards_quorum() { let mut store = MockProposalStore::new(); store .expect_load() - .returning(move |id| Ok(pending_proposal(id, ProposalKindTag::ApproveSdkClient))); + .returning(move |id| Ok(pending_proposal(id, kind))); store.expect_has_voted().returning(|_, _| Ok(false)); store .expect_operator_public_key() .returning(move |_| Ok(public_key.clone())); store.expect_record_vote().returning(|_| Ok(())); - store.expect_is_recovery_active().returning(|| Ok(false)); - // Two recovery operators exist but are asleep, so the threshold stays at 1 of 1. - store.expect_tally().returning(|_| Ok(tally(1, 0, 1, 2))); - store.expect_set_status().times(1).returning(|_, _| Ok(())); - store.expect_load_kind().returning(|_, _| { - Ok(crate::db::proposal::ProposalKind::ApproveSdkClient( - crate::db::proposal::approve_sdk_client::Settings { client_id: 1 }, - )) + store + .expect_is_recovery_active() + .returning(move || Ok(recovery_active)); + store.expect_tally().returning(move |_| Ok(tally)); + store + .expect_set_status() + .withf(move |_, status| *status == expected_status) + .returning(|_, _| Ok(())); + store.expect_load_kind().returning(move |_, _| { + Ok(match kind { + ProposalKindTag::TriggerRekey => crate::db::proposal::ProposalKind::TriggerRekey, + ProposalKindTag::ApproveSdkClient => { + crate::db::proposal::ProposalKind::ApproveSdkClient( + crate::db::proposal::approve_sdk_client::Settings { client_id: 1 }, + ) + } + ProposalKindTag::ReplaceOperator => crate::db::proposal::ProposalKind::ReplaceOperator( + crate::db::proposal::replace_operator::Settings { + old_operator_id: OperatorIdentityId::from_raw(1), + new_pubkey: vec![0u8; 32], + }, + ), + other => unreachable!("settle_vote_with has no load_kind fixture for {other:?}"), + }) }); let mut manager = ProposalManager::with_store(Arc::new(store), GlobalActors::spawn_message_bus()); - let outcome = manager + manager .cast_vote(id, voter, true, signature.to_bytes()) .await - .expect("a valid vote must be accepted"); + .expect("a valid vote must be accepted") +} + +/// The full-quorum rejection path is insensitive to electorate size by construction: +/// `threshold == total_eligible` there, so `total_eligible - threshold` is always 0 and any +/// single rejection settles the proposal, whether or not recovery operators are (wrongly) +/// counted. This does not exercise the electorate-narrowing fix -- see +/// `unanimous_ordinary_rejection_rejects_a_non_full_quorum_proposal_while_recovery_is_awake` +/// below for the test that does -- it just pins that `cast_vote` still writes `Rejected` +/// through `settle` for a full-quorum kind. +#[tokio::test] +async fn unanimous_rejection_settles_a_full_quorum_rekey_via_cast_vote() { + let outcome = settle_vote_with( + ProposalKindTag::TriggerRekey, + Tally { + approve: 0, + reject: 3, + total_ordinary: 3, + total_recovery: 2, + }, + /* recovery_active */ true, + ProposalStatus::Rejected, + ) + .await; + + assert_eq!(outcome, VoteOutcome::Rejected); +} + +/// §3.3 full quorum for a rekey means every *ordinary* operator, not every identity on file. +#[tokio::test] +async fn unanimous_ordinary_approval_approves_a_rekey_while_recovery_is_awake() { + let outcome = settle_vote_with( + ProposalKindTag::TriggerRekey, + Tally { + approve: 3, + reject: 0, + total_ordinary: 3, + total_recovery: 2, + }, + /* recovery_active */ true, + ProposalStatus::Approved, + ) + .await; + + assert_eq!(outcome, VoteOutcome::Approved); +} + +/// §3.5: recovery operators do not vote on `ApproveSdkClient`, so they must not inflate its +/// electorate. Before the fix, `total_eligible` counted them anyway (5, not 3), so the +/// rejection test `reject > total_eligible - threshold` became `3 > 5 - 2 = 3`, which is +/// false -- three unanimous rejections left the proposal `Pending` forever, since a fourth +/// vote could never arrive. After the fix, `total_eligible` is 3 and the same test becomes +/// `3 > 3 - 2 = 1`, which settles it. +#[tokio::test] +async fn unanimous_ordinary_rejection_rejects_a_non_full_quorum_proposal_while_recovery_is_awake() { + let outcome = settle_vote_with( + ProposalKindTag::ApproveSdkClient, + Tally { + approve: 0, + reject: 3, + total_ordinary: 3, + total_recovery: 2, + }, + /* recovery_active */ true, + ProposalStatus::Rejected, + ) + .await; + + assert_eq!(outcome, VoteOutcome::Rejected); +} + +/// §3.5/§3.6: `ReplaceOperator` is the one kind recovery may vote on, so it is the only kind +/// where whether recovery is awake is observable at all -- for every other kind +/// `narrow_electorate` zeroes `total_recovery` regardless of `is_recovery_active`, short- +/// circuiting before that call. A sleeping recovery electorate must not raise the bar here: +/// with 1 ordinary operator and 2 (asleep) recovery operators, the lone ordinary approval +/// must already reach full quorum. +#[tokio::test] +async fn sleeping_recovery_operators_do_not_count_towards_quorum() { + let outcome = settle_vote_with( + ProposalKindTag::ReplaceOperator, + Tally { + approve: 1, + reject: 0, + total_ordinary: 1, + total_recovery: 2, + }, + /* recovery_active */ false, + ProposalStatus::Approved, + ) + .await; assert_eq!(outcome, VoteOutcome::Approved); } diff --git a/server/crates/arbiter-server/src/db/proposal/mod.rs b/server/crates/arbiter-server/src/db/proposal/mod.rs index a8e756a..b03dbbd 100644 --- a/server/crates/arbiter-server/src/db/proposal/mod.rs +++ b/server/crates/arbiter-server/src/db/proposal/mod.rs @@ -82,6 +82,12 @@ impl ProposalKindTag { pub const fn requires_full_quorum(self) -> bool { matches!(self, Self::ReplaceOperator | Self::TriggerRekey) } + + /// §3.5: recovery operators weigh in on operator replacement and nothing else. + #[must_use] + pub const fn recovery_may_vote(self) -> bool { + matches!(self, Self::ReplaceOperator) + } } /// Pins every implementation to the variant it is dispatched from. Without this a