WIP: feat-shamir (old) #103

Draft
CleverWild wants to merge 66 commits from feat-shamir into main
Member

DO NOT MERGE!

DO NOT MERGE!
Skipper requested changes 2026-08-25 13:48:33 +00:00
Skipper left a comment
Owner

Separate database interactions in ProposalManager into separate DBAL trait, so we could mock it in the future using automock crate and test properly.

ProposalManager is also too bloated: not only does it coordinate voting, it also tried to execute the result of votes. Which is not its responsibility. Instead, it should broadcast which votes succeed on MessageBus and other relevant actors should execute it instead.

Separate database interactions in `ProposalManager` into separate DBAL trait, so we could mock it in the future using `automock` crate and test properly. `ProposalManager` is also too bloated: not only does it coordinate voting, it also tried to execute the result of votes. Which is not its responsibility. Instead, it should broadcast which votes succeed on `MessageBus` and other relevant actors should execute it instead.
@@ -8,6 +8,7 @@ use rand::RngExt;
pub static CLIENT_CONTEXT: &[u8] = b"arbiter_client";
pub static OPERATOR_CONTEXT: &[u8] = b"arbiter_operator";
pub static GOVERNANCE_CONTEXT: &[u8] = b"arbiter_governance_vote";
Owner

declare enum here with 3 possible context, associate a binary data with them:
https://docs.rs/strum/latest/strum/derive.IntoStaticStr.html

