feat(payload): separate builder interrupt and wait deadlines

crates/commonware-node/src/args.rs

@@ -97,12 +97,28 @@ pub struct Args {
     )]
     pub inactive_views_until_leader_skip: u64,
 
-    /// The amount of time this node will use to construct a block as a proposal.
+    /// The maximum amount of time to spend on executing transactions when preparing a proposal as a leader.
+    ///
+    /// NOTE: This only limits the time the builder spends on transaction execution, and does not
+    /// include the state root calculation time. For this reason, we keep it well below `consensus.time-to-build-proposal`.
+    #[arg(
+        long = "consensus.time-to-prepare-proposal-transactions",
+        default_value = "200ms"
+    )]
+    pub time_to_prepare_proposal_transactions: PositiveDuration,
+
+    /// The minimum amount of time this node waits before sending a proposal
+    ///
+    /// The intention is to keep block times stable even if there is low load on the network.
     /// This value should be well below `consensus.wait-for-proposal` to account
     /// for the leader to enter the view, build and broadcast the proposal, and
     /// have the other peers receive the proposal.
-    #[arg(long = "consensus.time-to-build-proposal", default_value = "500ms")]
-    pub time_to_build_proposal: PositiveDuration,
+    #[arg(
+        long = "consensus.minimum-time-before-propose",
+        alias = "consensus.time-to-build-proposal",
+        default_value = "450ms"
+    )]
+    pub minimum_time_before_propose: PositiveDuration,
 
     /// The amount of time this node will use to construct a subblock before
     /// sending it to the next proposer. This value should be well below

crates/commonware-node/src/consensus/application/actor.rs

@@ -84,7 +84,8 @@ where
                 fee_recipient: config.fee_recipient,
                 epoch_strategy: config.epoch_strategy,
 
-                new_payload_wait_time: config.new_payload_wait_time,
+                payload_resolve_time: config.payload_resolve_time,
+                payload_return_time: config.payload_return_time,
 
                 my_mailbox,
                 marshal: config.marshal,
@@ -185,7 +186,8 @@ where
 struct Inner<TState> {
     fee_recipient: alloy_primitives::Address,
     epoch_strategy: FixedEpocher,
-    new_payload_wait_time: Duration,
+    payload_resolve_time: Duration,
+    payload_return_time: Duration,
 
     my_mailbox: Mailbox,
 
@@ -573,10 +575,23 @@ impl Inner<Init> {
             .wrap_err("failed requesting new payload from the execution layer")?;
 
         debug!(
-            timeout_ms = self.new_payload_wait_time.as_millis(),
-            "sleeping for payload builder timeout"
+            resolve_time_ms = self.payload_resolve_time.as_millis(),
+            return_time_ms = self.payload_return_time.as_millis(),
+            "sleeping before payload builder resolving"
         );
-        context.sleep(self.new_payload_wait_time).await;
+
+        // Start the timer for `self.payload_return_time`
+        //
+        // This guarantees that we will not propose the block too early, and waits for at least `self.payload_return_time`,
+        // plus whatever time is needed to finish building the block.
+        let payload_return_time = context.current() + self.payload_return_time;
+
+        // Give payload builder at least `self.payload_resolve_time` until we interrupt it.
+        //
+        // The interrupt doesn't mean we'll immediately get the payload back,
+        // but only signals the builder to stop executing transactions,
+        // and start calculating the state root and sealing the block.
+        context.sleep(self.payload_resolve_time).await;
 
         interrupt_handle.interrupt();
 
@@ -593,6 +608,9 @@ impl Inner<Init> {
             .and_then(|rsp| rsp.map_err(Into::<eyre::Report>::into))
             .wrap_err_with(|| format!("failed getting payload for payload ID `{payload_id}`"))?;
 
+        // Keep waiting for `self.payload_return_time`, if there's anything left after building the block.
+        context.sleep_until(payload_return_time).await;
+
         Ok(Block::from_execution_block(payload.block().clone()))
     }
 
@@ -705,7 +723,8 @@ impl Inner<Uninit> {
         let initialized = Inner {
             fee_recipient: self.fee_recipient,
             epoch_strategy: self.epoch_strategy,
-            new_payload_wait_time: self.new_payload_wait_time,
+            payload_resolve_time: self.payload_resolve_time,
+            payload_return_time: self.payload_return_time,
             my_mailbox: self.my_mailbox,
             marshal: self.marshal,
             execution_node: self.execution_node,

crates/commonware-node/src/consensus/application/mod.rs

@@ -55,8 +55,11 @@ pub(super) struct Config<TContext> {
     /// A handle to the subblocks service to get subblocks for proposals.
     pub(crate) subblocks: subblocks::Mailbox,
 
-    /// The minimum amount of time to wait before resolving a new payload from the builder
-    pub(super) new_payload_wait_time: Duration,
+    /// The minimum amount of time to wait before resolving a new payload from the builder.
+    pub(super) payload_resolve_time: Duration,
+
+    /// The minimum amount of time to wait before returning the built payload back to consensus for proposal.
+    pub(super) payload_return_time: Duration,
 
     /// The epoch strategy used by tempo, to map block heights to epochs.
     pub(super) epoch_strategy: FixedEpocher,

crates/commonware-node/src/consensus/engine.rs

@@ -89,6 +89,7 @@ pub struct Builder<TBlocker, TPeerManager> {
     pub time_for_peer_response: Duration,
     pub views_to_track: u64,
     pub views_until_leader_skip: u64,
+    pub payload_interrupt_time: Duration,
     pub new_payload_wait_time: Duration,
     pub time_to_build_subblock: Duration,
     pub subblock_broadcast_interval: Duration,
@@ -347,7 +348,8 @@ where
             marshal: marshal_mailbox.clone(),
             execution_node: execution_node.clone(),
             executor: executor_mailbox.clone(),
-            new_payload_wait_time: self.new_payload_wait_time,
+            payload_resolve_time: self.payload_interrupt_time,
+            payload_return_time: self.new_payload_wait_time,
             subblocks: subblocks.mailbox(),
             scheme_provider: scheme_provider.clone(),
             epoch_strategy: epoch_strategy.clone(),

crates/commonware-node/src/lib.rs

@@ -107,7 +107,8 @@ pub async fn run_consensus_stack(
         time_for_peer_response: config.wait_for_peer_response.into_duration(),
         views_to_track: config.views_to_track,
         views_until_leader_skip: config.inactive_views_until_leader_skip,
-        new_payload_wait_time: config.time_to_build_proposal.into_duration(),
+        payload_interrupt_time: config.time_to_prepare_proposal_transactions.into_duration(),
+        new_payload_wait_time: config.minimum_time_before_propose.into_duration(),
         time_to_build_subblock: config.time_to_build_subblock.into_duration(),
         subblock_broadcast_interval: config.subblock_broadcast_interval.into_duration(),
         fcu_heartbeat_interval: config.fcu_heartbeat_interval.into_duration(),

crates/e2e/src/lib.rs

@@ -252,7 +252,8 @@ pub async fn setup_validators(
             time_for_peer_response: Duration::from_secs(2),
             views_to_track: 10,
             views_until_leader_skip: 5,
-            new_payload_wait_time: Duration::from_millis(200),
+            payload_interrupt_time: Duration::from_millis(200),
+            new_payload_wait_time: Duration::from_millis(300),
             time_to_build_subblock: Duration::from_millis(100),
             subblock_broadcast_interval: Duration::from_millis(50),
             fcu_heartbeat_interval: Duration::from_secs(300),