diff --git a/crates/contract/src/api/attestation.rs b/crates/contract/src/api/attestation.rs index 457f074794..cd72ec8aea 100644 --- a/crates/contract/src/api/attestation.rs +++ b/crates/contract/src/api/attestation.rs @@ -353,6 +353,7 @@ impl MpcContract { BTreeMap::new(), ); let res = running_state.transition_to_resharing_no_checks(&proposed_parameters); + self.parameters_votes.clear(); if let Some(resharing) = res { self.protocol_state = ProtocolContractState::Resharing(resharing); } @@ -488,7 +489,9 @@ mod tests { submit_valid_attestations, }; use crate::config::Config; - use crate::primitives::key_state::{AttemptId, EpochId, KeyForDomain, Keyset}; + use crate::primitives::key_state::{ + AttemptId, AuthenticatedAccountId, EpochId, KeyForDomain, Keyset, + }; use crate::primitives::participants::{ParticipantId, Participants}; use crate::primitives::test_utils::{ bogus_ed25519_near_public_key, bogus_ed25519_public_key, bogus_tee_verifier_account_id, @@ -1001,6 +1004,12 @@ mod tests { .block_timestamp((ATTESTATION_EXPIRY_SECONDS + 1) * 1_000_000_000) // nanoseconds .build() ); + // A pending parameter vote must not survive the kickout's transition to Resharing. + let voter = AuthenticatedAccountId::new(parameters.participants()).unwrap(); + let pending_proposal = + ProposedGovernanceThresholdParameters::new(parameters.clone(), BTreeMap::new()); + contract.parameters_votes.vote(&pending_proposal, voter); + assert!(!contract.parameters_votes.is_empty()); // Call verify_tee - should trigger resharing let result = contract.verify_tee(); @@ -1009,6 +1018,7 @@ mod tests { Ok(true), "verify_tee should return Ok(true) when threshold is met" ); + assert!(contract.parameters_votes.is_empty()); // Verify contract transitioned to Resharing state let ProtocolContractState::Resharing(resharing_state) = &contract.protocol_state else { diff --git a/crates/contract/src/api/governance.rs b/crates/contract/src/api/governance.rs index 47e2083662..0b74cc48ef 100644 --- a/crates/contract/src/api/governance.rs +++ b/crates/contract/src/api/governance.rs @@ -52,10 +52,11 @@ impl MpcContract { let proposed_participants = proposal.participants(); match validation_result { TeeValidationResult::Full => { - if let Some(new_state) = self - .protocol_state - .vote_new_parameters(prospective_epoch_id, &proposal)? - { + if let Some(new_state) = self.protocol_state.vote_new_parameters( + prospective_epoch_id, + &proposal, + &mut self.parameters_votes, + )? { self.protocol_state = new_state; } Ok(()) @@ -99,6 +100,7 @@ impl MpcContract { if let Some(new_state) = self.protocol_state.vote_add_domains(domains)? { self.protocol_state = new_state; + self.parameters_votes.clear(); } Ok(()) } @@ -121,12 +123,15 @@ mod tests { }; use crate::dto_mapping::IntoInterfaceType; use crate::errors::{DomainError, InvalidCandidateSet, InvalidState, InvalidThreshold}; - use crate::primitives::key_state::{AttemptId, KeyForDomain, Keyset}; + use crate::primitives::key_state::{AttemptId, EpochId, KeyEventId, KeyForDomain, Keyset}; use crate::primitives::participants::Participants; - use crate::primitives::test_utils::{bogus_tee_verifier_account_id, gen_participants}; - use crate::primitives::threshold_votes::GovernanceThresholdParametersVotes; + use crate::primitives::test_utils::{ + authenticate_account_as, bogus_tee_verifier_account_id, gen_participants, + }; use crate::primitives::thresholds::GovernanceThresholdParameters; use crate::state::ProtocolContractState; + use crate::state::key_event::tests::find_leader; + use crate::tee::test_utils::Environment; use dtos::{Curve, DomainId, ParticipantCount, Protocol, ReconstructionThreshold}; use near_mpc_contract_interface::types::DomainPurpose; use near_sdk::test_utils::VMContextBuilder; @@ -482,11 +487,131 @@ mod tests { Error::InvalidState(InvalidState::NotParticipant { account_id }) if account_id == outsider ); // Check that vote was not accepted. + assert_matches!(&contract.protocol_state, ProtocolContractState::Running(_)); + assert!(contract.parameters_votes.is_empty()); + } + + #[test] + fn vote_add_domains__should_clear_pending_parameter_votes_on_transition() { + // Given + let (mut contract, participants, signer, domain_id) = setup_running_contract_with_domain( + 3, + GovernanceThreshold::new(2), + ReconstructionThreshold::new(2), + ); + let proposal = ProposedGovernanceThresholdParameters::new( + GovernanceThresholdParameters::new(participants.clone(), GovernanceThreshold::new(2)) + .unwrap(), + BTreeMap::new(), + ); + vote_params(&mut contract, &signer, &proposal).unwrap(); + assert!(!contract.parameters_votes.is_empty()); + let new_domain = dtos::DomainConfig { + id: DomainId(domain_id.0 + 1), + protocol: Protocol::CaitSith, + reconstruction_threshold: ReconstructionThreshold::new(2), + purpose: DomainPurpose::Sign, + }; + + // When + let mut env = Environment::new(Some(0), None, None); + for (account_id, _, _) in participants.participants() { + env.set_signer(account_id); + contract.vote_add_domains(vec![new_domain.clone()]).unwrap(); + } + + // Then + assert_matches!( + &contract.protocol_state, + ProtocolContractState::Initializing(_) + ); + assert!(contract.parameters_votes.is_empty()); + } + + /// Installs a context where `account_id` calls as an attested participant: `init_running` + /// mocks each participant's attestation with its TLS key as the account key. + fn attested_context(participants: &Participants, account_id: &AccountId) { + let info = participants.info(account_id).unwrap(); + Environment::new(Some(0), Some(account_id.clone()), None) + .set_pk(near_sdk::PublicKey::from(info.tls_public_key.clone())); + } + + /// Drives a 3-participant Running contract with one domain into Resharing: every + /// participant votes `proposal`, a key refresh keeping participants and threshold. + fn resharing_contract_with_domain() -> ( + MpcContract, + Participants, + DomainId, + ProposedGovernanceThresholdParameters, + ) { + let (mut contract, participants, _, domain_id) = setup_running_contract_with_domain( + 3, + GovernanceThreshold::new(2), + ReconstructionThreshold::new(2), + ); + let proposal = ProposedGovernanceThresholdParameters::new( + GovernanceThresholdParameters::new(participants.clone(), GovernanceThreshold::new(2)) + .unwrap(), + BTreeMap::new(), + ); + for (account_id, _, _) in participants.participants() { + vote_params(&mut contract, account_id, &proposal).unwrap(); + } + assert_matches!( + &contract.protocol_state, + ProtocolContractState::Resharing(_) + ); + (contract, participants, domain_id, proposal) + } + + #[test] + fn vote_new_parameters__should_record_a_re_proposal_vote_while_resharing() { + // Given + let (mut contract, participants, _, proposal) = resharing_contract_with_domain(); + let (voter, _, _) = &participants.participants()[0]; + Environment::new(Some(0), Some(voter.clone()), None); + + // When: a participant re-proposes at the next prospective epoch + let result = + contract.vote_new_parameters(dtos::EpochId::new(2), (&proposal).into_dto_type()); + + // Then + assert_matches!(result, Ok(())); assert_matches!( &contract.protocol_state, - ProtocolContractState::Running(state) - if state.parameters_votes == GovernanceThresholdParametersVotes::default() + ProtocolContractState::Resharing(_) ); + assert!(!contract.parameters_votes.is_empty()); + } + + #[test] + fn vote_reshared__should_clear_pending_parameter_votes_on_transition() { + // Given: a Resharing contract holding a pending re-proposal vote + let (mut contract, participants, domain_id, proposal) = resharing_contract_with_domain(); + let voter = authenticate_account_as(&participants.participants()[0].0, &participants); + contract.parameters_votes.vote(&proposal, voter); + assert!(!contract.parameters_votes.is_empty()); + + // When: the resharing completes + let ProtocolContractState::Resharing(resharing) = &contract.protocol_state else { + panic!("expected Resharing state"); + }; + let (leader, _) = find_leader(&resharing.resharing_key); + let key_event_id = KeyEventId { + attempt_id: AttemptId::new(), + domain_id, + epoch_id: EpochId::new(1), + }; + attested_context(&participants, &leader); + contract.start_reshare_instance(key_event_id).unwrap(); + for (account_id, _, _) in participants.participants() { + attested_context(&participants, account_id); + contract.vote_reshared(key_event_id).unwrap(); + } + + // Then + assert_matches!(&contract.protocol_state, ProtocolContractState::Running(_)); + assert!(contract.parameters_votes.is_empty()); } #[test] diff --git a/crates/contract/src/api/key_events.rs b/crates/contract/src/api/key_events.rs index 563254c80b..ced7760d8f 100644 --- a/crates/contract/src/api/key_events.rs +++ b/crates/contract/src/api/key_events.rs @@ -107,6 +107,7 @@ impl MpcContract { if let Some(new_state) = self.protocol_state.vote_reshared(key_event_id)? { // Resharing has concluded, transition to running state self.protocol_state = new_state; + self.parameters_votes.clear(); self.recompute_available_foreign_chains(); // Spawn a promise to clean up votes from non-participants. diff --git a/crates/contract/src/api/lifecycle.rs b/crates/contract/src/api/lifecycle.rs index 8522d2f0e5..fab4cb562e 100644 --- a/crates/contract/src/api/lifecycle.rs +++ b/crates/contract/src/api/lifecycle.rs @@ -10,6 +10,7 @@ use crate::primitives::domain::{ AddDomainsVotes, DomainRegistry, validate_domains_against_governance, }; use crate::primitives::key_state::{EpochId, Keyset}; +use crate::primitives::threshold_votes::GovernanceThresholdParametersVotes; use crate::primitives::thresholds::GovernanceThresholdParameters; use crate::state::ProtocolContractState; use crate::state::running::RunningContractState; @@ -75,6 +76,7 @@ impl MpcContract { tee_verifier_account_id, tee_verifier_votes: TeeVerifierVotes::default(), available_attestation_grants: IterableMap::new(StorageKey::AttestationGrants), + parameters_votes: GovernanceThresholdParametersVotes::default(), }) } @@ -154,6 +156,7 @@ impl MpcContract { tee_verifier_account_id, tee_verifier_votes: TeeVerifierVotes::default(), available_attestation_grants: IterableMap::new(StorageKey::AttestationGrants), + parameters_votes: GovernanceThresholdParametersVotes::default(), }) } diff --git a/crates/contract/src/api/test_utils.rs b/crates/contract/src/api/test_utils.rs index 157574de2f..c62da1ad6b 100644 --- a/crates/contract/src/api/test_utils.rs +++ b/crates/contract/src/api/test_utils.rs @@ -295,6 +295,7 @@ impl MpcContract { tee_verifier_account_id: bogus_tee_verifier_account_id(), tee_verifier_votes: Default::default(), available_attestation_grants: IterableMap::new(StorageKey::AttestationGrants), + parameters_votes: Default::default(), } } } diff --git a/crates/contract/src/dto_mapping.rs b/crates/contract/src/dto_mapping.rs index 0bd778c47a..b7182eb76f 100644 --- a/crates/contract/src/dto_mapping.rs +++ b/crates/contract/src/dto_mapping.rs @@ -14,6 +14,7 @@ use mpc_attestation::{ collateral::Collateral, }; use near_mpc_contract_interface::types as dtos; +use std::collections::BTreeMap; use crate::{ config::Config, @@ -23,7 +24,6 @@ use crate::{ domain::{AddDomainsVotes, DomainRegistry}, key_state::{AuthenticatedAccountId, AuthenticatedParticipantId, KeyForDomain, Keyset}, participants::{ParticipantInfo, Participants}, - threshold_votes::GovernanceThresholdParametersVotes, thresholds::{GovernanceThresholdParameters, ProposedGovernanceThresholdParameters}, votes::Votes, }, @@ -804,21 +804,6 @@ impl IntoInterfaceType // --- Voting types --- -impl IntoInterfaceType - for &GovernanceThresholdParametersVotes -{ - fn into_dto_type(self) -> dtos::GovernanceThresholdParametersVotes { - let proposal_by_account = self - .proposal_by_account - .iter() - .map(|(account, params)| (account.into_dto_type(), params.into_dto_type())) - .collect(); - dtos::GovernanceThresholdParametersVotes { - proposal_by_account, - } - } -} - impl IntoInterfaceType for &AddDomainsVotes { fn into_dto_type(self) -> dtos::AddDomainsVotes { dtos::AddDomainsVotes { @@ -979,7 +964,10 @@ impl IntoInterfaceType for &RunningContractState { domains: (&self.domains).into_dto_type(), keyset: (&self.keyset).into_dto_type(), parameters: (&self.parameters).into_dto_type(), - parameters_votes: (&self.parameters_votes).into_dto_type(), + // TODO(#2825): reporting dropped ahead of the switch to hash-only votes. + parameters_votes: dtos::GovernanceThresholdParametersVotes { + proposal_by_account: BTreeMap::new(), + }, add_domains_votes: (&self.add_domains_votes).into_dto_type(), previously_cancelled_resharing_epoch_id: self.previously_cancelled_resharing_epoch_id, } diff --git a/crates/contract/src/lib.rs b/crates/contract/src/lib.rs index 8e50cd2e7e..c80a31e5b3 100644 --- a/crates/contract/src/lib.rs +++ b/crates/contract/src/lib.rs @@ -30,7 +30,8 @@ mod sandbox_test_methods; pub use crate::pending_requests::MAX_PENDING_REQUEST_FAN_OUT; use crate::{ - foreign_chains_metadata::ForeignChainsMetadata, tee::tee_state::TeeState, + foreign_chains_metadata::ForeignChainsMetadata, + primitives::threshold_votes::GovernanceThresholdParametersVotes, tee::tee_state::TeeState, tee::verifier_votes::TeeVerifierVotes, update::ContractUpdateVotes, }; use config::Config; @@ -66,6 +67,10 @@ pub struct MpcContract { tee_verifier_votes: TeeVerifierVotes, /// A row is removed at zero, so the map holds no entry for an account with none. available_attestation_grants: IterableMap, + // TODO(#2825): make these storage-backed `Votes`, which is why they live outside the state. + /// Pending votes for new parameters; emptied on every protocol state transition except a + /// resharing cancellation. + parameters_votes: GovernanceThresholdParametersVotes, } impl Default for MpcContract { diff --git a/crates/contract/src/primitives/threshold_votes.rs b/crates/contract/src/primitives/threshold_votes.rs index bfcd337315..4e4213a2f8 100644 --- a/crates/contract/src/primitives/threshold_votes.rs +++ b/crates/contract/src/primitives/threshold_votes.rs @@ -6,10 +6,9 @@ use std::collections::BTreeMap; /// Tracks votes for proposed threshold parameters (new participants, threshold, /// and per-domain threshold updates). Each current participant can maintain one vote. -// TODO(#2825): Replace with Votes from votes.rs -// once this type is moved out of RunningContractState (which requires Clone + PartialEq). +// TODO(#2825): Replace with Votes from votes.rs. #[near(serializers=[borsh])] -#[derive(Clone, Debug, Default, PartialEq, Eq)] +#[derive(Debug, Default)] pub struct GovernanceThresholdParametersVotes { pub(crate) proposal_by_account: BTreeMap, @@ -52,6 +51,15 @@ impl GovernanceThresholdParametersVotes { .count(), ) } + + pub fn clear(&mut self) { + self.proposal_by_account.clear(); + } + + #[cfg(test)] + pub fn is_empty(&self) -> bool { + self.proposal_by_account.is_empty() + } } #[cfg(test)] diff --git a/crates/contract/src/snapshots/mpc_contract__tests__mpc_contract_borsh_schema_has_not_changed.snap b/crates/contract/src/snapshots/mpc_contract__tests__mpc_contract_borsh_schema_has_not_changed.snap index 0e322fa16e..47a7a7597a 100644 --- a/crates/contract/src/snapshots/mpc_contract__tests__mpc_contract_borsh_schema_has_not_changed.snap +++ b/crates/contract/src/snapshots/mpc_contract__tests__mpc_contract_borsh_schema_has_not_changed.snap @@ -615,6 +615,10 @@ BorshSchemaContainer { "available_attestation_grants", "IterableMap", ), + ( + "parameters_votes", + "GovernanceThresholdParametersVotes", + ), ], ), }, @@ -942,10 +946,6 @@ BorshSchemaContainer { "parameters", "GovernanceThresholdParameters", ), - ( - "parameters_votes", - "GovernanceThresholdParametersVotes", - ), ( "add_domains_votes", "AddDomainsVotes", diff --git a/crates/contract/src/state.rs b/crates/contract/src/state.rs index 5efb670036..b04be0d2f8 100644 --- a/crates/contract/src/state.rs +++ b/crates/contract/src/state.rs @@ -11,6 +11,7 @@ use crate::primitives::{ domain::DomainRegistry, key_state::{AuthenticatedParticipantId, EpochId, KeyEventId}, participants::Participants, + threshold_votes::GovernanceThresholdParametersVotes, thresholds::{ GovernanceThreshold, GovernanceThresholdParameters, ProposedGovernanceThresholdParameters, }, @@ -129,13 +130,14 @@ impl ProtocolContractState { &mut self, prospective_epoch_id: EpochId, proposed_parameters: &ProposedGovernanceThresholdParameters, + votes: &mut GovernanceThresholdParametersVotes, ) -> Result, Error> { match self { ProtocolContractState::Running(state) => { - state.vote_new_parameters(prospective_epoch_id, proposed_parameters) + state.vote_new_parameters(prospective_epoch_id, proposed_parameters, votes) } ProtocolContractState::Resharing(state) => { - state.vote_new_parameters(prospective_epoch_id, proposed_parameters) + state.vote_new_parameters(prospective_epoch_id, proposed_parameters, votes) } _ => Err(InvalidState::ProtocolStateNotRunningNorResharing.into()), } diff --git a/crates/contract/src/state/initializing.rs b/crates/contract/src/state/initializing.rs index 7809231d71..7a3449aa73 100644 --- a/crates/contract/src/state/initializing.rs +++ b/crates/contract/src/state/initializing.rs @@ -156,7 +156,6 @@ pub mod tests { use crate::primitives::test_utils::{ NUM_PROTOCOLS, bogus_ed25519_public_key_extended, gen_account_id, }; - use crate::primitives::threshold_votes::GovernanceThresholdParametersVotes; use crate::state::key_event::tests::find_leader; use crate::state::running::RunningContractState; use crate::state::test_utils::gen_initializing_state; @@ -306,10 +305,6 @@ pub mod tests { assert_eq!(running_state.keyset.domains, state.generated_keys); assert_eq!(running_state.keyset.domains.len(), num_domains); assert_eq!(running_state.domains, state.domains); - assert_eq!( - running_state.parameters_votes, - GovernanceThresholdParametersVotes::default() - ); assert_eq!(running_state.add_domains_votes, AddDomainsVotes::default()); } diff --git a/crates/contract/src/state/resharing.rs b/crates/contract/src/state/resharing.rs index 057ef385d6..1446968ff5 100644 --- a/crates/contract/src/state/resharing.rs +++ b/crates/contract/src/state/resharing.rs @@ -6,6 +6,7 @@ use crate::errors::{Error, InvalidParameters}; use crate::primitives::key_state::{ AuthenticatedAccountId, EpochId, KeyEventId, KeyForDomain, Keyset, }; +use crate::primitives::threshold_votes::GovernanceThresholdParametersVotes; use crate::primitives::thresholds::ProposedGovernanceThresholdParameters; use near_account_id::AccountId; use near_mpc_contract_interface::types::{DomainId, ParticipantCount, ReconstructionThreshold}; @@ -20,8 +21,8 @@ use near_sdk::near; /// This state keeps the previous running state because: /// - The previous running state's GovernanceThresholdParameters are needed in order to facilitate the /// possible re-proposal of a new GovernanceThresholdParameters, in case the currently proposed set of -/// participants are no longer all online. For tracking the votes we also use the same -/// tracking structure in the running state. +/// participants are no longer all online. Re-proposal votes are tallied in +/// [`MpcContract`](crate::MpcContract)'s `parameters_votes`, not here. /// - The previous running state's keys are needed to copy the public keys. /// - We use the previous running state's DomainRegistry. #[near(serializers=[borsh])] @@ -54,9 +55,10 @@ impl ResharingContractState { /// Note that transitioning to a new state implicitly requires `threshold` number of votes from participants of the /// previous running state. pub fn vote_new_parameters( - &mut self, + &self, prospective_epoch_id: EpochId, proposal: &ProposedGovernanceThresholdParameters, + votes: &mut GovernanceThresholdParametersVotes, ) -> Result, Error> { let expected_prospective_epoch_id = self.prospective_epoch_id().next(); if prospective_epoch_id != expected_prospective_epoch_id { @@ -68,7 +70,7 @@ impl ResharingContractState { } if self .previous_running_state - .process_new_parameters_proposal(proposal)? + .process_new_parameters_proposal(proposal, votes)? { return Ok(Some(ResharingContractState { previous_running_state: RunningContractState::new( @@ -355,10 +357,6 @@ pub mod tests { assert_eq!(running_state.keyset.domains, state.reshared_keys); assert_eq!(running_state.keyset.domains.len(), num_domains); assert_eq!(running_state.domains, state.previous_running_state.domains); - assert_eq!( - running_state.parameters_votes, - GovernanceThresholdParametersVotes::default() - ); assert_eq!(running_state.add_domains_votes, AddDomainsVotes::default()); } @@ -374,6 +372,7 @@ pub mod tests { #[test] fn test_resharing_reproposal() { + let mut votes = GovernanceThresholdParametersVotes::default(); let (mut env, mut state) = gen_resharing_state(NUM_PROTOCOLS); // Vote for first domain's key. @@ -451,10 +450,14 @@ pub mod tests { { env.set_signer(&old_participants.participants()[0].0); let _ = state - .vote_new_parameters(state.prospective_epoch_id(), &proposed_1) + .vote_new_parameters(state.prospective_epoch_id(), &proposed_1, &mut votes) .unwrap_err(); let _ = state - .vote_new_parameters(state.prospective_epoch_id().next().next(), &proposed_1) + .vote_new_parameters( + state.prospective_epoch_id().next().next(), + &proposed_1, + &mut votes, + ) .unwrap_err(); } @@ -464,12 +467,12 @@ pub mod tests { env.set_signer(account); assert!(new_state.is_none()); new_state = state - .vote_new_parameters(state.prospective_epoch_id().next(), &proposed_1) + .vote_new_parameters(state.prospective_epoch_id().next(), &proposed_1, &mut votes) .unwrap(); } // We should've gotten a new resharing state. assert!(new_state.is_some()); - let mut new_state = new_state.unwrap(); + let new_state = new_state.unwrap(); // New state should start from the beginning, with the epoch ID bumped. assert_eq!(new_state.reshared_keys.len(), 0); assert_eq!( @@ -490,7 +493,11 @@ pub mod tests { // Repropose with new_params_2. That should fail. env.set_signer(&old_participants.participants()[0].0); let _ = new_state - .vote_new_parameters(new_state.prospective_epoch_id().next(), &proposed_2) + .vote_new_parameters( + new_state.prospective_epoch_id().next(), + &proposed_2, + &mut votes, + ) .unwrap_err(); } @@ -506,6 +513,7 @@ pub mod tests { #[expect(non_snake_case)] #[test] fn vote_reshared__final_transition__should_apply_threshold_updates_to_registry() { + let mut votes = GovernanceThresholdParametersVotes::default(); // Given a CaitSith domain at the default threshold 2 and a key-refresh proposal // moving it to the GovernanceThreshold. let mut env = Environment::new(Some(100), None, None); @@ -528,7 +536,7 @@ pub mod tests { for (account, _, _) in proposal.participants().participants() { env.set_signer(account); state = running - .vote_new_parameters(prospective_epoch_id, &proposal) + .vote_new_parameters(prospective_epoch_id, &proposal, &mut votes) .unwrap(); } let mut state = state.expect("Should've transitioned into resharing"); @@ -572,6 +580,7 @@ pub mod tests { #[expect(non_snake_case)] #[test] fn vote_reshared__final_transition__should_apply_distinct_thresholds_per_domain() { + let mut votes = GovernanceThresholdParametersVotes::default(); // Given CaitSith (index 0) and Frost (index 1) at default threshold 2, and a // proposal moving only Frost to the GovernanceThreshold. let mut env = Environment::new(Some(100), None, None); @@ -593,7 +602,7 @@ pub mod tests { for (account, _, _) in proposal.participants().participants() { env.set_signer(account); state = running - .vote_new_parameters(prospective_epoch_id, &proposal) + .vote_new_parameters(prospective_epoch_id, &proposal, &mut votes) .unwrap(); } let mut state = state.expect("Should've transitioned into resharing"); diff --git a/crates/contract/src/state/running.rs b/crates/contract/src/state/running.rs index 17f5b140e8..7db7cac242 100644 --- a/crates/contract/src/state/running.rs +++ b/crates/contract/src/state/running.rs @@ -34,8 +34,6 @@ pub struct RunningContractState { pub keyset: Keyset, /// The current participants and threshold. pub parameters: GovernanceThresholdParameters, - /// Votes for proposals for a new set of participants and threshold. - pub parameters_votes: GovernanceThresholdParametersVotes, /// Votes for proposals to add new domains. pub add_domains_votes: AddDomainsVotes, /// The previous epoch id for a resharing state that was cancelled. @@ -57,7 +55,6 @@ impl RunningContractState { domains, keyset, parameters, - parameters_votes: GovernanceThresholdParametersVotes::default(), add_domains_votes: remaining_add_domain_votes, previously_cancelled_resharing_epoch_id: None, } @@ -107,6 +104,7 @@ impl RunningContractState { &mut self, prospective_epoch_id: EpochId, proposal: &ProposedGovernanceThresholdParameters, + votes: &mut GovernanceThresholdParametersVotes, ) -> Result, Error> { let expected_prospective_epoch_id = self.prospective_epoch_id(); @@ -118,7 +116,7 @@ impl RunningContractState { .into()); } - if self.process_new_parameters_proposal(proposal)? { + if self.process_new_parameters_proposal(proposal, votes)? { return Ok(self.transition_to_resharing_no_checks(proposal)); } Ok(None) @@ -139,10 +137,12 @@ impl RunningContractState { /// Casts a vote for `proposal`, removing any previous votes by [`env::signer_account_id()`](near_sdk::env::signer_account_id). /// Fails if the proposal is invalid or the signer is not a proposed participant. - /// Returns true if all participants of the proposed parameters voted for it. + /// Returns true if all participants of the proposed parameters voted for it; pending votes + /// are then cleared. pub(super) fn process_new_parameters_proposal( - &mut self, + &self, proposal: &ProposedGovernanceThresholdParameters, + votes: &mut GovernanceThresholdParametersVotes, ) -> Result { // ensure the proposal is valid against the current parameters self.parameters @@ -190,17 +190,18 @@ impl RunningContractState { // If the signer is not a participant of the current epoch, they can only vote after // `threshold` participant of the current epoch have casted their vote to admit them. if AuthenticatedAccountId::new(self.parameters.participants()).is_err() { - let n_votes = self - .parameters_votes - .n_votes(proposal, self.parameters.participants()); + let n_votes = votes.n_votes(proposal, self.parameters.participants()); if !self.parameters.threshold().is_met_by(n_votes) { return Err(VoteError::VoterPending.into()); } } // finally, vote. - let n_votes = self.parameters_votes.vote(proposal, candidate); - Ok(new_num_participants == n_votes) + let accepted = new_num_participants == votes.vote(proposal, candidate); + if accepted { + votes.clear(); + } + Ok(accepted) } /// Casts a vote for the signer participant to add new domains, replacing any previous vote. @@ -259,6 +260,7 @@ pub mod running_tests { use super::RunningContractState; use crate::errors::{Error, InvalidState, InvalidThreshold, VoteError}; use crate::primitives::domain::AddDomainsVotes; + use crate::primitives::participants::ParticipantCount; use crate::primitives::participants::Participants; use crate::primitives::test_utils::{NUM_PROTOCOLS, gen_proposed_threshold_params}; use crate::primitives::threshold_votes::GovernanceThresholdParametersVotes; @@ -283,13 +285,14 @@ pub mod running_tests { state.parameters.threshold().value() ); let mut env = Environment::new(None, None, None); + let mut votes = GovernanceThresholdParametersVotes::default(); let participants = state.parameters.participants().clone(); // Assert that random proposals get rejected. for (account_id, _, _) in participants.participants() { let ksp = gen_proposed_threshold_params(30); env.set_signer(account_id); let _ = state - .vote_new_parameters(state.keyset.epoch_id.next(), &ksp) + .vote_new_parameters(state.keyset.epoch_id.next(), &ksp, &mut votes) .unwrap_err(); } // Assert that proposals of the wrong epoch ID get rejected. @@ -297,10 +300,10 @@ pub mod running_tests { let ksp = gen_valid_params_proposal(&state.parameters); env.set_signer(&participants.participants()[0].0); let _ = state - .vote_new_parameters(state.keyset.epoch_id, &ksp) + .vote_new_parameters(state.keyset.epoch_id, &ksp, &mut votes) .unwrap_err(); let _ = state - .vote_new_parameters(state.keyset.epoch_id.next().next(), &ksp) + .vote_new_parameters(state.keyset.epoch_id.next().next(), &ksp, &mut votes) .unwrap_err(); } // Assert that disagreeing proposals do not reach consensus. @@ -327,7 +330,7 @@ pub mod running_tests { env.set_signer(account_id); assert!( state - .vote_new_parameters(state.keyset.epoch_id.next(), &proposals[i]) + .vote_new_parameters(state.keyset.epoch_id.next(), &proposals[i], &mut votes) .unwrap() .is_none() ); @@ -347,7 +350,7 @@ pub mod running_tests { n_votes += 1; env.set_signer(account_id); let res = state - .vote_new_parameters(state.keyset.epoch_id.next(), &proposal) + .vote_new_parameters(state.keyset.epoch_id.next(), &proposal, &mut votes) .unwrap(); if n_votes < proposal.participants().len() || num_domains == 0 { assert!(res.is_none()); @@ -363,7 +366,7 @@ pub mod running_tests { n_votes += 1; env.set_signer(account_id); let res = state - .vote_new_parameters(state.keyset.epoch_id.next(), &proposal) + .vote_new_parameters(state.keyset.epoch_id.next(), &proposal, &mut votes) .unwrap(); if n_votes < proposal.participants().len() || num_domains == 0 { assert!(res.is_none()); @@ -376,8 +379,8 @@ pub mod running_tests { // epoch ID, not resharing. assert_eq!(state.keyset.epoch_id, original_epoch_id.next()); assert_eq!( - state.parameters_votes, - GovernanceThresholdParametersVotes::default() + votes.n_votes(&proposal, proposal.participants()), + ParticipantCount::new(0) ); assert_eq!(state.add_domains_votes, AddDomainsVotes::default()); } else { @@ -602,7 +605,11 @@ pub mod running_tests { env.set_signer(&signer); // When voting with an empty per_domain_thresholds map (legacy shape) - let res = state.vote_new_parameters(state.keyset.epoch_id.next(), &proposal); + let res = state.vote_new_parameters( + state.keyset.epoch_id.next(), + &proposal, + &mut GovernanceThresholdParametersVotes::default(), + ); // Then the vote is recorded without error assert!( @@ -624,7 +631,11 @@ pub mod running_tests { threshold_updates.insert(DomainId(9999), ReconstructionThreshold::new(2)); let proposal = proposal.with_per_domain_thresholds(threshold_updates); let err = state - .vote_new_parameters(state.keyset.epoch_id.next(), &proposal) + .vote_new_parameters( + state.keyset.epoch_id.next(), + &proposal, + &mut GovernanceThresholdParametersVotes::default(), + ) .unwrap_err(); // Then the unknown-domain guard rejects it @@ -682,18 +693,19 @@ pub mod running_tests { { // Given let ParticipantsUpdate { - mut state, + state, proposal, removed, .. } = gen_participants_update(); let mut env = Environment::new(None, None, None); + let mut votes = GovernanceThresholdParametersVotes::default(); for voter in &removed { env.set_signer(voter); // When - let result = state.process_new_parameters_proposal(&proposal); + let result = state.process_new_parameters_proposal(&proposal, &mut votes); // Then assert_matches!( @@ -712,7 +724,7 @@ pub mod running_tests { ) { // Given let ParticipantsUpdate { - mut state, + state, proposal, retained, added, @@ -723,16 +735,19 @@ pub mod running_tests { "case must stay below the fixture's threshold" ); let mut env = Environment::new(None, None, None); + let mut votes = GovernanceThresholdParametersVotes::default(); for old in retained.iter().take(n_old_votes as usize) { env.set_signer(old); - state.process_new_parameters_proposal(&proposal).unwrap(); + state + .process_new_parameters_proposal(&proposal, &mut votes) + .unwrap(); } for voter in &added { env.set_signer(voter); // When - let result = state.process_new_parameters_proposal(&proposal); + let result = state.process_new_parameters_proposal(&proposal, &mut votes); // Then assert_matches!(result.unwrap_err(), Error::Vote(VoteError::VoterPending)); @@ -744,7 +759,7 @@ pub mod running_tests { { // Given let ParticipantsUpdate { - mut state, + state, proposal, retained, added, @@ -752,16 +767,19 @@ pub mod running_tests { } = gen_participants_update(); let threshold = state.parameters.threshold().value(); let mut env = Environment::new(None, None, None); + let mut votes = GovernanceThresholdParametersVotes::default(); for old in retained.iter().take(threshold as usize) { env.set_signer(old); - state.process_new_parameters_proposal(&proposal).unwrap(); + state + .process_new_parameters_proposal(&proposal, &mut votes) + .unwrap(); } for voter in &added { env.set_signer(voter); // When - let result = state.process_new_parameters_proposal(&proposal); + let result = state.process_new_parameters_proposal(&proposal, &mut votes); // Then assert_matches!(result, Ok(_)); @@ -773,13 +791,14 @@ pub mod running_tests { { // Given let ParticipantsUpdate { - mut state, + state, proposal, retained, added, .. } = gen_participants_update(); let mut env = Environment::new(None, None, None); + let mut votes = GovernanceThresholdParametersVotes::default(); // When let accepted: Vec = retained @@ -787,7 +806,9 @@ pub mod running_tests { .chain(&added) .map(|voter| { env.set_signer(voter); - state.process_new_parameters_proposal(&proposal).unwrap() + state + .process_new_parameters_proposal(&proposal, &mut votes) + .unwrap() }) .collect(); @@ -904,7 +925,11 @@ pub mod running_tests { threshold_updates.insert(domain_id, ReconstructionThreshold::new(1)); let proposal = proposal.with_per_domain_thresholds(threshold_updates); let err = state - .vote_new_parameters(state.keyset.epoch_id.next(), &proposal) + .vote_new_parameters( + state.keyset.epoch_id.next(), + &proposal, + &mut GovernanceThresholdParametersVotes::default(), + ) .unwrap_err(); // Then the updated value (not the stored value) is validated and rejected @@ -940,7 +965,11 @@ pub mod running_tests { let mut threshold_updates = BTreeMap::new(); threshold_updates.insert(domain_id, ReconstructionThreshold::new(3)); let proposal = proposal.with_per_domain_thresholds(threshold_updates); - let res = state.vote_new_parameters(state.keyset.epoch_id.next(), &proposal); + let res = state.vote_new_parameters( + state.keyset.epoch_id.next(), + &proposal, + &mut GovernanceThresholdParametersVotes::default(), + ); // Then the vote is recorded without error assert!( @@ -968,7 +997,11 @@ pub mod running_tests { ); let proposal = proposal.with_per_domain_thresholds(threshold_updates); let err = state - .vote_new_parameters(state.keyset.epoch_id.next(), &proposal) + .vote_new_parameters( + state.keyset.epoch_id.next(), + &proposal, + &mut GovernanceThresholdParametersVotes::default(), + ) .unwrap_err(); // Then the updated value is validated against the proposed participants @@ -1020,7 +1053,11 @@ pub mod running_tests { let mut threshold_updates = BTreeMap::new(); threshold_updates.insert(caitsith_id, ReconstructionThreshold::new(3)); let proposal = proposal.with_per_domain_thresholds(threshold_updates); - let res = state.vote_new_parameters(state.keyset.epoch_id.next(), &proposal); + let res = state.vote_new_parameters( + state.keyset.epoch_id.next(), + &proposal, + &mut GovernanceThresholdParametersVotes::default(), + ); // Then CaitSith domains may diverge in threshold. assert!(res.is_ok(), "Expected acceptance: {res:?}"); diff --git a/crates/contract/src/state/test_utils.rs b/crates/contract/src/state/test_utils.rs index 4d4c1e82f4..6ee9f10c51 100644 --- a/crates/contract/src/state/test_utils.rs +++ b/crates/contract/src/state/test_utils.rs @@ -11,6 +11,7 @@ use crate::primitives::{ key_state::{EpochId, KeyForDomain, Keyset}, participants::{ParticipantId, Participants}, test_utils::{gen_participant, gen_participants, gen_threshold_params}, + threshold_votes::GovernanceThresholdParametersVotes, thresholds::{ GovernanceThreshold, GovernanceThresholdParameters, ProposedGovernanceThresholdParameters, governance_threshold_lower_relative_bound, @@ -78,12 +79,13 @@ pub fn gen_resharing_state(num_domains: usize) -> (Environment, ResharingContrac let mut env = Environment::new(Some(100), None, None); let mut running = gen_running_state(num_domains); let proposal = gen_valid_params_proposal(&running.parameters); + let mut votes = GovernanceThresholdParametersVotes::default(); let mut resharing_state = None; for (account, _, _) in proposal.participants().participants() { env.set_signer(account); assert!(resharing_state.is_none()); resharing_state = running - .vote_new_parameters(running.keyset.epoch_id.next(), &proposal) + .vote_new_parameters(running.keyset.epoch_id.next(), &proposal, &mut votes) .unwrap(); } ( diff --git a/crates/contract/src/v3_18_0_state.rs b/crates/contract/src/v3_18_0_state.rs index f449983001..85e6e6eb47 100644 --- a/crates/contract/src/v3_18_0_state.rs +++ b/crates/contract/src/v3_18_0_state.rs @@ -7,12 +7,14 @@ //! However, this approach (a) requires manual effort from a developer and (b) increases the binary size. //! A better approach: only copy the structures that have changed and import the rest from the existing codebase. +use std::collections::{BTreeMap, BTreeSet}; + use borsh::{BorshDeserialize, BorshSerialize}; use near_mpc_contract_interface::types::{ - CKDRequest, SignatureRequest, VerifyForeignTransactionRequest, YieldIndex, + self as dtos, CKDRequest, SignatureRequest, VerifyForeignTransactionRequest, YieldIndex, }; use near_sdk::{ - AccountId, require, + AccountId, env, store::{IterableMap, Lazy, LookupMap}, }; @@ -20,17 +22,76 @@ use crate::{ config::Config, foreign_chains_metadata::ForeignChainsMetadata, node_migrations::NodeMigrations, - state::ProtocolContractState, + primitives::{ + domain::{AddDomainsVotes, DomainRegistry}, + key_state::{AuthenticatedAccountId, EpochId, KeyForDomain, Keyset}, + threshold_votes::GovernanceThresholdParametersVotes, + thresholds::{GovernanceThresholdParameters, ProposedGovernanceThresholdParameters}, + }, + state::{ + ProtocolContractState, initializing::InitializingContractState, key_event::KeyEvent, + running::RunningContractState, + }, tee::{tee_state::TeeState, verifier_votes::TeeVerifierVotes}, update::ContractUpdateVotes, }; +/// Shadow of the `3.18.0` `GovernanceThresholdParametersVotes` layout. +// TODO(#4984): drop once no deployed contract stores parameter votes by account. +#[derive(Debug, BorshSerialize, BorshDeserialize)] +struct OldGovernanceThresholdParametersVotes { + proposal_by_account: BTreeMap, +} + +/// Shadow of the `3.18.0` `RunningContractState` layout. +#[derive(Debug, BorshSerialize, BorshDeserialize)] +struct OldRunningContractState { + domains: DomainRegistry, + keyset: Keyset, + parameters: GovernanceThresholdParameters, + parameters_votes: OldGovernanceThresholdParametersVotes, + add_domains_votes: AddDomainsVotes, + previously_cancelled_resharing_epoch_id: Option, +} + +impl From for RunningContractState { + fn from(old: OldRunningContractState) -> Self { + // Pending parameter votes are dropped. Participants re-vote after the upgrade. + RunningContractState { + domains: old.domains, + keyset: old.keyset, + parameters: old.parameters, + add_domains_votes: old.add_domains_votes, + previously_cancelled_resharing_epoch_id: old.previously_cancelled_resharing_epoch_id, + } + } +} + +/// Shadow of the `3.18.0` `ResharingContractState` layout. +#[derive(Debug, BorshSerialize, BorshDeserialize)] +struct OldResharingContractState { + previous_running_state: OldRunningContractState, + reshared_keys: Vec, + resharing_key: KeyEvent, + cancellation_requests: BTreeSet, + per_domain_thresholds: BTreeMap, +} + +/// Shadow of the `3.18.0` `ProtocolContractState` layout. +#[derive(Debug, BorshSerialize, BorshDeserialize)] +enum OldProtocolContractState { + NotInitialized, + Initializing(InitializingContractState), + Running(OldRunningContractState), + Resharing(OldResharingContractState), +} + /// Keep this module in sync with [`crate::MpcContract`]: the moment a field's borsh /// layout diverges, shadow the old type here (see earlier `v*_state.rs` modules in git history /// for examples) so state written by the `3.18.0` contract still deserializes during migration. #[derive(Debug, BorshSerialize, BorshDeserialize)] pub struct MpcContract { - protocol_state: ProtocolContractState, + protocol_state: OldProtocolContractState, pending_signature_requests: LookupMap>, pending_ckd_requests: LookupMap>, pending_verify_foreign_tx_requests: LookupMap>, @@ -47,13 +108,12 @@ pub struct MpcContract { impl From for crate::MpcContract { fn from(old: MpcContract) -> Self { - require!( - matches!(old.protocol_state, ProtocolContractState::Running(_)), - "Contract must be in running state when migrating." - ); + let OldProtocolContractState::Running(running_state) = old.protocol_state else { + env::panic_str("Contract must be in running state when migrating."); + }; crate::MpcContract { - protocol_state: old.protocol_state, + protocol_state: ProtocolContractState::Running(running_state.into()), pending_signature_requests: old.pending_signature_requests, pending_ckd_requests: old.pending_ckd_requests, pending_verify_foreign_tx_requests: old.pending_verify_foreign_tx_requests, @@ -66,6 +126,72 @@ impl From for crate::MpcContract { tee_verifier_account_id: old.tee_verifier_account_id, tee_verifier_votes: old.tee_verifier_votes, available_attestation_grants: old.available_attestation_grants, + parameters_votes: GovernanceThresholdParametersVotes::default(), + } + } +} + +#[cfg(test)] +#[expect(non_snake_case)] +mod tests { + use super::*; + use crate::state::test_utils::gen_running_state; + use crate::storage_keys::StorageKey; + use near_sdk::{test_utils::VMContextBuilder, testing_env}; + + fn old_contract(protocol_state: OldProtocolContractState) -> MpcContract { + MpcContract { + protocol_state, + pending_signature_requests: LookupMap::new(StorageKey::PendingSignatureRequestsV4), + pending_ckd_requests: LookupMap::new(StorageKey::PendingCKDRequestsV3), + pending_verify_foreign_tx_requests: LookupMap::new( + StorageKey::PendingVerifyForeignTxRequestsV3, + ), + contract_update_votes: ContractUpdateVotes::default(), + config: Config::default(), + tee_state: TeeState::default(), + accept_requests: true, + node_migrations: NodeMigrations::default(), + foreign_chains: Lazy::new( + StorageKey::ForeignChainMetadata, + ForeignChainsMetadata::default(), + ), + tee_verifier_account_id: "verifier.near".parse().unwrap(), + tee_verifier_votes: TeeVerifierVotes::default(), + available_attestation_grants: IterableMap::new(StorageKey::AttestationGrants), } } + + #[test] + fn from__should_drop_pending_parameter_votes() { + // Given: a 3.18.0 running state holding one pending parameter vote + let running = gen_running_state(1); + let participants = running.parameters.participants(); + let signer = participants.participants()[0].0.clone(); + testing_env!(VMContextBuilder::new().signer_account_id(signer).build()); + let voter = AuthenticatedAccountId::new(participants).unwrap(); + let proposal = + ProposedGovernanceThresholdParameters::new(running.parameters.clone(), BTreeMap::new()); + let old = old_contract(OldProtocolContractState::Running(OldRunningContractState { + domains: running.domains.clone(), + keyset: running.keyset.clone(), + parameters: running.parameters.clone(), + parameters_votes: OldGovernanceThresholdParametersVotes { + proposal_by_account: BTreeMap::from([(voter, proposal)]), + }, + add_domains_votes: running.add_domains_votes.clone(), + previously_cancelled_resharing_epoch_id: running + .previously_cancelled_resharing_epoch_id, + })); + + // When + let migrated: crate::MpcContract = old.into(); + + // Then + assert!(migrated.parameters_votes.is_empty()); + assert_eq!( + migrated.protocol_state, + ProtocolContractState::Running(running) + ); + } } diff --git a/crates/node/src/indexer/fake.rs b/crates/node/src/indexer/fake.rs index e52b7d246a..06953eef7f 100644 --- a/crates/node/src/indexer/fake.rs +++ b/crates/node/src/indexer/fake.rs @@ -377,7 +377,6 @@ impl FakeMpcContractState { domains: state.domains.clone(), keyset: state.keyset.clone(), parameters: new_parameters, - parameters_votes: state.parameters_votes.clone(), add_domains_votes: state.add_domains_votes.clone(), previously_cancelled_resharing_epoch_id: state .previously_cancelled_resharing_epoch_id,