declare enum here with 3 possible context, associate a binary data with them: https://docs.rs/strum/latest/strum/derive.IntoStaticStr.html
CleverWild marked this conversation as resolved
@@ -92,1 +93,4 @@
}
#[must_use]
pub fn verify_message(&self, message: &[u8], context: &[u8], signature: &Signature) -> bool {
Owner

here accept enum from previous comment instead of random byte slice

here accept enum from previous comment instead of random byte slice
CleverWild marked this conversation as resolved
@@ -56,6 +56,7 @@ create table if not exists operator (
share blob not null,
share_nonce blob not null,
share_salt blob not null default (randomblob(32)),
Owner

there should be no default and salt should always be supplied by our daemon, wigga

there should be no default and salt should always be supplied by our daemon, wigga
CleverWild marked this conversation as resolved
@@ -218,0 +220,4 @@
create table if not exists proposal (
id integer not null primary key,
kind text not null,
payload blob not null,
Owner

why the fuck do you store binary data inside relational database. declare tables, wigga

why the fuck do you store binary data inside relational database. declare tables, wigga
CleverWild marked this conversation as resolved
@@ -218,0 +224,4 @@
initiator_id integer not null references operator_identity(id) on delete restrict,
created_at integer not null default(unixepoch('now')),
expires_at integer not null,
status text not null default 'pending'
Owner

expires_at already implied when the proposal expires, so remove expired from a list of possible values, wigga

`expires_at` already implied when the proposal expires, so remove `expired` from a list of possible values, wigga
CleverWild marked this conversation as resolved
@@ -218,0 +241,4 @@
create table if not exists proposal_result (
proposal_id integer not null primary key references proposal(id) on delete cascade,
data blob not null,
Owner

why the fuck do you store binary data inside relational database. declare tables, wigga

why the fuck do you store binary data inside relational database. declare tables, wigga
CleverWild marked this conversation as resolved
@@ -0,0 +35,4 @@
}
#[derive(Debug, Clone)]
pub enum ProposalKind {
Owner

ProposalKindTag should be derived using this enum and strum crate, wigga

`ProposalKindTag` should be derived using this enum and `strum` crate, wigga
CleverWild marked this conversation as resolved
@@ -0,0 +43,4 @@
wallet_id: i32,
client_id: i32,
},
ApproveServerUpdate,
Owner

Scope creep, remove this for now

Scope creep, remove this for now
CleverWild marked this conversation as resolved
@@ -0,0 +48,4 @@
old_operator_id: i32,
new_pubkey: Vec<u8>,
},
UpdateShamirParameters {
Owner

Explain what is the UpdateShamirParameters. Do you want to edit the quorum after committee has already formed??? wigga

Explain what is the `UpdateShamirParameters`. Do you want to edit the quorum after committee has already formed??? wigga
Author
Member

After some effort I got a very interesting answer. It was used as a trigger to start rekeying.
Replaced with a more suitable name and removed body.

After some effort I got a very interesting answer. It was used as a trigger to start rekeying. Replaced with a more suitable name and removed body.
CleverWild marked this conversation as resolved
@@ -0,0 +61,4 @@
impl ProposalKind {
pub const fn tag(&self) -> ProposalKindTag {
match self {
Owner

explained alr

explained alr
CleverWild marked this conversation as resolved
@@ -0,0 +76,4 @@
self.tag().into()
}
pub fn encode_payload(&self) -> Vec<u8> {
Owner

Wigga, please. Remove homebrew protocol, instead, make those as protobuf entities. And convert to them using already established traits, wigga

Wigga, please. Remove homebrew protocol, instead, make those as protobuf entities. And convert to them using already established traits, wigga
CleverWild marked this conversation as resolved
@@ -0,0 +174,4 @@
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum VoteOutcome {
Pending,
QuorumApproved,
Owner

remove Qurom prefix from the variants

remove `Qurom` prefix from the variants
CleverWild marked this conversation as resolved
@@ -0,0 +195,4 @@
#[error("Database query error: {0}")]
DatabaseQuery(#[from] diesel::result::Error),
#[error("Execution failed: {0}")]
ExecutionFailed(String),
Owner

ProposalManager should focus on one thing, and one thing only, the vote coordination: not the outcome execution.
Otherwise, this actor becomes too bloated, like your code currently is. Wigga.

`ProposalManager` should focus on one thing, and one thing only, the vote coordination: not the outcome execution. Otherwise, this actor becomes too bloated, like your code currently is. Wigga.
CleverWild marked this conversation as resolved
@@ -0,0 +209,4 @@
#[derive(Debug)]
pub struct ProposalSummary {
pub id: i32,
pub kind: String,
Owner

Vote kind should be a enum. Not a string. We are here forcing type-safety, only for you, wigga, to break it with your strings. wigga.

Vote kind should be a enum. Not a string. We are here forcing type-safety, only for you, wigga, to break it with your strings. wigga.
CleverWild marked this conversation as resolved
@@ -0,0 +219,4 @@
pub struct ProposalManager {
pub(crate) db: db::DatabasePool,
pub(crate) vault: ActorRef<Vault>,
pub(crate) evm: ActorRef<EvmActor>,
Owner

ProposalManager should focus on one thing, and one thing only, the vote coordination: not the outcome execution.
Otherwise, this actor becomes too bloated, like your code currently is. Wigga.

`ProposalManager` should focus on one thing, and one thing only, the vote coordination: not the outcome execution. Otherwise, this actor becomes too bloated, like your code currently is. Wigga.
CleverWild marked this conversation as resolved
@@ -0,0 +271,4 @@
) -> Result<i32, Error> {
let ttl = ttl_secs.unwrap_or(DEFAULT_TTL_SECS);
let expires_at = SqliteTimestamp::from(Utc::now() + chrono::Duration::seconds(ttl));
Owner

add maximum ttl check here

add maximum `ttl` check here
CleverWild marked this conversation as resolved
@@ -0,0 +290,4 @@
}
#[message]
pub async fn query_pending(&mut self, operator_id: i32) -> Vec<ProposalSummary> {
Owner

let's use OperatorId unit type here

let's use `OperatorId` unit type here
CleverWild marked this conversation as resolved
@@ -0,0 +303,4 @@
return vec![];
};
let voted_ids: Vec<i32> = schema::proposal_vote::table
Owner

unit type for ids here as well

unit type for ids here as well
CleverWild marked this conversation as resolved
@@ -0,0 +320,4 @@
let mut summaries = Vec::with_capacity(proposals.len());
for p in proposals {
let approve_count: i64 = schema::proposal_vote::table
Owner

2n+1 query problem here, replace those queries with aggregate ones

2n+1 query problem here, replace those queries with aggregate ones
CleverWild marked this conversation as resolved
@@ -0,0 +392,4 @@
})?;
// Check for duplicate vote before status check so AlreadyVoted takes priority
let existing: i64 = schema::proposal_vote::table
Owner

there is exists() function in diesel, which returns true / false if record exist

there is `exists()` function in diesel, which returns `true` / `false` if record exist
CleverWild marked this conversation as resolved
@@ -0,0 +421,4 @@
.map_err(|()| Error::InvalidSignature)?;
// Canonical vote message: proposal_id (i64 big-endian) || approve (u8)
let mut vote_msg = Vec::with_capacity(9);
Owner

extract this signature concatenation into either a separate function, or better into crypto submodule.

extract this signature concatenation into either a separate function, or better into `crypto` submodule.
CleverWild marked this conversation as resolved
@@ -0,0 +531,4 @@
/// §3.6: Any ordinary operator may request recovery wake-up.
/// Fails if a wake-up is already pending or active.
#[message]
pub async fn request_recovery_wakeup(&mut self, operator_id: i32) -> Result<(), Error> {
Owner

OperatorId unit type

`OperatorId` unit type
CleverWild marked this conversation as resolved
@@ -0,0 +548,4 @@
/// §3.6: Any ordinary operator may cancel a pending wake-up request.
/// Fails if there is no uncancelled request.
#[message]
pub async fn cancel_recovery_wakeup(&mut self, operator_id: i32) -> Result<(), Error> {
Owner

OperatorId unit type

`OperatorId` unit type
CleverWild marked this conversation as resolved
@@ -0,0 +609,4 @@
return Err(Error::ProposalNotPending);
}
let pubkey_bytes: Vec<u8> = schema::recovery_operator_identity::table
Owner

signature check is quite complex, so separate it into several helpers and put them into crypto submodule

signature check is quite complex, so separate it into several helpers and put them into `crypto` submodule
CleverWild marked this conversation as resolved
@@ -0,0 +715,4 @@
schema::recovery_wakeup_request::requested_at.le(diesel::dsl::sql::<
diesel::sql_types::Integer,
>(&format!(
"unixepoch('now') - {}",
Owner
https://docs.rs/diesel/latest/diesel/prelude/macro.define_sql_function.html
CleverWild marked this conversation as resolved
@@ -0,0 +727,4 @@
/// Returns true when there is any uncancelled wakeup request (pending or active).
async fn has_uncancelled_wakeup(conn: &mut db::DatabaseConnection) -> Result<bool, Error> {
let count: i64 = schema::recovery_wakeup_request::table
Owner

there is exists() function in diesel

there is `exists()` function in `diesel`
CleverWild marked this conversation as resolved
@@ -0,0 +735,4 @@
Ok(count > 0)
}
async fn execute_proposal(&self, proposal: &Proposal) -> Result<(), Error> {
Owner

remove this, separation of concerns

remove this, separation of concerns
CleverWild marked this conversation as resolved
Author
Member

JIT, wigga

JIT, wigga
CleverWild requested review from Skipper 2026-08-28 09:45:34 +00:00
Skipper changed target branch from feat-shamir-protos to main 2026-09-05 12:13:23 +00:00
Skipper added 51 commits 2026-09-05 12:13:23 +00:00
smlang generates a public state enum whose variants contain ChallengeContext,
requiring the type itself to be fully public. Also tightens the wildcard arm
in client auth to an exhaustive match.
Sets revoked_at on the evm_basic_grant row; returns NotFound if the grant
does not exist. Wires the handler in OperatorSession replacing the todo!().
Each operator row now stores a 32-byte random salt used to derive the
per-operator share encryption key from their passphrase (Argon2 KDF).
Wraps vsss_rs Gf256::split_array / combine_array into thin split_key /
combine_shares helpers. Also widens derive_key salt parameter from &[u8;16]
to &[u8] to accommodate the 32-byte share salts.
Bootstrap and TryUnseal now accept a SafeCell<Vec<u8>> seal key directly.
The Bootstrapping intermediate state is removed — multi-operator coordination
is the responsibility of VaultCoordinator, which calls Bootstrap atomically
once all shares are collected.
VaultCoordinator collects operator passphrases, splits the seal key into
Shamir shares on bootstrap (encrypting each share with the operator's
passphrase via Argon2 + XChaCha20-Poly1305), and reconstructs the seal
key from threshold shares on unseal. Adds vsss-rs 5.4.0 and rand_core 0.6
dependencies.
Adds DeclareCommittee and ContributePassphrase variants to bootstrap.proto,
ContributePassphrase to unseal.proto, and AwaitingContributions result codes
to both. Implements corresponding inbound converters and outbound reply
mappings. VaultGate handlers delegate to VaultCoordinator.
- Add `rekey.proto` with `ContributePassphrase` / `ContributeRecoveryPassphrase` / `RekeyResult`
- Wire `rekey` as a 4th vault stream payload in `vault.proto` and gRPC dispatch
- Add `RekeyRootKey` message to `Vault` actor: generates new random seal key, re-encrypts root key, writes new `root_key_history` row
- Add `StartRekey`, `ContributeRekey`, `ContributeRecoveryRekey` messages to `VaultCoordinator`; `finalize_rekey` uses threshold-1 fast path identical to bootstrap
- `execute_replace_operator` now UPDATEs `operator_identity.public_key` in-place (avoids FK constraint violation), deletes stale `operator` share row, then triggers `StartRekey`
- `execute_update_shamir_parameters` triggers `StartRekey` instead of warning stub
- `ProposalKind::ReplaceOperator` carries `old_operator_id`; encode/decode updated accordingly
- `GlobalActors::spawn` extracts `vault_coordinator` before `Ok(Self { … })` so it can be cloned into `ProposalManager::new`
- Add `handle_rekey` in session handlers forwarding passphrase contributions to `VaultCoordinator`
- Fix test: rename `replace_operator_inserts_identity_row` → `replace_operator_updates_pubkey_and_starts_rekey`, assert count stays 1 and pubkey is updated
CleverWild force-pushed feat-shamir from d8dd17ee92 to a80cd39695 2026-09-08 14:12:19 +00:00 Compare
CleverWild changed title from feat-shamir to WIP: feat-shamir (old) 2026-09-11 05:10:19 +00:00
CleverWild removed review request for Skipper 2026-09-11 05:11:58 +00:00
This pull request has changes conflicting with the target branch.
  • server/Cargo.lock
  • server/crates/arbiter-server/src/actors/bootstrap.rs
  • server/crates/arbiter-server/src/actors/evm/mod.rs
  • server/crates/arbiter-server/src/actors/vault/mod.rs
  • server/crates/arbiter-server/src/peers/operator/auth/state.rs
  • server/crates/arbiter-server/src/peers/operator/session/handlers.rs
  • server/crates/arbiter-server/src/peers/operator/session/mod.rs
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feat-shamir:feat-shamir
git checkout feat-shamir
Sign in to join this conversation.
No Reviewers
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: MarketTakers/arbiter#103