fix(evm): revoke dependent grants when wallet access is revoked

server/crates/arbiter-server/src/actors/evm/mod.rs

@@ -71,15 +71,51 @@ pub enum Error {
     #[error("Signing error: {0}")]
     Sign(#[from] SignTransactionError),
 
-    #[error("Grant timestamp {0} is outside the representable range")]
+    #[error(
+        "Grant timestamp {0} is outside the i32 range a grant boundary column can store \
+         (Unix seconds, so no later than 2038-01-19T03:14:07Z)"
+    )]
     InvalidTimestamp(i64),
+
+    #[error("Wallet access {0} is revoked or does not exist")]
+    AccessNotActive(i32),
 }
 
-/// Converts a grant boundary from Unix seconds. `None` in means "unbounded"; an
+/// Converts a grant boundary from Unix seconds. `None` in means "unbounded"; a value the
-/// unrepresentable value is an error, never a silently unbounded grant.
+/// boundary column cannot store is an error, never a silently different window.
+///
+/// The range is `i32`, not `i64`, because that is what actually reaches the database:
+/// `SqliteTimestamp::to_sql` narrows to `i32` (`fixme! #84`), so `3_000_000_000` -- a
+/// `valid_from` in 2065 -- would wrap to 1902 and open the grant immediately instead of in
+/// forty years. Accepting only what round-trips keeps the grant that gets written the grant
+/// that was voted on.
 fn grant_timestamp(secs: Option<i64>) -> Result<Option<chrono::DateTime<chrono::Utc>>, Error> {
-    secs.map(|s| chrono::DateTime::from_timestamp(s, 0).ok_or(Error::InvalidTimestamp(s)))
+    secs.map(|s| {
-        .transpose()
+        let storable = i32::try_from(s).map_err(|_| Error::InvalidTimestamp(s))?;
+        chrono::DateTime::from_timestamp(i64::from(storable), 0).ok_or(Error::InvalidTimestamp(s))
+    })
+    .transpose()
+}
+
+/// Refuses an access id that is revoked or absent, so nothing hangs a grant off it.
+async fn ensure_access_active(
+    conn: &mut crate::db::DatabaseConnection,
+    access_id: i32,
+) -> Result<(), Error> {
+    let active: bool = diesel::select(diesel::dsl::exists(
+        schema::evm_wallet_access::table
+            .filter(schema::evm_wallet_access::id.eq(access_id))
+            .filter(schema::evm_wallet_access::revoked_at.is_null()),
+    ))
+    .get_result(conn)
+    .await
+    .map_err(DatabaseError::from)?;
+
+    if active {
+        Ok(())
+    } else {
+        Err(Error::AccessNotActive(access_id))
+    }
 }
 
 #[derive(Actor)]
