From e5dcecbb4f54c47676fb087085546814926c0f0c Mon Sep 17 00:00:00 2001 From: 1xstj <106580853+1xstj@users.noreply.github.com> Date: Thu, 20 Jul 2023 23:17:21 +0530 Subject: [PATCH] [fix] improvements to proposal handling (#690) * [fix] improvements to proposal handling * fix harness * fix clippy --- dkg-gadget/src/constants.rs | 61 ++++++++++++ dkg-gadget/src/gossip_engine/network.rs | 12 +-- dkg-gadget/src/keygen_manager/mod.rs | 6 +- dkg-gadget/src/lib.rs | 5 +- dkg-gadget/src/signing_manager/mod.rs | 9 +- dkg-gadget/src/worker.rs | 12 +-- dkg-test-orchestrator/src/main.rs | 2 +- pallets/dkg-proposal-handler/src/functions.rs | 96 ++++++++++++++++++- pallets/dkg-proposal-handler/src/lib.rs | 83 +++++++--------- 9 files changed, 200 insertions(+), 86 deletions(-) create mode 100644 dkg-gadget/src/constants.rs diff --git a/dkg-gadget/src/constants.rs b/dkg-gadget/src/constants.rs new file mode 100644 index 000000000..b11b1b1dd --- /dev/null +++ b/dkg-gadget/src/constants.rs @@ -0,0 +1,61 @@ +// Constants for dkg-gadget + +// ================= Common ======================== // +pub const DKG_KEYGEN_PROTOCOL_NAME: &str = "/webb-tools/dkg/keygen/1"; + +pub const DKG_SIGNING_PROTOCOL_NAME: &str = "/webb-tools/dkg/signing/1"; + +// ================= Worker ========================== // +pub mod worker { + pub const ENGINE_ID: sp_runtime::ConsensusEngineId = *b"WDKG"; + + pub const STORAGE_SET_RETRY_NUM: usize = 5; + + pub const MAX_SUBMISSION_DELAY: u32 = 3; + + pub const MAX_KEYGEN_RETRIES: usize = 5; + + /// How many blocks to keep the proposal hash in out local cache. + pub const PROPOSAL_HASH_LIFETIME: u32 = 10; +} + +// ============= Signing Manager ======================= // + +pub mod signing_manager { + // the maximum number of tasks that the work manager tries to assign + pub const MAX_RUNNING_TASKS: usize = 1; + + // the maximum number of tasks that can be enqueued, + // enqueued here implies not actively running but listening for messages + pub const MAX_ENQUEUED_TASKS: usize = 20; + + // How often to poll the jobs to check completion status + pub const JOB_POLL_INTERVAL_IN_MILLISECONDS: u64 = 500; + + // Number of signing sets to generate for every proposal + pub const MAX_POTENTIAL_SIGNING_SETS_PER_PROPOSAL: u8 = 2; +} + +// ============= Networking ======================= // + +pub mod network { + /// Maximum number of known messages hashes to keep for a peer. + pub const MAX_KNOWN_MESSAGES: usize = 4096; + + /// Maximum allowed size for a DKG Signed Message notification. + pub const MAX_MESSAGE_SIZE: u64 = 16 * 1024 * 1024; + + /// Maximum number of duplicate messages that a single peer can send us. + /// + /// This is to prevent a malicious peer from spamming us with messages. + pub const MAX_DUPLICATED_MESSAGES_PER_PEER: usize = 8; +} + +// ============= Keygen Manager ======================= // + +pub mod keygen_manager { + /// only 1 task at a time may run for keygen + pub const MAX_RUNNING_TASKS: usize = 1; + /// There should never be any job enqueueing for keygen + pub const MAX_ENQUEUED_TASKS: usize = 0; +} diff --git a/dkg-gadget/src/gossip_engine/network.rs b/dkg-gadget/src/gossip_engine/network.rs index 878970f4f..f9e67f589 100644 --- a/dkg-gadget/src/gossip_engine/network.rs +++ b/dkg-gadget/src/gossip_engine/network.rs @@ -40,6 +40,7 @@ //! engine, and it is verified then it will be added to the Engine's internal stream of DKG //! messages, later the DKG Gadget will read this stream and process the DKG message. +pub use crate::constants::network::*; use crate::{debug_logger::DebugLogger, metrics::Metrics, worker::HasLatestHeader, DKGKeystore}; use codec::{Decode, Encode}; use dkg_primitives::types::{DKGError, SignedDKGMessage}; @@ -152,17 +153,6 @@ impl NetworkGossipEngineBuilder { } } -/// Maximum number of known messages hashes to keep for a peer. -const MAX_KNOWN_MESSAGES: usize = 4096; - -/// Maximum allowed size for a DKG Signed Message notification. -const MAX_MESSAGE_SIZE: u64 = 16 * 1024 * 1024; - -/// Maximum number of duplicate messages that a single peer can send us. -/// -/// This is to prevent a malicious peer from spamming us with messages. -const MAX_DUPLICATED_MESSAGES_PER_PEER: usize = 8; - #[allow(unused)] mod rep { use sc_peerset::ReputationChange as Rep; diff --git a/dkg-gadget/src/keygen_manager/mod.rs b/dkg-gadget/src/keygen_manager/mod.rs index d38d737ca..de697720a 100644 --- a/dkg-gadget/src/keygen_manager/mod.rs +++ b/dkg-gadget/src/keygen_manager/mod.rs @@ -2,6 +2,7 @@ use crate::{ async_protocols::{remote::AsyncProtocolRemote, KeygenPartyId, KeygenRound}, + constants::keygen_manager::*, dkg_modules::KeygenProtocolSetupParameters, gossip_engine::GossipEngineIface, signing_manager::work_manager::{JobMetadata, PollMethod, WorkManager}, @@ -68,11 +69,6 @@ pub enum KeygenState { Failed { session_id: u64 }, } -/// only 1 task at a time may run for keygen -const MAX_RUNNING_TASKS: usize = 1; -/// There should never be any job enqueueing for keygen -const MAX_ENQUEUED_TASKS: usize = 0; - impl KeygenManager where B: Block, diff --git a/dkg-gadget/src/lib.rs b/dkg-gadget/src/lib.rs index d2e3dd294..7f172784b 100644 --- a/dkg-gadget/src/lib.rs +++ b/dkg-gadget/src/lib.rs @@ -43,17 +43,16 @@ pub mod worker; pub mod async_protocols; pub use dkg_logging::debug_logger; +pub mod constants; pub mod dkg_modules; pub mod gossip_messages; pub mod storage; +pub use constants::{DKG_KEYGEN_PROTOCOL_NAME, DKG_SIGNING_PROTOCOL_NAME}; pub use debug_logger::RoundsEventType; use gossip_engine::NetworkGossipEngineBuilder; pub use keystore::DKGKeystore; -pub const DKG_KEYGEN_PROTOCOL_NAME: &str = "/webb-tools/dkg/keygen/1"; -pub const DKG_SIGNING_PROTOCOL_NAME: &str = "/webb-tools/dkg/signing/1"; - /// Returns the configuration value to put in /// [`sc_network::config::NetworkConfiguration::extra_sets`]. pub fn dkg_peers_set_config( diff --git a/dkg-gadget/src/signing_manager/mod.rs b/dkg-gadget/src/signing_manager/mod.rs index 5a12dcaba..a2ed38995 100644 --- a/dkg-gadget/src/signing_manager/mod.rs +++ b/dkg-gadget/src/signing_manager/mod.rs @@ -8,6 +8,7 @@ use dkg_primitives::{ use self::work_manager::WorkManager; use crate::{ async_protocols::KeygenPartyId, + constants::signing_manager::*, dkg_modules::SigningProtocolSetupParameters, gossip_engine::GossipEngineIface, metric_inc, @@ -21,7 +22,6 @@ use dkg_runtime_primitives::crypto::Public; use sp_api::HeaderT; use std::sync::atomic::{AtomicBool, Ordering}; use webb_proposals::TypedChainId; - /// For balancing the amount of work done by each node pub mod work_manager; @@ -51,13 +51,6 @@ impl Clone for SigningManager { } } -// the maximum number of tasks that the work manager tries to assign -const MAX_RUNNING_TASKS: usize = 4; -const MAX_ENQUEUED_TASKS: usize = 20; -// How often to poll the jobs to check completion status -const JOB_POLL_INTERVAL_IN_MILLISECONDS: u64 = 500; -pub const MAX_POTENTIAL_SIGNING_SETS_PER_PROPOSAL: u8 = 2; - impl SigningManager where B: Block, diff --git a/dkg-gadget/src/worker.rs b/dkg-gadget/src/worker.rs index bb71f4b57..477b62e71 100644 --- a/dkg-gadget/src/worker.rs +++ b/dkg-gadget/src/worker.rs @@ -53,6 +53,7 @@ use dkg_runtime_primitives::{ GENESIS_AUTHORITY_SET_ID, }; +pub use crate::constants::worker::*; use crate::{ async_protocols::{remote::AsyncProtocolRemote, AsyncProtocolParameters}, dkg_modules::DKGModules, @@ -70,17 +71,6 @@ use crate::{ Client, }; -pub const ENGINE_ID: sp_runtime::ConsensusEngineId = *b"WDKG"; - -pub const STORAGE_SET_RETRY_NUM: usize = 5; - -pub const MAX_SUBMISSION_DELAY: u32 = 3; - -pub const MAX_KEYGEN_RETRIES: usize = 5; - -/// How many blocks to keep the proposal hash in out local cache. -pub const PROPOSAL_HASH_LIFETIME: u32 = 10; - pub type Shared = Arc>; pub struct WorkerParams diff --git a/dkg-test-orchestrator/src/main.rs b/dkg-test-orchestrator/src/main.rs index 89e3ef410..fde9ffc00 100644 --- a/dkg-test-orchestrator/src/main.rs +++ b/dkg-test-orchestrator/src/main.rs @@ -93,7 +93,7 @@ async fn main() -> Result<(), Box> { ); let max_signing_sets_per_proposal = - dkg_gadget::signing_manager::MAX_POTENTIAL_SIGNING_SETS_PER_PROPOSAL; + dkg_gadget::constants::signing_manager::MAX_POTENTIAL_SIGNING_SETS_PER_PROPOSAL; // first, spawn the orchestrator/mock-blockchain let orchestrator_task = MockBlockchain::new( diff --git a/pallets/dkg-proposal-handler/src/functions.rs b/pallets/dkg-proposal-handler/src/functions.rs index e4ab7b5d5..99175f414 100644 --- a/pallets/dkg-proposal-handler/src/functions.rs +++ b/pallets/dkg-proposal-handler/src/functions.rs @@ -1,6 +1,6 @@ use super::*; use dkg_runtime_primitives::handlers::decode_proposals::ProposalIdentifier; -use sp_runtime::traits::{CheckedAdd, One}; +use sp_runtime::traits::{CheckedAdd, CheckedSub, One}; impl Pallet { // *** API methods *** @@ -272,4 +272,98 @@ impl Pallet { pub fn signed_proposals_len() -> usize { SignedProposals::::iter_keys().count() } + + pub fn on_idle_create_proposal_batches(mut remaining_weight: Weight) -> Weight { + // fetch all unsigned proposals + let unsigned_proposals: Vec<_> = UnsignedProposals::::iter().collect(); + let unsigned_proposals_len = unsigned_proposals.len() as u64; + remaining_weight = + remaining_weight.saturating_sub(T::DbWeight::get().reads(unsigned_proposals_len)); + + for (typed_chain_id, unsigned_proposals) in unsigned_proposals { + remaining_weight = + remaining_weight.saturating_sub(T::DbWeight::get().reads_writes(1, 3)); + + if remaining_weight.is_zero() { + break + } + + let batch_id_res = Self::generate_next_batch_id(); + + if batch_id_res.is_err() { + log::debug!( + target: "runtime::dkg_proposal_handler", + "on_idle: Cannot generate next batch ID: {:?}", + batch_id_res, + ); + return remaining_weight + } + + let batch_id = batch_id_res.expect("checked above"); + + // create new proposal batch + let proposal_batch = StoredUnsignedProposalBatchOf:: { + batch_id, + proposals: unsigned_proposals, + timestamp: >::block_number(), + }; + // push the batch to unsigned proposal queue + UnsignedProposalQueue::::insert(typed_chain_id, batch_id, proposal_batch); + + // remove the batch from the unsigned proposal list + UnsignedProposals::::remove(typed_chain_id); + } + + remaining_weight + } + + pub fn on_idle_remove_expired_batches( + now: T::BlockNumber, + mut remaining_weight: Weight, + ) -> Weight { + use dkg_runtime_primitives::DKGPayloadKey::RefreshProposal; + + // early return if we dont have enough weight to perform a read + if remaining_weight.is_zero() { + return remaining_weight + } + + // fetch all unsigned proposals + let unsigned_proposals: Vec<_> = UnsignedProposalQueue::::iter().collect(); + let unsigned_proposals_len = unsigned_proposals.len() as u64; + remaining_weight = + remaining_weight.saturating_sub(T::DbWeight::get().reads(unsigned_proposals_len)); + + // filter out proposals to delete + let unsigned_proposal_past_expiry = unsigned_proposals.into_iter().filter( + |(_, _, StoredUnsignedProposalBatchOf:: { proposals, timestamp, .. })| { + let key = proposals.first().expect("cannot be empty").key; + + // Skip expiration for keygen related proposals + if let RefreshProposal(_) = key { + return false + } + + let time_passed = now.checked_sub(timestamp).unwrap_or_default(); + time_passed > T::UnsignedProposalExpiry::get() + }, + ); + + // remove unsigned proposal until we run out of weight + for expired_proposal in unsigned_proposal_past_expiry { + remaining_weight = + remaining_weight.saturating_sub(T::DbWeight::get().writes(One::one())); + + if remaining_weight.is_zero() { + break + } + Self::deposit_event(Event::::ProposalBatchExpired { + target_chain: expired_proposal.0, + batch_id: expired_proposal.1, + }); + UnsignedProposalQueue::::remove(expired_proposal.0, expired_proposal.1); + } + + remaining_weight + } } diff --git a/pallets/dkg-proposal-handler/src/lib.rs b/pallets/dkg-proposal-handler/src/lib.rs index c36289194..fb06f2067 100644 --- a/pallets/dkg-proposal-handler/src/lib.rs +++ b/pallets/dkg-proposal-handler/src/lib.rs @@ -150,7 +150,6 @@ pub mod pallet { use frame_support::dispatch::DispatchResultWithPostInfo; use frame_system::{offchain::CreateSignedTransaction, pallet_prelude::*}; use log; - use sp_runtime::traits::Zero; use webb_proposals::Proposal; use super::*; @@ -295,13 +294,18 @@ pub mod pallet { data: Vec, }, /// RuntimeEvent When a Proposal is removed from UnsignedProposalQueue. - ProposalRemoved { - /// The Payload Type or the Key. - key: DKGPayloadKey, + ProposalBatchRemoved { /// The Target Chain. target_chain: TypedChainId, - /// Whether the proposal is due to expiration - expired: bool, + /// The batch ID of the proposal + batch_id: T::BatchId, + }, + /// RuntimeEvent When a Proposal is expired and removed from UnsignedProposalQueue. + ProposalBatchExpired { + /// The Target Chain. + target_chain: TypedChainId, + /// The batch ID of the proposal + batch_id: T::BatchId, }, /// RuntimeEvent When a Proposal Gets Signed by DKG. ProposalBatchSigned { @@ -349,6 +353,8 @@ pub mod pallet { ArithmeticOverflow, /// Batch does not contain proposals EmptyBatch, + /// Proposal batch does not exist + ProposalBatchNotFound, } #[pallet::hooks] @@ -371,47 +377,11 @@ pub mod pallet { return remaining_weight } - // fetch all unsigned proposals - let unsigned_proposals: Vec<_> = UnsignedProposals::::iter().collect(); - let unsigned_proposals_len = unsigned_proposals.len() as u64; - remaining_weight = - remaining_weight.saturating_sub(T::DbWeight::get().reads(unsigned_proposals_len)); - - for (typed_chain_id, unsigned_proposals) in unsigned_proposals { - remaining_weight = - remaining_weight.saturating_sub(T::DbWeight::get().reads_writes(1, 3)); - - if remaining_weight.is_zero() { - break - } - - let batch_id_res = Self::generate_next_batch_id(); - - if batch_id_res.is_err() { - log::debug!( - target: "runtime::dkg_proposal_handler", - "on_idle: Cannot generate next batch ID: {:?}", - batch_id_res, - ); - return remaining_weight - } - - let batch_id = batch_id_res.expect("checked above"); - - // create new proposal batch - let proposal_batch = StoredUnsignedProposalBatchOf:: { - batch_id, - proposals: unsigned_proposals, - timestamp: >::block_number(), - }; - // push the batch to unsigned proposal queue - UnsignedProposalQueue::::insert(typed_chain_id, batch_id, proposal_batch); - - // remove the batch from the unsigned proposal list - UnsignedProposals::::remove(typed_chain_id); - } + // create proposal batches + remaining_weight = Self::on_idle_create_proposal_batches(remaining_weight); - remaining_weight + // remove expired proposals with remaining weight + Self::on_idle_remove_expired_batches(now, remaining_weight) } } @@ -520,6 +490,27 @@ pub mod pallet { Err(Error::::ProposalMustBeUnsigned.into()) } } + + #[pallet::weight(::WeightInfo::force_submit_unsigned_proposal())] + #[pallet::call_index(2)] + pub fn force_remove_unsigned_proposal_batch( + origin: OriginFor, + typed_chain_id: TypedChainId, + batch_id: T::BatchId, + ) -> DispatchResultWithPostInfo { + // Call must come from root (likely from a democracy proposal passing) + ::ForceOrigin::ensure_origin(origin)?; + ensure!( + UnsignedProposalQueue::::contains_key(typed_chain_id, batch_id), + Error::::ProposalBatchNotFound + ); + UnsignedProposalQueue::::remove(typed_chain_id, batch_id); + Self::deposit_event(Event::ProposalBatchRemoved { + target_chain: typed_chain_id, + batch_id, + }); + Ok(().into()) + } } #[pallet::validate_unsigned]