WIP: feat-shamir (old) #103
Draft
CleverWild
wants to merge 66 commits from
feat-shamir into main
pull from: feat-shamir
merge into: MarketTakers:main
MarketTakers:main
MarketTakers:cleverwild/sznwsuou
MarketTakers:feat-shamir-custody
MarketTakers:feat-shamir-v2
MarketTakers:push-zvwxtnqkumlx
MarketTakers:error-handling-hardening
MarketTakers:terminate-VaultGate-connection-after-vault-lockout
MarketTakers:hashable-integrable-macro
MarketTakers:zeroized-bootstrap-token
MarketTakers:feat-auto-fix-lints
MarketTakers:enforcing-integrity
MarketTakers:push-wxnlsulvnrpz
MarketTakers:check-uac-cerf
MarketTakers:impl-useragent_delete_grant
MarketTakers:Client-key-replacement-attack
MarketTakers:critical-fix-CI-check-all-features
MarketTakers:win-service
MarketTakers:fix-proto-build-script
MarketTakers:terrors-refactor
MarketTakers:PoC-terrors
MarketTakers:push-lspnytwuyulm
MarketTakers:key-alternative
MarketTakers:push-yyxvkwvyspxv
MarketTakers:security-breaktrougth
Labels
Clear labels
Compat
Breaking
Breaking change that won't be backward compatible
Difficulty
High
3
Hard difficulty. Experience needed to fix: A lot.
Difficulty
Low
1
Easy difficulty. Experience needed to fix: Not much. Good first issue.
Difficulty
Medium
2
Medium difficulty. Experience needed to fix: Intermediate.
Kind
Bug
This is a bug.
Kind
Enhancement
Improve existing functionality.
Kind
Feature
New functionality
Kind
RFC
Request for comments
Kind
Security
This is security issue
Kind
Testing
This is testing issue.
Kind
Tracking issue
An issue tracking the progress of sth. like the implementation of an RFC
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Low
4
The priority is low
Priority
Medium
3
The priority is medium
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Pending
4
Requires review by dev
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: MarketTakers/arbiter#103
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Delete Branch "feat-shamir"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
DO NOT MERGE!
Separate database interactions in
ProposalManagerinto separate DBAL trait, so we could mock it in the future usingautomockcrate and test properly.ProposalManageris 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 onMessageBusand 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";declare enum here with 3 possible context, associate a binary data with them:
https://docs.rs/strum/latest/strum/derive.IntoStaticStr.html
@@ -92,1 +93,4 @@}#[must_use]pub fn verify_message(&self, message: &[u8], context: &[u8], signature: &Signature) -> bool {here accept enum from previous comment instead of random byte slice
@@ -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)),there should be no default and salt should always be supplied by our daemon, wigga
@@ -218,0 +220,4 @@create table if not exists proposal (id integer not null primary key,kind text not null,payload blob not null,why the fuck do you store binary data inside relational database. declare tables, wigga
@@ -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'expires_atalready implied when the proposal expires, so removeexpiredfrom a list of possible values, wigga@@ -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,why the fuck do you store binary data inside relational database. declare tables, wigga
@@ -0,0 +35,4 @@}#[derive(Debug, Clone)]pub enum ProposalKind {ProposalKindTagshould be derived using this enum andstrumcrate, wigga@@ -0,0 +43,4 @@wallet_id: i32,client_id: i32,},ApproveServerUpdate,Scope creep, remove this for now
@@ -0,0 +48,4 @@old_operator_id: i32,new_pubkey: Vec<u8>,},UpdateShamirParameters {Explain what is the
UpdateShamirParameters. Do you want to edit the quorum after committee has already formed??? wiggaAfter 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.
@@ -0,0 +61,4 @@impl ProposalKind {pub const fn tag(&self) -> ProposalKindTag {match self {explained alr
@@ -0,0 +76,4 @@self.tag().into()}pub fn encode_payload(&self) -> Vec<u8> {Wigga, please. Remove homebrew protocol, instead, make those as protobuf entities. And convert to them using already established traits, wigga
@@ -0,0 +174,4 @@#[derive(Debug, Clone, PartialEq, Eq)]pub enum VoteOutcome {Pending,QuorumApproved,remove
Quromprefix from the variants@@ -0,0 +195,4 @@#[error("Database query error: {0}")]DatabaseQuery(#[from] diesel::result::Error),#[error("Execution failed: {0}")]ExecutionFailed(String),ProposalManagershould 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.
@@ -0,0 +209,4 @@#[derive(Debug)]pub struct ProposalSummary {pub id: i32,pub kind: String,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.
@@ -0,0 +219,4 @@pub struct ProposalManager {pub(crate) db: db::DatabasePool,pub(crate) vault: ActorRef<Vault>,pub(crate) evm: ActorRef<EvmActor>,ProposalManagershould 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.
@@ -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));add maximum
ttlcheck here@@ -0,0 +290,4 @@}#[message]pub async fn query_pending(&mut self, operator_id: i32) -> Vec<ProposalSummary> {let's use
OperatorIdunit type here@@ -0,0 +303,4 @@return vec![];};let voted_ids: Vec<i32> = schema::proposal_vote::tableunit type for ids here as well
@@ -0,0 +320,4 @@let mut summaries = Vec::with_capacity(proposals.len());for p in proposals {let approve_count: i64 = schema::proposal_vote::table2n+1 query problem here, replace those queries with aggregate ones
@@ -0,0 +392,4 @@})?;// Check for duplicate vote before status check so AlreadyVoted takes prioritylet existing: i64 = schema::proposal_vote::tablethere is
exists()function in diesel, which returnstrue/falseif record exist@@ -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);extract this signature concatenation into either a separate function, or better into
cryptosubmodule.@@ -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> {OperatorIdunit type@@ -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> {OperatorIdunit type@@ -0,0 +609,4 @@return Err(Error::ProposalNotPending);}let pubkey_bytes: Vec<u8> = schema::recovery_operator_identity::tablesignature check is quite complex, so separate it into several helpers and put them into
cryptosubmodule@@ -0,0 +715,4 @@schema::recovery_wakeup_request::requested_at.le(diesel::dsl::sql::<diesel::sql_types::Integer,>(&format!("unixepoch('now') - {}",https://docs.rs/diesel/latest/diesel/prelude/macro.define_sql_function.html
@@ -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::tablethere is
exists()function indiesel@@ -0,0 +735,4 @@Ok(count > 0)}async fn execute_proposal(&self, proposal: &Proposal) -> Result<(), Error> {remove this, separation of concerns
JIT, wigga
- 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 updatedSigningContextenum a501283b0cd8dd17ee92toa80cd39695feat-shamirto WIP: feat-shamir (old)View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.