From 9ed8fab844fc6ebb1d5eabbc87fa989e78358f4a Mon Sep 17 00:00:00 2001 From: Artemii Gerasimovich Date: Wed, 27 Nov 2024 13:54:38 +0100 Subject: [PATCH] Suggestions --- crates/legacy/src/block_size_limits.rs | 17 +++++++++++-- crates/legacy/src/service.rs | 33 ++++++++++++++++++-------- crates/legacy/src/testing/mod.rs | 2 ++ crates/marketplace/src/hooks.rs | 2 +- crates/marketplace/src/lib.rs | 2 +- crates/shared/src/block.rs | 14 ++++------- crates/shared/src/coordinator/mod.rs | 6 ++++- crates/shared/src/error.rs | 5 ++++ crates/shared/src/state.rs | 21 ++++------------ crates/shared/src/testing/mock.rs | 4 ++-- 10 files changed, 64 insertions(+), 42 deletions(-) diff --git a/crates/legacy/src/block_size_limits.rs b/crates/legacy/src/block_size_limits.rs index d6e16a52..a88616b2 100644 --- a/crates/legacy/src/block_size_limits.rs +++ b/crates/legacy/src/block_size_limits.rs @@ -5,16 +5,29 @@ use std::sync::atomic::Ordering; #[derive(Debug, Clone, Copy, bytemuck::NoUninit)] #[repr(C)] pub(crate) struct MutableState { + /// Current block size limits pub max_block_size: u64, + /// Last time we've incremented the max block size, obtained + /// as [`coarsetime::Instant::as_ticks()`] pub last_block_size_increment: u64, } -/// Adjustable limits for block size ceiled by -/// maximum block size allowed by the protocol +/// Adjustable limits for block size ceiled by maximum block size allowed by the protocol. +/// We will avoid build blocks over this size limit for performance reasons: computing VID +/// for bigger blocks could be too costly and lead to API timeouts. +/// +/// Will be decremented if we fail to respond to `claim_block_header_input` request in time, +/// and periodically incremented in two cases +/// - we've served a response to `claim_block_header_input` in time and the block we've served +/// was truncated because of our current max block size policy. +/// - we've served a response to `claim_block_header_input` in time and [`Self::increment_period`] +/// has passed since last time we've incremented the block limits #[derive(Debug)] pub struct BlockSizeLimits { pub(crate) mutable_state: Atomic, + /// Maximum block size as defined by protocol. We'll never increment beyound that pub protocol_max_block_size: u64, + /// Period between optimistic increments of the block size pub increment_period: Duration, } diff --git a/crates/legacy/src/service.rs b/crates/legacy/src/service.rs index 0a6dbfba..b82accd9 100644 --- a/crates/legacy/src/service.rs +++ b/crates/legacy/src/service.rs @@ -52,9 +52,18 @@ use tagged_base64::TaggedBase64; use tide_disco::{method::ReadState, App}; use tokio::task::JoinHandle; -// We will not increment max block value if we aren't able to serve a response -// with a margin below [`ProxyGlobalState::max_api_waiting_time`] -// more than [`ProxyGlobalState::max_api_waiting_time`] / `VID_RESPONSE_TARGET_MARGIN_DIVISOR` +/// Proportion of overall allotted time to wait for optimal builder state +/// to appear before resorting to highest view builder state +const BUILDER_STATE_EXACT_MATCH_DIVISOR: u32 = 2; + +/// This constant governs duration of sleep in various retry loops +/// in the API. If we're re-trying something with a timeout of `X`, +/// we will sleep for `X / RETRY_LOOP_RESOLUTION` between attempts. +const RETRY_LOOP_RESOLUTION: u32 = 10; + +/// We will not increment max block value if we aren't able to serve a response +/// with a margin below [`GlobalState::max_api_waiting_time`] +/// more than [`GlobalState::max_api_waiting_time`] / `VID_RESPONSE_TARGET_MARGIN_DIVISOR` const VID_RESPONSE_TARGET_MARGIN_DIVISOR: u32 = 10; /// [`ALLOW_EMPTY_BLOCK_PERIOD`] is a constant that is used to determine the @@ -254,7 +263,7 @@ where match self.coordinator.lookup_builder_state(state_id).await { BuilderStateLookup::Found(builder) => break Ok(builder), BuilderStateLookup::Decided => { - return Err(Error::NotFound); + return Err(Error::AlreadyDecided); } BuilderStateLookup::NotFound => { sleep(check_period).await; @@ -272,7 +281,7 @@ where builder_state: Arc>, ) -> Result>, Error> { let timeout_after = Instant::now() + self.maximize_txn_capture_timeout; - let sleep_interval = self.maximize_txn_capture_timeout / 10; + let sleep_interval = self.maximize_txn_capture_timeout / RETRY_LOOP_RESOLUTION; while Instant::now() <= timeout_after { let queue_populated = builder_state.collect_txns(timeout_after).await; @@ -288,11 +297,14 @@ where // If the parent block had transactions included and [`ALLOW_EMPTY_BLOCK_PERIOD`] views has not // passed since, we will allow building empty blocks. This is done to allow for faster finalization // of previous blocks that have had transactions included in them. - let should_prioritize_finalization = builder_state.parent_block_references.tx_number != 0 + let should_prioritize_finalization = builder_state.parent_block_references.tx_count != 0 && builder_state .parent_block_references - .views_since_nonempty_block - .map(|value| value < ALLOW_EMPTY_BLOCK_PERIOD) + .last_nonempty_view + .map(|nonempty_view| { + nonempty_view.saturating_sub(*builder_state.parent_block_references.view_number) + < ALLOW_EMPTY_BLOCK_PERIOD + }) .unwrap_or(false); let builder: &Arc> = &builder_state; @@ -404,8 +416,9 @@ where ) -> Result>, Error> { let start = Instant::now(); - let check_period = self.max_api_waiting_time / 10; - let time_to_wait_for_matching_builder = self.max_api_waiting_time / 2; + let check_period = self.max_api_waiting_time / RETRY_LOOP_RESOLUTION; + let time_to_wait_for_matching_builder = + self.max_api_waiting_time / BUILDER_STATE_EXACT_MATCH_DIVISOR; let builder = match timeout( time_to_wait_for_matching_builder, diff --git a/crates/legacy/src/testing/mod.rs b/crates/legacy/src/testing/mod.rs index f926ab4f..3be68736 100644 --- a/crates/legacy/src/testing/mod.rs +++ b/crates/legacy/src/testing/mod.rs @@ -160,6 +160,8 @@ impl TestServiceWrapper { pub(crate) async fn submit_transactions_public(&self, transactions: Vec) { self.event_sender .broadcast(Event { + // This field is never actually used in the builder, so we can just use + // an arbitrary value view_number: ViewNumber::genesis(), event: EventType::Transactions { transactions }, }) diff --git a/crates/marketplace/src/hooks.rs b/crates/marketplace/src/hooks.rs index f95d2a06..9c787fb4 100644 --- a/crates/marketplace/src/hooks.rs +++ b/crates/marketplace/src/hooks.rs @@ -6,7 +6,7 @@ use hotshot_types::traits::node_implementation::NodeType; /// A trait for hooks into the builder service. Used to further customize /// builder behaviour in ways not possible in builder core. -/// If you don't such customisation, use [`NoHooks`]. +/// If you don't need such customisation, use [`NoHooks`]. /// /// A simple example filtering incoming transactions based on some imaginary /// application-specific magic byte: diff --git a/crates/marketplace/src/lib.rs b/crates/marketplace/src/lib.rs index 9cc59b5a..969757f0 100644 --- a/crates/marketplace/src/lib.rs +++ b/crates/marketplace/src/lib.rs @@ -2,7 +2,7 @@ //! It mainly provides this API service to hotshot proposers: //! 1. Serves a proposer(leader)'s request to provide a bundle of transactions //! -//! It also provides one API services external users: +//! It also provides one API service to external users: //! 1. Serves a user's request to submit a private transaction #![cfg_attr(coverage_nightly, feature(coverage_attribute))] diff --git a/crates/shared/src/block.rs b/crates/shared/src/block.rs index 89871b72..6625f37f 100644 --- a/crates/shared/src/block.rs +++ b/crates/shared/src/block.rs @@ -105,13 +105,9 @@ pub struct ParentBlockReferences { /// Builder commitment of the parent block payload pub builder_commitment: BuilderCommitment, /// Number of transactions included in the parent block - pub tx_number: usize, - /// Number of views since the last non-empty block, if known - /// Can be interpreted as offset from parent block to the - /// last non-empty one, i.e. 0 indicates that the parent block is - /// not empty, and 1 would indicate that this block is empty, - /// but grandparent isn't - pub views_since_nonempty_block: Option, + pub tx_count: usize, + /// Last known view that had a block with transactions + pub last_nonempty_view: Option, } impl ParentBlockReferences @@ -125,8 +121,8 @@ where vid_commitment: VidCommitment::default(), leaf_commit: fake_commitment(), builder_commitment: BuilderCommitment::from_bytes([]), - tx_number: 0, - views_since_nonempty_block: None, + tx_count: 0, + last_nonempty_view: None, } } } diff --git a/crates/shared/src/coordinator/mod.rs b/crates/shared/src/coordinator/mod.rs index caa95d3c..e38a7cbf 100644 --- a/crates/shared/src/coordinator/mod.rs +++ b/crates/shared/src/coordinator/mod.rs @@ -138,7 +138,11 @@ where pruned } - /// This function should be called whenever new transactions are received from HotShot. + /// Enqueue new transaction in all builder states managed by this coordinator. + /// + /// Builder states will automatically filter transactions already included from + /// their point of view when dequeing transactions. + /// ///
/// /// Important: [`BuilderState`]s do not automatically remove transactions from the channel. diff --git a/crates/shared/src/error.rs b/crates/shared/src/error.rs index e0fd680b..92eb36de 100644 --- a/crates/shared/src/error.rs +++ b/crates/shared/src/error.rs @@ -18,6 +18,8 @@ pub enum Error { ApiTimeout, #[error("Resource not found")] NotFound, + #[error("Request for an already decided view")] + AlreadyDecided, #[error(transparent)] BuildBlock(>::Error), #[error(transparent)] @@ -35,6 +37,9 @@ impl From> for BuildError { Error::Signing(_) => BuildError::Error("Failed to sign response".to_owned()), Error::ApiTimeout => BuildError::Error("Timeout".to_owned()), Error::NotFound => BuildError::NotFound, + Error::AlreadyDecided => { + BuildError::Error("Request for an already decided view".to_owned()) + } Error::BuildBlock(_) => BuildError::Error("Failed to build block".to_owned()), Error::TxnSender(_) => BuildError::Error("Transaction channel error".to_owned()), Error::TxTooBig { len, max_tx_len } => { diff --git a/crates/shared/src/state.rs b/crates/shared/src/state.rs index a07bf6a3..13182b62 100644 --- a/crates/shared/src/state.rs +++ b/crates/shared/src/state.rs @@ -159,22 +159,11 @@ where >::from_bytes(encoded_txns, metadata); let txn_commitments = block_payload.transaction_commitments(metadata); - let views_since_nonempty_block = if txn_commitments.is_empty() { - // If this block is empty, we should add the amount of views that passed since - // parent. We can't just assume one view has passed because we have to take - // possible failed views which don't produce da_proposals and thus don't lead to - // children spawning - self.parent_block_references - .views_since_nonempty_block - .map(|old_value| { - old_value - + (quorum_proposal - .view_number - .saturating_sub(*self.parent_block_references.view_number)) - }) + let last_nonempty_view = if txn_commitments.is_empty() { + self.parent_block_references.last_nonempty_view } else { // This block is non-empty - Some(0) + Some(quorum_proposal.view_number) }; // We replace our parent_block_references with information from the @@ -185,8 +174,8 @@ where vid_commitment: quorum_proposal.block_header.payload_commitment(), leaf_commit: Committable::commit(&leaf), builder_commitment: quorum_proposal.block_header.builder_commitment(), - tx_number: txn_commitments.len(), - views_since_nonempty_block, + tx_count: txn_commitments.len(), + last_nonempty_view, }; let mut txn_queue = self.txn_queue.read().await.clone(); diff --git a/crates/shared/src/testing/mock.rs b/crates/shared/src/testing/mock.rs index dd80fcdb..01618255 100644 --- a/crates/shared/src/testing/mock.rs +++ b/crates/shared/src/testing/mock.rs @@ -144,7 +144,7 @@ pub fn parent_references(view: u64) -> ParentBlockReferences { builder_commitment: BuilderCommitment::from_bytes( rng.sample_iter(Standard).take(32).collect::>(), ), - tx_number: rng.gen(), - views_since_nonempty_block: None, + tx_count: rng.gen(), + last_nonempty_view: None, } }