Suggestions

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<MutableState>,
+    /// 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,
 }
 

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<BuilderState<Types>>,
     ) -> Result<Option<BlockInfo<Types>>, Error<Types>> {
         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<BuilderState<Types>> = &builder_state;
@@ -404,8 +416,9 @@ where
     ) -> Result<Vec<AvailableBlockInfo<Types>>, Error<Types>> {
         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,

crates/legacy/src/testing/mod.rs

@@ -160,6 +160,8 @@ impl TestServiceWrapper {
     pub(crate) async fn submit_transactions_public(&self, transactions: Vec<TestTransaction>) {
         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 },
             })

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:

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))]
 

crates/shared/src/block.rs

@@ -105,13 +105,9 @@ pub struct ParentBlockReferences<Types: NodeType> {
     /// 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<u64>,
+    pub tx_count: usize,
+    /// Last known view that had a block with transactions
+    pub last_nonempty_view: Option<Types::View>,
 }
 
 impl<Types> ParentBlockReferences<Types>
@@ -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,
         }
     }
 }

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.
+    ///
     /// <div class="warning">
     ///
     ///   Important: [`BuilderState`]s do not automatically remove transactions from the channel.

crates/shared/src/error.rs

@@ -18,6 +18,8 @@ pub enum Error<Types: NodeType> {
     ApiTimeout,
     #[error("Resource not found")]
     NotFound,
+    #[error("Request for an already decided view")]
+    AlreadyDecided,
     #[error(transparent)]
     BuildBlock(<Types::BlockPayload as BlockPayload<Types>>::Error),
     #[error(transparent)]
@@ -35,6 +37,9 @@ impl<Types: NodeType> From<Error<Types>> 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 } => {

crates/shared/src/state.rs

@@ -159,22 +159,11 @@ where
             <Types::BlockPayload as BlockPayload<Types>>::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();

crates/shared/src/testing/mock.rs

@@ -144,7 +144,7 @@ pub fn parent_references(view: u64) -> ParentBlockReferences<TestTypes> {
         builder_commitment: BuilderCommitment::from_bytes(
             rng.sample_iter(Standard).take(32).collect::<Vec<_>>(),
         ),
-        tx_number: rng.gen(),
-        views_since_nonempty_block: None,
+        tx_count: rng.gen(),
+        last_nonempty_view: None,
     }
 }