diff --git a/server/crates/arbiter-server/src/actors/evm/mod.rs b/server/crates/arbiter-server/src/actors/evm/mod.rs index 59d91c8..41ab023 100644 --- a/server/crates/arbiter-server/src/actors/evm/mod.rs +++ b/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 -/// unrepresentable value is an error, never a silently unbounded grant. +/// Converts a grant boundary from Unix seconds. `None` in means "unbounded"; a value the +/// 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) -> Result>, Error> { - secs.map(|s| chrono::DateTime::from_timestamp(s, 0).ok_or(Error::InvalidTimestamp(s))) - .transpose() + secs.map(|s| { + 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" + ); + } } diff --git a/server/crates/arbiter-server/src/actors/proposal_manager.rs b/server/crates/arbiter-server/src/actors/proposal_manager.rs index ae93685..76167c0 100644 --- a/server/crates/arbiter-server/src/actors/proposal_manager.rs +++ b/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 diff --git a/server/crates/arbiter-server/src/actors/proposal_manager/store.rs b/server/crates/arbiter-server/src/actors/proposal_manager/store.rs index 749443b..fd2c6d3 100644 --- a/server/crates/arbiter-server/src/actors/proposal_manager/store.rs +++ b/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 reject: i64, + pub ordinary_approve: 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, - reject: ordinary_reject + recovery_reject, + ordinary_approve, + ordinary_reject, + recovery_approve, + recovery_reject, total_ordinary, total_recovery, }) 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 1b50763..632f8eb 100644 --- a/server/crates/arbiter-server/src/actors/proposal_manager/tests.rs +++ b/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, - reject, + ordinary_approve: approve, + 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 -/// 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 +/// `set_status` carries an argument matcher but no `.times()`, and `None` relaxes even the +/// matcher: the caller's own `assert_eq!` on the outcome is what pins the behaviour, so a +/// regression fails on 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, ) -> 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 { - approve: 0, - reject: 3, - total_ordinary: 3, - total_recovery: 2, - }, + tally(0, 3, 3, 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 { - approve: 3, - reject: 0, - total_ordinary: 3, - total_recovery: 2, - }, + tally(3, 0, 3, 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 { - approve: 0, - reject: 3, - total_ordinary: 3, - total_recovery: 2, - }, + tally(0, 3, 3, 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 { - approve: 1, - reject: 0, - total_ordinary: 1, - total_recovery: 2, - }, + tally(1, 0, 1, 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" + ); +} diff --git a/server/crates/arbiter-server/src/actors/vault_coordinator/mod.rs b/server/crates/arbiter-server/src/actors/vault_coordinator/mod.rs index 326536e..240d99b 100644 --- a/server/crates/arbiter-server/src/actors/vault_coordinator/mod.rs +++ b/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) { - return Err(Error::AlreadyBootstrapping); - } + self.ensure_idle()?; let mut conn = self.db.get().await?; let ordinary_count: i64 = schema::operator_identity::table .count() @@ -794,12 +792,28 @@ impl Message 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 = 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"); + } +} diff --git a/server/crates/arbiter-server/src/db/mod.rs b/server/crates/arbiter-server/src/db/mod.rs index cffde4f..10b0f38 100644 --- a/server/crates/arbiter-server/src/db/mod.rs +++ b/server/crates/arbiter-server/src/db/mod.rs @@ -59,25 +59,39 @@ fn database_path() -> Result { 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("PRAGMA synchronous = NORMAL;")?; - // write WAL changes back every 1000 pages, for an in average 1MB WAL file. - // May affect readers if number is increased - conn.batch_execute("PRAGMA wal_autocheckpoint = 1000;")?; + conn.batch_execute(CONNECTION_PRAGMAS)?; + + // The rest belong to the database file rather than the connection, so the one-shot + // migration connection is the right and only place for them. + // 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 i32 { + diesel::sql_query(format!("select {name} as value from pragma_{name}()")) + .get_result::(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); + } } diff --git a/server/crates/arbiter-server/src/peers/operator/session/handlers.rs b/server/crates/arbiter-server/src/peers/operator/session/handlers.rs index a3c2173..451895f 100644 --- a/server/crates/arbiter-server/src/peers/operator/session/handlers.rs +++ b/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, ) -> 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, @@ -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 -/// `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`, +/// Marks access rows revoked by their own id rather than deleting them, and revokes every +/// grant that hangs off them. Returns how many access rows this call revoked. +/// +/// 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 { - 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) - .filter(evm_wallet_access::id.eq_any(ids)) - .filter(evm_wallet_access::revoked_at.is_null()) - .set(evm_wallet_access::revoked_at.eq(SqliteTimestamp::now())) - .execute(conn) - .await + conn.transaction(async |conn| { + let now = SqliteTimestamp::now(); + + let revoked = diesel::update(evm_wallet_access::table) + .filter(evm_wallet_access::id.eq_any(ids)) + .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 { + 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 = 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" + ); + } } diff --git a/server/crates/arbiter-server/src/peers/operator/session/mod.rs b/server/crates/arbiter-server/src/peers/operator/session/mod.rs index 9bf0777..3debd89 100644 --- a/server/crates/arbiter-server/src/peers/operator/session/mod.rs +++ b/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> }, }