@@ -326,7 +362,10 @@ impl EvmActor {
         let mut conn = self.db.get().await.map_err(DatabaseError::from)?;
 
         // Revives a previously revoked row instead of conflicting on it forever:
-        // `uniq_wallet_access` is a unique index on `(wallet_id, client_id)`.
+        // `uniq_wallet_access` is a unique index on `(wallet_id, client_id)`. Visibility is
+        // all this restores -- revocation closes the grants that hung off the access, so a
+        // persistent grant needs its own vote again (§3.2). See
+        // `peers::operator::session::handlers::revoke_wallet_access`.
         insert_into(schema::evm_wallet_access::table)
             .values((
                 schema::evm_wallet_access::wallet_id.eq(EvmWalletId::from_raw(settings.wallet_id)),
@@ -355,6 +394,14 @@ impl EvmActor {
         use alloy::primitives::U256;
         use chrono::Duration;
 
+        // A persistent grant is only as good as the visibility it hangs off (§3.2, two
+        // separate votes). The proposal names the access id when it is created and can be
+        // approved much later, so the access may have been revoked in between; a grant
+        // against a revoked access would sit dormant and go live the moment anyone re-grants.
+        let mut conn = self.db.get().await.map_err(DatabaseError::from)?;
+        ensure_access_active(&mut conn, grant.wallet_access_id).await?;
+        drop(conn);
+
         let volume = |limit: persistent_grant::VolumeLimit| VolumeRateLimit {
             max_volume: U256::from_be_bytes(limit.max_volume),
             window: Duration::seconds(limit.window_secs),
@@ -433,7 +480,11 @@ impl EvmActor {
 
 #[cfg(test)]
 mod tests {
-    use super::{Error, grant_timestamp};
+    use super::{Error, EvmActor, ensure_access_active, grant_timestamp};
+    use crate::db::{self, models, schema};
+
+    use diesel::{ExpressionMethods as _, QueryDsl as _, dsl::insert_into};
+    use diesel_async::RunQueryDsl;
 
     #[test]
     fn absent_timestamp_stays_absent() {
@@ -453,4 +504,191 @@ mod tests {
         let err = grant_timestamp(Some(i64::MAX)).unwrap_err();
         assert!(matches!(err, Error::InvalidTimestamp(i64::MAX)));
     }
+
+    /// A `valid_from` past 2038 is representable as a `DateTime` but not as the `i32` the
+    /// boundary column stores: `3_000_000_000` (2065) wraps to a negative, which reads back as
+    /// 1902 and makes the grant active immediately. Refusing it is the only way the grant
+    /// that lands can match the window that was voted on.
+    #[test]
+    fn a_timestamp_past_2038_is_an_error() {
+        let past_2038 = 3_000_000_000_i64;
+        assert!(
+            chrono::DateTime::from_timestamp(past_2038, 0).is_some(),
+            "the fixture must be a date chrono accepts, or it proves nothing about storage"
+        );
+
+        let err = grant_timestamp(Some(past_2038)).unwrap_err();
+        assert!(
+            matches!(err, Error::InvalidTimestamp(got) if got == past_2038),
+            "expected an out-of-range error, got {err:?}"
+        );
+    }
+
+    /// The last second the boundary column can hold must still be accepted: the range check
+    /// has to stop at what storage can take, not short of it.
+    #[test]
+    fn the_last_storable_timestamp_is_accepted() {
+        let converted = grant_timestamp(Some(i64::from(i32::MAX))).unwrap();
+        assert_eq!(converted.unwrap().timestamp(), i64::from(i32::MAX));
+    }
+
+    /// Seeds a wallet, a client and one access row between them, and returns the access id.
+    async fn seed_access(conn: &mut db::DatabaseConnection) -> i32 {
+        let root_key_id: models::RootKeyHistoryId = insert_into(schema::root_key_history::table)
+            .values(&models::NewRootKeyHistory {
+                ciphertext: vec![0u8; 32],
+                tag: vec![0u8; 16],
+                root_key_encryption_nonce: vec![0u8; 24],
+                data_encryption_nonce: vec![0u8; 24],
+                schema_version: 1,
+                salt: vec![0u8; 16],
+            })
+            .returning(schema::root_key_history::id)
+            .get_result(conn)
+            .await
+            .unwrap();
+
+        let aead_id: i32 = insert_into(schema::aead_encrypted::table)
+            .values(&models::NewAeadEncrypted {
+                ciphertext: vec![0u8; 32],
+                tag: vec![0u8; 16],
+                current_nonce: vec![0u8; 24],
+                schema_version: 1,
+                associated_root_key_id: root_key_id,
+                created_at: chrono::Utc::now().into(),
+            })
+            .returning(schema::aead_encrypted::id)
+            .get_result(conn)
+            .await
+            .unwrap();
+
+        let wallet_id: models::EvmWalletId = insert_into(schema::evm_wallet::table)
+            .values((
+                schema::evm_wallet::address.eq(rand::random::<[u8; 20]>().to_vec()),
+                schema::evm_wallet::aead_encrypted_id.eq(aead_id),
+            ))
+            .returning(schema::evm_wallet::id)
+            .get_result(conn)
+            .await
+            .unwrap();
+
+        let metadata_id: i32 = insert_into(schema::client_metadata::table)
+            .values(schema::client_metadata::name.eq("test"))
+            .returning(schema::client_metadata::id)
+            .get_result(conn)
+            .await
+            .unwrap();
+
+        let client_id: i32 = insert_into(schema::program_client::table)
+            .values((
+                schema::program_client::public_key.eq(rand::random::<[u8; 32]>().to_vec()),
+                schema::program_client::metadata_id.eq(metadata_id),
+            ))
+            .returning(schema::program_client::id)
+            .get_result(conn)
+            .await
+            .unwrap();
+
+        insert_into(schema::evm_wallet_access::table)
+            .values((
+                schema::evm_wallet_access::wallet_id.eq(wallet_id),
+                schema::evm_wallet_access::client_id.eq(client_id),
+            ))
+            .returning(schema::evm_wallet_access::id)
+            .get_result(conn)
+            .await
+            .unwrap()
+    }
+
+    /// Both directions, so a guard that refused everything could not pass: a live access is
+    /// let through, a revoked one is not.
+    #[tokio::test]
+    async fn only_a_live_access_passes_the_grant_guard() {
+        let pool = db::create_test_pool().await;
+        let mut conn = pool.get().await.unwrap();
+
+        let access_id = seed_access(&mut conn).await;
+        ensure_access_active(&mut conn, access_id)
+            .await
+            .expect("a live access must pass");
+
+        diesel::update(schema::evm_wallet_access::table)
+            .filter(schema::evm_wallet_access::id.eq(access_id))
+            .set(schema::evm_wallet_access::revoked_at.eq(models::SqliteTimestamp::now()))
+            .execute(&mut conn)
+            .await
+            .unwrap();
+
+        let err = ensure_access_active(&mut conn, access_id)
+            .await
+            .expect_err("a revoked access must be refused");
+        assert!(
+            matches!(err, Error::AccessNotActive(got) if got == access_id),
+            "expected AccessNotActive, got {err:?}"
+        );
+    }
+
+    /// The guard has to be wired into the executor, not just exist: an approved persistent
+    /// grant whose access was revoked between proposal and approval must not create a grant
+    /// that would go live again the moment anyone re-grants that access (§3.2).
+    #[tokio::test]
+    async fn an_approved_persistent_grant_refuses_a_revoked_access() {
+        use crate::actors::{GlobalActors, vault::Vault};
+        use crate::db::proposal::persistent_grant;
+        use kameo::actor::Spawn as _;
+
+        let pool = db::create_test_pool().await;
+        let mut conn = pool.get().await.unwrap();
+
+        let access_id = seed_access(&mut conn).await;
+        diesel::update(schema::evm_wallet_access::table)
+            .filter(schema::evm_wallet_access::id.eq(access_id))
+            .set(schema::evm_wallet_access::revoked_at.eq(models::SqliteTimestamp::now()))
+            .execute(&mut conn)
+            .await
+            .unwrap();
+        drop(conn);
+
+        let vault = Vault::spawn(
+            Vault::new(pool.clone(), GlobalActors::spawn_message_bus())
+                .await
+                .unwrap(),
+        );
+        let mut evm_actor = EvmActor::new(vault, pool.clone());
+
+        let err = evm_actor
+            .create_persistent_grant(persistent_grant::Settings {
+                wallet_access_id: access_id,
+                chain_id: 1,
+                valid_from_secs: None,
+                valid_until_secs: None,
+                max_gas_fee_per_gas: None,
+                max_priority_fee_per_gas: None,
+                rate_limit: None,
+                specific: persistent_grant::Specific::EtherTransfer {
+                    targets: vec![[0u8; 20]],
+                    limit: persistent_grant::VolumeLimit {
+                        max_volume: [0u8; 32],
+                        window_secs: 3600,
+                    },
+                },
+            })
+            .await
+            .expect_err("a grant against a revoked access must be refused");
+        assert!(
+            matches!(err, Error::AccessNotActive(got) if got == access_id),
+            "expected AccessNotActive, got {err:?}"
+        );
+
+        let grants: i64 = schema::evm_basic_grant::table
+            .filter(schema::evm_basic_grant::wallet_access_id.eq(access_id))
+            .count()
+            .get_result(&mut pool.get().await.unwrap())
+            .await
+            .unwrap();
+        assert_eq!(
+            grants, 0,
+            "no grant row may be written for a revoked access"
+        );
+    }
 }

server/crates/arbiter-server/src/actors/proposal_manager.rs

@@ -255,9 +255,15 @@ impl ProposalManager {
     /// §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.
+    ///
+    /// The votes go out with the voters. A wake-up can be cancelled after recovery operators
+    /// have already voted (`cancel_wakeup` cancels any uncancelled request, elapsed or not),
+    /// so a `ReplaceOperator` tally can hold recovery approvals at the moment the committee
+    /// stops being eligible. Keeping those while zeroing only the electorate size would let
+    /// them cover ordinary votes that were never cast.
     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;
+            tally.drop_recovery();
         }
         Ok(())
     }
@@ -270,6 +276,13 @@ impl ProposalManager {
     pub(crate) const fn evaluate_quorum(tally: &Tally, requires_full_quorum: bool) -> VoteOutcome {
         let total_eligible = tally.total_ordinary + tally.total_recovery;
 
+        // No electorate, nothing to settle. Guarded before the branch rather than inside it:
+        // the full-quorum arm would otherwise set `threshold` to 0 and read an empty tally as
+        // unanimous approval.
+        if total_eligible <= 0 {
+            return VoteOutcome::Pending;
+        }
+
         #[expect(
             clippy::cast_possible_truncation,
             clippy::cast_sign_loss,
@@ -289,9 +302,9 @@ impl ProposalManager {
             }
         };
 
-        if tally.approve >= threshold {
+        if tally.approve() >= threshold {
             VoteOutcome::Approved
-        } else if tally.reject > total_eligible - threshold {
+        } else if tally.reject() > total_eligible - threshold {
             VoteOutcome::Rejected
         } else {
             VoteOutcome::Pending

server/crates/arbiter-server/src/actors/proposal_manager/store.rs

@@ -26,14 +26,43 @@ use std::collections::HashMap;
 use strum::IntoDiscriminant as _;
 
 /// Everything the quorum rules need to know about one proposal's votes.
+///
+/// Votes are kept per electorate rather than pre-summed: an electorate can stop counting
+/// between the vote and the tally (§3.6 -- recovery goes back to sleep the moment a wake-up
+/// is cancelled), and the votes it already cast have to leave with it. A single `approve`
+/// field would carry them past [`Tally::drop_recovery`] into a threshold computed for the
+/// ordinary committee alone.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub struct Tally {
-    pub approve: i64,
+    pub ordinary_approve: i64,
-    pub reject: i64,
+    pub ordinary_reject: i64,
+    pub recovery_approve: i64,
+    pub recovery_reject: i64,
     pub total_ordinary: i64,
     pub total_recovery: i64,
 }
 
+impl Tally {
+    /// Approvals from every electorate that still counts.
+    pub const fn approve(&self) -> i64 {
+        self.ordinary_approve + self.recovery_approve
+    }
+
+    /// Rejections from every electorate that still counts.
+    pub const fn reject(&self) -> i64 {
+        self.ordinary_reject + self.recovery_reject
+    }
+
+    /// Takes the recovery committee out of the electorate, votes and all. The three numbers
+    /// go together: leaving the votes behind counts them against a threshold derived from an
+    /// electorate they are no longer part of.
+    pub const fn drop_recovery(&mut self) {
+        self.recovery_approve = 0;
+        self.recovery_reject = 0;
+        self.total_recovery = 0;
+    }
+}
+
 #[cfg_attr(test, mockall::automock)]
 #[async_trait]
 pub trait ProposalStore: Send + Sync + 'static {
@@ -273,8 +302,10 @@ impl ProposalStore for DieselProposalStore {
             .await?;
 
         Ok(Tally {
-            approve: ordinary_approve + recovery_approve,
+            ordinary_approve,
-            reject: ordinary_reject + recovery_reject,
+            ordinary_reject,
+            recovery_approve,
+            recovery_reject,
             total_ordinary,
             total_recovery,
         })

server/crates/arbiter-server/src/actors/proposal_manager/tests.rs

@@ -20,15 +20,37 @@ use arbiter_crypto::authn::{SigningContext, SigningKey};
 use chrono::{Duration, Utc};
 use std::sync::Arc;
 
+/// A tally where every vote came from the ordinary committee.
 const fn tally(approve: i64, reject: i64, ordinary: i64, recovery: i64) -> Tally {
     Tally {
-        approve,
+        ordinary_approve: approve,
-        reject,
+        ordinary_reject: reject,
+        recovery_approve: 0,
+        recovery_reject: 0,
         total_ordinary: ordinary,
         total_recovery: recovery,
     }
 }
 
+/// A tally with votes from both committees, in the order approve/reject per committee.
+const fn mixed_tally(
+    ordinary_approve: i64,
+    ordinary_reject: i64,
+    recovery_approve: i64,
+    recovery_reject: i64,
+    total_ordinary: i64,
+    total_recovery: i64,
+) -> Tally {
+    Tally {
+        ordinary_approve,
+        ordinary_reject,
+        recovery_approve,
+        recovery_reject,
+        total_ordinary,
+        total_recovery,
+    }
+}
+
 #[test]
 fn simple_majority_approves_at_two_of_three() {
     assert_eq!(
@@ -57,16 +79,32 @@ fn full_quorum_kind_needs_every_voter() {
 #[test]
 fn recovery_voters_count_towards_full_quorum() {
     assert_eq!(
-        ProposalManager::evaluate_quorum(&tally(3, 0, 2, 1), true),
+        ProposalManager::evaluate_quorum(&mixed_tally(2, 0, 1, 0, 2, 1), true),
         VoteOutcome::Approved
     );
     assert_eq!(
-        ProposalManager::evaluate_quorum(&tally(2, 0, 2, 1), true),
+        ProposalManager::evaluate_quorum(&mixed_tally(2, 0, 0, 0, 2, 1), true),
         VoteOutcome::Pending,
         "the sleeping recovery operator still owes a vote"
     );
 }
 
+/// An empty committee cannot approve anything. Both arms have to say so: the full-quorum arm
+/// derives its threshold from the electorate, so with nobody eligible it would compare 0
+/// approvals against a threshold of 0 and call that unanimous.
+#[test]
+fn an_empty_electorate_settles_nothing() {
+    assert_eq!(
+        ProposalManager::evaluate_quorum(&tally(0, 0, 0, 0), true),
+        VoteOutcome::Pending,
+        "a full-quorum proposal must not pass with no eligible voters"
+    );
+    assert_eq!(
+        ProposalManager::evaluate_quorum(&tally(0, 0, 0, 0), false),
+        VoteOutcome::Pending
+    );
+}
+
 #[test]
 fn rejection_is_decided_once_approval_is_unreachable() {
     // Threshold is 2 of 3, so two rejections leave at most one approval available.
@@ -182,19 +220,18 @@ async fn a_vote_short_of_quorum_does_not_touch_the_status() {
 
 /// 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.
+/// `expected_status` is the status a settled outcome must be persisted under, or `None` for
+/// a caller that expects the vote to leave the proposal pending.
 ///
-/// `set_status` carries an argument matcher but no `.times()`: whichever outcome a caller
+/// `set_status` carries an argument matcher but no `.times()`, and `None` relaxes even the
-/// asserts is either `Approved` or `Rejected` (never `Pending`), so the write must happen
+/// matcher: the caller's own `assert_eq!` on the outcome is what pins the behaviour, so a
-/// with the right status if it happens at all, but leaving the count unconstrained means a
+/// regression fails on a readable diff rather than on a mockall cardinality panic that hides
-/// 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,
+    expected_status: Option<ProposalStatus>,
 ) -> VoteOutcome {
     let id = ProposalId::from_raw(11);
     let voter = OperatorIdentityId::from_raw(1);
@@ -219,7 +256,11 @@ async fn settle_vote_with(
     store.expect_tally().returning(move |_| Ok(tally));
     store
         .expect_set_status()
-        .withf(move |_, status| *status == expected_status)
+        .withf(move |_, status| {
+            expected_status
+                .as_ref()
+                .is_none_or(|expected| status == expected)
+        })
         .returning(|_, _| Ok(()));
     store.expect_load_kind().returning(move |_, _| {
         Ok(match kind {
@@ -259,14 +300,9 @@ async fn settle_vote_with(
 async fn unanimous_rejection_settles_a_full_quorum_rekey_via_cast_vote() {
     let outcome = settle_vote_with(
         ProposalKindTag::TriggerRekey,
-        Tally {
+        tally(0, 3, 3, 2),
-            approve: 0,
-            reject: 3,
-            total_ordinary: 3,
-            total_recovery: 2,
-        },
         /* recovery_active */ true,
-        ProposalStatus::Rejected,
+        Some(ProposalStatus::Rejected),
     )
     .await;
 
@@ -278,14 +314,9 @@ async fn unanimous_rejection_settles_a_full_quorum_rekey_via_cast_vote() {
 async fn unanimous_ordinary_approval_approves_a_rekey_while_recovery_is_awake() {
     let outcome = settle_vote_with(
         ProposalKindTag::TriggerRekey,
-        Tally {
+        tally(3, 0, 3, 2),
-            approve: 3,
-            reject: 0,
-            total_ordinary: 3,
-            total_recovery: 2,
-        },
         /* recovery_active */ true,
-        ProposalStatus::Approved,
+        Some(ProposalStatus::Approved),
     )
     .await;
 
@@ -302,14 +333,9 @@ async fn unanimous_ordinary_approval_approves_a_rekey_while_recovery_is_awake()
 async fn unanimous_ordinary_rejection_rejects_a_non_full_quorum_proposal_while_recovery_is_awake() {
     let outcome = settle_vote_with(
         ProposalKindTag::ApproveSdkClient,
-        Tally {
+        tally(0, 3, 3, 2),
-            approve: 0,
-            reject: 3,
-            total_ordinary: 3,
-            total_recovery: 2,
-        },
         /* recovery_active */ true,
-        ProposalStatus::Rejected,
+        Some(ProposalStatus::Rejected),
     )
     .await;
 
@@ -326,16 +352,44 @@ async fn unanimous_ordinary_rejection_rejects_a_non_full_quorum_proposal_while_r
 async fn sleeping_recovery_operators_do_not_count_towards_quorum() {
     let outcome = settle_vote_with(
         ProposalKindTag::ReplaceOperator,
-        Tally {
+        tally(1, 0, 1, 2),
-            approve: 1,
-            reject: 0,
-            total_ordinary: 1,
-            total_recovery: 2,
-        },
         /* recovery_active */ false,
-        ProposalStatus::Approved,
+        Some(ProposalStatus::Approved),
     )
     .await;
 
     assert_eq!(outcome, VoteOutcome::Approved);
 }
+
+/// The sequence the whole-branch review worked through, on a `ReplaceOperator` with 3
+/// ordinary and 2 recovery operators (§3.3: full quorum). Both recovery operators approve
+/// while awake; one ordinary operator approves; another ordinary operator then cancels the
+/// wake-up -- `cancel_wakeup` cancels an uncancelled request whether or not its window has
+/// elapsed, so the committee goes straight back to sleep with its votes on the record; a
+/// second ordinary operator approves.
+///
+/// The store now reports 4 approvals, 2 of them from a committee that is no longer eligible.
+/// Narrowing the electorate has to drop those votes along with the voters: what is left is 2
+/// of 3 ordinary approvals, and a full quorum needs all three. Counting the electorate down
+/// to 3 while keeping all 4 votes would replace an operator on two ordinary approvals.
+#[tokio::test]
+async fn recovery_votes_leave_with_the_committee_that_cast_them() {
+    let outcome = settle_vote_with(
+        ProposalKindTag::ReplaceOperator,
+        mixed_tally(
+            /* ordinary_approve */ 2, /* ordinary_reject */ 0,
+            /* recovery_approve */ 2, /* recovery_reject */ 0,
+            /* total_ordinary */ 3, /* total_recovery */ 2,
+        ),
+        /* recovery_active */ false,
+        None,
+    )
+    .await;
+
+    assert_eq!(
+        outcome,
+        VoteOutcome::Pending,
+        "two of three ordinary approvals must not carry a full-quorum proposal, whatever a \
+         sleeping recovery committee voted earlier"
+    );
+}

server/crates/arbiter-server/src/actors/vault_coordinator/mod.rs

@@ -684,9 +684,7 @@ impl VaultCoordinator {
     /// then transitions to Rekeying state awaiting contributions from all of them.
     #[message]
     pub async fn start_rekey(&mut self) -> Result<(), Error> {
-        if !matches!(self.state, CoordinatorState::Idle) {
+        self.ensure_idle()?;
-            return Err(Error::AlreadyBootstrapping);
-        }
         let mut conn = self.db.get().await?;
         let ordinary_count: i64 = schema::operator_identity::table
             .count()
@@ -794,12 +792,28 @@ impl Message<ProposalApproved> for VaultCoordinator {
 }
 
 impl VaultCoordinator {
+    /// The coordinator runs one ceremony at a time; anything that starts a new one has to say
+    /// so before it changes any state the ceremony depends on.
+    const fn ensure_idle(&self) -> Result<(), Error> {
+        if matches!(self.state, CoordinatorState::Idle) {
+            Ok(())
+        } else {
+            Err(Error::AlreadyBootstrapping)
+        }
+    }
+
     /// Replaces the operator's public key in place, keeping their id and history, drops the
     /// share that key no longer matches, then begins a coordinated re-key (§3.3).
     async fn replace_operator(
         &mut self,
         settings: &replace_operator::Settings,
     ) -> Result<(), Error> {
+        // Checked before anything is written. The re-key is what gives the replaced operator
+        // a share they can use; if the coordinator is mid-ceremony, `start_rekey` refuses, and
+        // swapping the key and destroying the share first would leave that operator locked
+        // out with no re-key running and nothing to undo it -- the caller only logs the error.
+        self.ensure_idle()?;
+
         let mut conn = self.db.get().await?;
 
         diesel::update(schema::operator_identity::table)
@@ -819,3 +833,91 @@ impl VaultCoordinator {
         self.start_rekey().await
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{CoordinatorState, Error, VaultCoordinator};
+    use crate::{
+        actors::{GlobalActors, vault::Vault},
+        db::{self, models::OperatorIdentityId, proposal::replace_operator, schema},
+    };
+
+    use diesel::{ExpressionMethods as _, QueryDsl as _, dsl::insert_into};
+    use diesel_async::RunQueryDsl;
+    use kameo::actor::Spawn as _;
+    use std::collections::HashMap;
+
+    /// An approved `ReplaceOperator` that arrives while another ceremony is running must
+    /// change nothing. Swapping the public key and deleting the share are only safe because a
+    /// re-key follows and hands the operator a share for the new key; when `start_rekey`
+    /// refuses, the operator would otherwise be left holding a key with no share, and the
+    /// caller does nothing with the error but log it.
+    #[tokio::test]
+    async fn a_refused_rekey_leaves_the_operator_untouched() {
+        let pool = db::create_test_pool().await;
+        let mut conn = pool.get().await.unwrap();
+
+        let old_key = rand::random::<[u8; 32]>().to_vec();
+        let operator_id: OperatorIdentityId = insert_into(schema::operator_identity::table)
+            .values(schema::operator_identity::public_key.eq(&old_key))
+            .returning(schema::operator_identity::id)
+            .get_result(&mut conn)
+            .await
+            .unwrap();
+        insert_into(schema::operator::table)
+            .values((
+                schema::operator::id.eq(Some(operator_id)),
+                schema::operator::share.eq(vec![1u8; 32]),
+                schema::operator::share_nonce.eq(vec![2u8; 24]),
+                schema::operator::share_salt.eq(vec![3u8; 32]),
+            ))
+            .execute(&mut conn)
+            .await
+            .unwrap();
+        drop(conn);
+
+        let vault = Vault::spawn(
+            Vault::new(pool.clone(), GlobalActors::spawn_message_bus())
+                .await
+                .unwrap(),
+        );
+        let mut coordinator = VaultCoordinator::new(pool.clone(), vault);
+        coordinator.state = CoordinatorState::Rekeying {
+            ordinary_count: 2,
+            recovery_count: 0,
+            passphrases: HashMap::new(),
+            recovery_passphrases: HashMap::new(),
+        };
+
+        let result = coordinator
+            .replace_operator(&replace_operator::Settings {
+                old_operator_id: operator_id,
+                new_pubkey: vec![9u8; 32],
+            })
+            .await;
+        assert!(
+            matches!(result, Err(Error::AlreadyBootstrapping)),
+            "a busy coordinator must refuse the replacement, got {result:?}"
+        );
+
+        let mut conn = pool.get().await.unwrap();
+        let stored_key: Vec<u8> = schema::operator_identity::table
+            .find(operator_id)
+            .select(schema::operator_identity::public_key)
+            .first(&mut conn)
+            .await
+            .unwrap();
+        assert_eq!(
+            stored_key, old_key,
+            "the public key must not be swapped when no re-key can follow"
+        );
+
+        let shares: i64 = schema::operator::table
+            .filter(schema::operator::id.eq(Some(operator_id)))
+            .count()
+            .get_result(&mut conn)
+            .await
+            .unwrap();
+        assert_eq!(shares, 1, "the operator's share must not be destroyed");
+    }
+}

server/crates/arbiter-server/src/db/mod.rs

@@ -59,25 +59,39 @@ fn database_path() -> Result<std::path::PathBuf, DatabaseSetupError> {
     Ok(db_path)
 }
 
+/// The pragmas `SQLite` scopes to one connection. They are defined once and run on every
+/// connection that reaches the database -- the migration connection below and each pooled
+/// connection in `create_pool` -- because a value set on one connection is invisible to the
+/// next, and every real write happens on a pooled one.
+const CONNECTION_PRAGMAS: &str = "
+    -- sleep if the database is busy; this corresponds to up to 9 seconds sleeping time.
+    -- see https://fractaledmind.github.io/2023/09/07/enhancing-rails-sqlite-fine-tuning/
+    PRAGMA busy_timeout = 9000;
+    -- fsync only in critical moments
+    PRAGMA synchronous = NORMAL;
+    -- write WAL changes back every 1000 pages, for an in average 1MB WAL file.
+    -- May affect readers if number is increased
+    PRAGMA wal_autocheckpoint = 1000;
+    -- sqlite foreign keys are disabled by default, enable them for safety
+    PRAGMA foreign_keys = ON;
+    -- overwrite freed pages instead of leaving encrypted shares, nonces and salts
+    -- readable in the file
+    PRAGMA secure_delete = ON;
+";
+
 #[tracing::instrument(level = "info", skip(conn))]
 fn db_config(conn: &mut SqliteConnection) -> Result<(), diesel::result::Error> {
-    // fsync only in critical moments
+    conn.batch_execute(CONNECTION_PRAGMAS)?;
-    conn.batch_execute("PRAGMA synchronous = NORMAL;")?;
+
-    // write WAL changes back every 1000 pages, for an in average 1MB WAL file.
+    // The rest belong to the database file rather than the connection, so the one-shot
-    // May affect readers if number is increased
+    // migration connection is the right and only place for them.
-    conn.batch_execute("PRAGMA wal_autocheckpoint = 1000;")?;
+
     // free some space by truncating possibly massive WAL files from the last run
     conn.batch_execute("PRAGMA wal_checkpoint(TRUNCATE);")?;
 
-    // sqlite foreign keys are disabled by default, enable them for safety
-    conn.batch_execute("PRAGMA foreign_keys = ON;")?;
-
     // better space reclamation
     conn.batch_execute("PRAGMA auto_vacuum = FULL;")?;
 
-    // secure delete, overwrite deleted content with zeros to prevent recovery
-    conn.batch_execute("PRAGMA secure_delete = ON;")?;
-
     Ok(())
 }
 
@@ -120,17 +134,13 @@ pub async fn create_pool(url: Option<&str>) -> Result<DatabasePool, DatabaseSetu
         Box::pin(async move {
             let mut conn = DatabaseConnection::establish(url).await?;
 
-            // see https://fractaledmind.github.io/2023/09/07/enhancing-rails-sqlite-fine-tuning/
+            // better write-concurrency; a property of the file, but harmless to reassert
-            // sleep if the database is busy, this corresponds to up to 9 seconds sleeping time.
-            conn.batch_execute("PRAGMA busy_timeout = 9000;")
-                .await
-                .map_err(diesel::ConnectionError::CouldntSetupConfiguration)?;
-            // better write-concurrency
             conn.batch_execute("PRAGMA journal_mode = WAL;")
                 .await
                 .map_err(diesel::ConnectionError::CouldntSetupConfiguration)?;
-            // Per-connection in SQLite: the migration connection enabling it is not enough.
+            // The migration connection setting these is not enough: SQLite scopes them to
-            conn.batch_execute("PRAGMA foreign_keys = ON;")
+            // one connection, and every real query runs on a pooled one.
+            conn.batch_execute(CONNECTION_PRAGMAS)
                 .await
                 .map_err(diesel::ConnectionError::CouldntSetupConfiguration)?;
 
@@ -208,4 +218,41 @@ mod tests {
             "expected a foreign-key violation for a dangling operator_identity reference, got {result:?}"
         );
     }
+
+    #[derive(diesel::QueryableByName)]
+    struct PragmaValue {
+        #[diesel(sql_type = diesel::sql_types::Integer)]
+        value: i32,
+    }
+
+    async fn pragma(conn: &mut DatabaseConnection, name: &str) -> i32 {
+        diesel::sql_query(format!("select {name} as value from pragma_{name}()"))
+            .get_result::<PragmaValue>(conn)
+            .await
+            .unwrap()
+            .value
+    }
+
+    /// `foreign_keys` had to be repeated on the pooled connection because `SQLite` scopes it
+    /// there; its siblings in `CONNECTION_PRAGMAS` are scoped the same way and were being
+    /// left behind on the migration connection. `secure_delete` is the one that matters in a
+    /// key-custody database: off by default, it leaves freed pages holding encrypted shares,
+    /// nonces and salts readable in the file.
+    #[tokio::test]
+    async fn pooled_connections_carry_the_shared_pragmas() {
+        let pool = create_test_pool().await;
+        let mut conn = pool.get().await.unwrap();
+
+        assert_eq!(
+            pragma(&mut conn, "secure_delete").await,
+            1,
+            "freed pages must be overwritten on the connection that does the writing"
+        );
+        assert_eq!(
+            pragma(&mut conn, "synchronous").await,
+            1,
+            "synchronous must be NORMAL (1), not the default FULL (2)"
+        );
+        assert_eq!(pragma(&mut conn, "foreign_keys").await, 1);
+    }
 }

server/crates/arbiter-server/src/peers/operator/session/handlers.rs

@@ -181,13 +181,23 @@ impl OperatorSession {
         Ok(())
     }
 
+    /// A revoke that matched fewer rows than it named did not do what the operator asked:
+    /// the id was never granted, or someone revoked it first. Answering `Ok` there tells the
+    /// operator access is cut off when nothing changed. The rows that did match stay revoked
+    /// -- rolling them back to report the shortfall would leave live access behind.
     #[message]
     pub(crate) async fn handle_revoke_evm_wallet_access(
         &mut self,
         entries: Vec<i32>,
     ) -> Result<(), Error> {
         let mut conn = self.props.db.get().await?;
-        revoke_wallet_access(&mut conn, &entries).await?;
+        let revoked = revoke_wallet_access(&mut conn, &entries).await?;
+        if revoked != entries.len() {
+            return Err(Error::PartialRevoke {
+                requested: entries.len(),
+                revoked,
+            });
+        }
         Ok(())
     }
 
@@ -209,6 +219,9 @@ impl OperatorSession {
 /// Grants access, reviving a previously revoked row rather than leaving it shadowed:
 /// `uniq_wallet_access` is a unique index on `(wallet_id, client_id)`, so a plain insert
 /// would conflict forever on a row that was revoked but never deleted.
+///
+/// Reviving restores visibility and nothing else: [`revoke_wallet_access`] closes the grants
+/// that hung off the access, so a persistent grant takes its own vote again (§3.2).
 pub(crate) async fn grant_wallet_access(
     conn: &mut crate::db::DatabaseConnection,
     entries: Vec<NewEvmWalletAccess>,
@@ -231,24 +244,51 @@ pub(crate) async fn grant_wallet_access(
     .await
 }
 
-/// Marks access rows revoked by their own id rather than deleting them. The wire carries
+/// Marks access rows revoked by their own id rather than deleting them, and revokes every
-/// `WalletAccessEntry.id` values, so filtering by `wallet_id` here would revoke every
+/// grant that hangs off them. Returns how many access rows this call revoked.
-/// client's access to that wallet. Deleting is not an option: `evm_basic_grant`,
+///
+/// The wire carries `WalletAccessEntry.id` values, so filtering by `wallet_id` here would
+/// revoke every client's access to that wallet. Deleting is not an option: `evm_basic_grant`,
 /// `evm_transaction_log`, and `proposal_persistent_grant` all reference this row
 /// `on delete restrict`, so an access that was ever granted, signed with, or proposed
 /// against can never be deleted -- only marked revoked.
+///
+/// The dependent grants have to go with it. `grant_wallet_access` revives a revoked row by
+/// its id, and grant lookup keys on `wallet_access_id` alone, so leaving the grants live
+/// would make a later re-grant restore every persistent grant the access ever held, with its
+/// original volume and rate limits. §3.2 votes visibility and a persistent grant separately;
+/// a committee that approves visibility must not silently hand back signing authority it did
+/// not vote on. The filter names every requested id, not just the rows this call flipped, so
+/// an access revoked before this fix has its orphaned grants closed too.
 pub(crate) async fn revoke_wallet_access(
     conn: &mut crate::db::DatabaseConnection,
     ids: &[i32],
 ) -> Result<usize, diesel::result::Error> {
-    use crate::db::{models::SqliteTimestamp, schema::evm_wallet_access};
+    use crate::db::{
+        models::SqliteTimestamp,
+        schema::{evm_basic_grant, evm_wallet_access},
+    };
 
-    diesel::update(evm_wallet_access::table)
+    conn.transaction(async |conn| {
-        .filter(evm_wallet_access::id.eq_any(ids))
+        let now = SqliteTimestamp::now();
-        .filter(evm_wallet_access::revoked_at.is_null())
+
-        .set(evm_wallet_access::revoked_at.eq(SqliteTimestamp::now()))
+        let revoked = diesel::update(evm_wallet_access::table)
-        .execute(conn)
+            .filter(evm_wallet_access::id.eq_any(ids))
-        .await
+            .filter(evm_wallet_access::revoked_at.is_null())
+            .set(evm_wallet_access::revoked_at.eq(now.clone()))
+            .execute(&mut *conn)
+            .await?;
+
+        diesel::update(evm_basic_grant::table)
+            .filter(evm_basic_grant::wallet_access_id.eq_any(ids))
+            .filter(evm_basic_grant::revoked_at.is_null())
+            .set(evm_basic_grant::revoked_at.eq(now))
+            .execute(&mut *conn)
+            .await?;
+
+        Ok(revoked)
+    })
+    .await
 }
 
 #[messages]
@@ -511,6 +551,18 @@ mod tests {
             .unwrap()
     }
 
+    /// The grants that grant lookup would treat as live for this access: the exact filter
+    /// `EtherTransfer::try_find_grant` and `TokenTransfer::try_find_grant` apply.
+    async fn live_grants_for(conn: &mut db::DatabaseConnection, access_id: i32) -> Vec<i32> {
+        schema::evm_basic_grant::table
+            .filter(schema::evm_basic_grant::wallet_access_id.eq(access_id))
+            .filter(schema::evm_basic_grant::revoked_at.is_null())
+            .select(schema::evm_basic_grant::id)
+            .load(conn)
+            .await
+            .unwrap()
+    }
+
     /// Two clients share one wallet. Revoking one access row must leave the other alone,
     /// and must mark the row revoked rather than deleting it.
     #[tokio::test]
@@ -552,6 +604,13 @@ mod tests {
             total, 2,
             "revoking an access row must mark it revoked, not delete it"
         );
+
+        // The count `handle_revoke_evm_wallet_access` answers on: a second revoke of the same
+        // id, like a revoke of an id that never existed, changes nothing and must say so.
+        let again = revoke_wallet_access(&mut conn, &[first_access])
+            .await
+            .unwrap();
+        assert_eq!(again, 0, "an already revoked access must report no rows");
     }
 
     /// The bug this round fixes: once an access has been used for a grant, a signed
@@ -682,8 +741,6 @@ mod tests {
         let metadata_id = seed_client_metadata(&mut conn).await;
         let client_id = seed_client(&mut conn, metadata_id).await;
         let access_id = insert_wallet_access(&mut conn, wallet_id, client_id).await;
-
-        revoke_wallet_access(&mut conn, &[access_id]).await.unwrap();
         drop(conn);
 
         let vault = Vault::spawn(
@@ -706,6 +763,28 @@ mod tests {
         };
         let wallet_address = Address::from_slice(&address);
 
+        // The paired positive case: while the access stands, both lookups resolve it and the
+        // calls fail further along (no grant, sealed vault) rather than at the access filter.
+        // Without this, a filter that rejected every row would pass the assertions below.
+        let live_analyze = evm_actor
+            .shared_analyze_transaction(client_id, wallet_address, transaction.clone())
+            .await;
+        assert!(
+            !matches!(live_analyze, Err(SignTransactionError::WalletNotFound)),
+            "a live access must resolve through shared_analyze_transaction: {live_analyze:?}"
+        );
+        let live_sign = evm_actor
+            .client_sign_transaction(client_id, wallet_address, transaction.clone())
+            .await;
+        assert!(
+            !matches!(live_sign, Err(SignTransactionError::WalletNotFound)),
+            "a live access must resolve through client_sign_transaction: {live_sign:?}"
+        );
+
+        let mut conn = pool.get().await.unwrap();
+        revoke_wallet_access(&mut conn, &[access_id]).await.unwrap();
+        drop(conn);
+
         // Both lookups resolve access the same way; both must reject the revoked row before
         // ever touching the vault (neither call bootstraps one).
         let analyze_result = evm_actor
@@ -771,4 +850,82 @@ mod tests {
             "re-granting a revoked access must revive the existing row, not add a second one"
         );
     }
+
+    /// §3.2 puts wallet visibility and a persistent grant to two separate votes. Reviving a
+    /// revoked access restores visibility, and must restore nothing else: grant lookup keys on
+    /// `wallet_access_id` with `revoked_at is null`, so a grant left open when the access was
+    /// cut off would come back live -- with its original volume and rate limits -- the moment
+    /// the id revives. `EvmActor::grant_wallet_access` executes an approved `GrantWalletAccess`
+    /// proposal, so that would hand signing authority back to a committee that voted only on
+    /// visibility.
+    #[tokio::test]
+    async fn regranting_an_access_does_not_revive_its_grants() {
+        let pool = db::create_test_pool().await;
+        let mut conn = pool.get().await.unwrap();
+
+        let (wallet_id, _address) = seed_wallet(&mut conn).await;
+        let metadata_id = seed_client_metadata(&mut conn).await;
+        let client_id = seed_client(&mut conn, metadata_id).await;
+
+        let entry = || models::NewEvmWalletAccess {
+            wallet_id,
+            client_id,
+        };
+        grant_wallet_access(&mut conn, vec![entry()]).await.unwrap();
+        let access_id: i32 = schema::evm_wallet_access::table
+            .filter(schema::evm_wallet_access::wallet_id.eq(wallet_id))
+            .filter(schema::evm_wallet_access::client_id.eq(client_id))
+            .select(schema::evm_wallet_access::id)
+            .first(&mut conn)
+            .await
+            .unwrap();
+
+        // The row every persistent grant hangs off: the specific ether- or token-transfer
+        // rows reference it, so liveness is decided here.
+        let grant_id: i32 = insert_into(schema::evm_basic_grant::table)
+            .values(models::NewEvmBasicGrant {
+                wallet_access_id: access_id,
+                chain_id: 1u64.into(),
+                valid_from: None,
+                valid_until: None,
+                max_gas_fee_per_gas: None,
+                max_priority_fee_per_gas: None,
+                rate_limit_count: None,
+                rate_limit_window_secs: None,
+                revoked_at: None,
+            })
+            .returning(schema::evm_basic_grant::id)
+            .get_result(&mut conn)
+            .await
+            .unwrap();
+
+        assert_eq!(
+            live_grants_for(&mut conn, access_id).await,
+            vec![grant_id],
+            "the seeded grant must start out live, or the assertions below prove nothing"
+        );
+
+        revoke_wallet_access(&mut conn, &[access_id]).await.unwrap();
+        assert!(
+            live_grants_for(&mut conn, access_id).await.is_empty(),
+            "revoking an access must revoke the grants that hang off it"
+        );
+
+        grant_wallet_access(&mut conn, vec![entry()]).await.unwrap();
+
+        let revoked_at: Option<models::SqliteTimestamp> = schema::evm_wallet_access::table
+            .find(access_id)
+            .select(schema::evm_wallet_access::revoked_at)
+            .first(&mut conn)
+            .await
+            .unwrap();
+        assert!(
+            revoked_at.is_none(),
+            "re-granting must restore visibility for the access itself"
+        );
+        assert!(
+            live_grants_for(&mut conn, access_id).await.is_empty(),
+            "re-granting an access must not revive the grants it held before revocation"
+        );
+    }
 }

server/crates/arbiter-server/src/peers/operator/session/mod.rs

@@ -24,6 +24,12 @@ pub enum Error {
     #[error("This operator role may not perform that action")]
     RoleNotPermitted,
 
+    /// Fewer access rows were revoked than the request named. Like `RoleNotPermitted` this is
+    /// an answer about the request, not a fault, so it is named rather than folded into
+    /// `Internal`.
+    #[error("Revoked {revoked} of {requested} wallet access entries")]
+    PartialRevoke { requested: usize, revoked: usize },
+
     #[error("Internal error: {message}")]
     Internal { message: Cow<'static, str> },
 }