diff --git a/crates/commonware-node/src/args.rs b/crates/commonware-node/src/args.rs index cad3bf250b..05e162e01a 100644 --- a/crates/commonware-node/src/args.rs +++ b/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 diff --git a/crates/commonware-node/src/consensus/application/actor.rs b/crates/commonware-node/src/consensus/application/actor.rs index a9947081df..23fef47ea3 100644 --- a/crates/commonware-node/src/consensus/application/actor.rs +++ b/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 { 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 { .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 { .and_then(|rsp| rsp.map_err(Into::::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 { 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, diff --git a/crates/commonware-node/src/consensus/application/mod.rs b/crates/commonware-node/src/consensus/application/mod.rs index ff5561b6a6..dd3ae8c8ad 100644 --- a/crates/commonware-node/src/consensus/application/mod.rs +++ b/crates/commonware-node/src/consensus/application/mod.rs @@ -55,8 +55,11 @@ pub(super) struct Config { /// 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, diff --git a/crates/commonware-node/src/consensus/engine.rs b/crates/commonware-node/src/consensus/engine.rs index 9c0004fbf6..ec7e05b634 100644 --- a/crates/commonware-node/src/consensus/engine.rs +++ b/crates/commonware-node/src/consensus/engine.rs @@ -89,6 +89,7 @@ pub struct Builder { 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(), diff --git a/crates/commonware-node/src/lib.rs b/crates/commonware-node/src/lib.rs index 734d9de0cb..cbab8e613f 100644 --- a/crates/commonware-node/src/lib.rs +++ b/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(), diff --git a/crates/e2e/src/lib.rs b/crates/e2e/src/lib.rs index 26bde61d2d..b5bfa47ff4 100644 --- a/crates/e2e/src/lib.rs +++ b/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),