Simplify logic and variables involved in determining when to broadcas…

…t a Proposal (#1706)

* feat: remove has_txn and exponential timeout check from block proposal timeout

* feat: propose blocks as soon as empty_block_timeout is reached

* fix: rebase corrections

* chore: devnet version 5

* fix: Change namings to make code purpose clearer

* fix: update time for broadcast to 100ms

z2/src/setup.rs

@@ -24,12 +24,11 @@ use zilliqa::{
 use zilliqa::{
     cfg::{
         self, allowed_timestamp_skew_default, block_request_batch_size_default,
-        block_request_limit_default, consensus_timeout_default, empty_block_timeout_default,
+        block_request_limit_default, block_time_default, consensus_timeout_default,
         eth_chain_id_default, failed_request_sleep_duration_default, local_address_default,
-        max_blocks_in_flight_default, minimum_time_left_for_empty_block_default,
-        scilla_address_default, scilla_ext_libs_path_default, scilla_stdlib_dir_default,
-        state_rpc_limit_default, total_native_token_supply_default, Amount, ConsensusConfig,
-        ContractUpgradesBlockHeights, Forks, GenesisDeposit,
+        max_blocks_in_flight_default, scilla_address_default, scilla_ext_libs_path_default,
+        scilla_stdlib_dir_default, state_rpc_limit_default, total_native_token_supply_default,
+        Amount, ConsensusConfig, ContractUpgradesBlockHeights, Forks, GenesisDeposit,
     },
     transaction::EvmGas,
 };
@@ -521,15 +520,14 @@ impl Setup {
                     scilla_address: scilla_address_default(),
                     scilla_stdlib_dir: scilla_stdlib_dir_default(),
                     scilla_ext_libs_path: scilla_ext_libs_path_default(),
-                    minimum_time_left_for_empty_block: minimum_time_left_for_empty_block_default(),
                     main_shard_id: None,
                     local_address: local_address_default(),
                     consensus_timeout: consensus_timeout_default(),
                     genesis_deposits: Vec::new(),
                     eth_block_gas_limit: EvmGas(84000000),
                     gas_price: 4_761_904_800_000u128.into(),
                     minimum_stake: 10_000_000_000_000_000_000_000_000u128.into(),
-                    empty_block_timeout: empty_block_timeout_default(),
+                    block_time: block_time_default(),
                     genesis_accounts: Vec::new(),
                     is_main: true,
                     blocks_per_hour: 3600,

zilliqa/src/api/admin.rs

@@ -38,7 +38,7 @@ fn consensus_info(_: Params, node: &Arc<Mutex<Node>>) -> Result<ConsensusInfo> {
 
     let view = node.consensus.get_view()?;
     let high_qc = QuorumCertificate::from_qc(&node.consensus.high_qc);
-    let (milliseconds_since_last_view_change, exponential_backoff_timeout, _) =
+    let (milliseconds_since_last_view_change, _, exponential_backoff_timeout) =
         node.consensus.get_consensus_timeout_params()?;
     let milliseconds_until_next_view_change =
         exponential_backoff_timeout.saturating_sub(milliseconds_since_last_view_change);

zilliqa/src/cfg.rs

@@ -314,13 +314,9 @@ pub struct ConsensusConfig {
     /// Accounts that will be pre-funded at genesis.
     #[serde(default)]
     pub genesis_accounts: Vec<(Address, Amount)>,
-    /// Minimum time to wait for consensus to propose new block if there are no transactions. This therefore acts also as the minimum block time.
-    #[serde(default = "empty_block_timeout_default")]
-    pub empty_block_timeout: Duration,
-    /// Minimum remaining time before end of round in which Proposer has the opportunity to broadcast empty block proposal.
-    /// If there is less time than this value left in a round then the view will likely move on before a proposal has time to be finalised.
-    #[serde(default = "minimum_time_left_for_empty_block_default")]
-    pub minimum_time_left_for_empty_block: Duration,
+    /// The expected time between blocks when no views are missed.
+    #[serde(default = "block_time_default")]
+    pub block_time: Duration,
     /// Address of the Scilla server. Defaults to "http://localhost:3000".
     #[serde(default = "scilla_address_default")]
     pub scilla_address: String,
@@ -381,8 +377,7 @@ impl Default for ConsensusConfig {
             consensus_timeout: consensus_timeout_default(),
             genesis_deposits: vec![],
             genesis_accounts: vec![],
-            empty_block_timeout: empty_block_timeout_default(),
-            minimum_time_left_for_empty_block: minimum_time_left_for_empty_block_default(),
+            block_time: block_time_default(),
             scilla_address: scilla_address_default(),
             scilla_stdlib_dir: scilla_stdlib_dir_default(),
             scilla_ext_libs_path: scilla_ext_libs_path_default(),
@@ -489,14 +484,10 @@ pub fn consensus_timeout_default() -> Duration {
     Duration::from_secs(5)
 }
 
-pub fn empty_block_timeout_default() -> Duration {
+pub fn block_time_default() -> Duration {
     Duration::from_millis(1000)
 }
 
-pub fn minimum_time_left_for_empty_block_default() -> Duration {
-    Duration::from_millis(3000)
-}
-
 pub fn scilla_address_default() -> String {
     String::from("http://localhost:3000")
 }

zilliqa/src/consensus.rs

@@ -23,6 +23,7 @@ use crate::{
     block_store::BlockStore,
     blockhooks,
     cfg::{ConsensusConfig, NodeConfig},
+    constants::TIME_TO_ALLOW_PROPOSAL_BROADCAST,
     contracts,
     crypto::{verify_messages, BlsSignature, Hash, NodePublicKey, SecretKey},
     db::{self, Db},
@@ -170,7 +171,7 @@ pub struct Consensus {
     transaction_pool: TransactionPool,
     /// Pending proposal. Gets created as soon as we become aware that we are leader for this view.
     early_proposal: Option<EarlyProposal>,
-    /// Flag indicating that block creation should be postponed at least until empty_block_timeout is reached
+    /// Flag indicating that block broadcasting should be postponed at least until block_time is reached
     create_next_block_on_timeout: bool,
     /// Timestamp of most recent view change
     view_updated_at: SystemTime,
@@ -501,28 +502,21 @@ impl Consensus {
         }
 
         let (
-            time_since_last_view_change,
+            milliseconds_since_last_view_change,
+            milliseconds_remaining_of_block_time,
             exponential_backoff_timeout,
-            minimum_time_left_for_empty_block,
         ) = self.get_consensus_timeout_params()?;
-
         trace!(
+            milliseconds_since_last_view_change,
+            exponential_backoff_timeout,
+            milliseconds_remaining_of_block_time,
             "timeout reached create_next_block_on_timeout: {}",
             self.create_next_block_on_timeout
         );
-        if self.create_next_block_on_timeout {
-            let empty_block_timeout_ms =
-                self.config.consensus.empty_block_timeout.as_millis() as u64;
 
-            let has_txns_for_next_block = self.transaction_pool.has_txn_ready();
-
-            // Check if enough time elapsed or there's something in mempool or we don't have enough
-            // time but let's try at least until new view can happen
-            if time_since_last_view_change > empty_block_timeout_ms
-                || has_txns_for_next_block
-                || (time_since_last_view_change + minimum_time_left_for_empty_block
-                    >= exponential_backoff_timeout)
-            {
+        if self.create_next_block_on_timeout {
+            // Check if enough time elapsed to propose block
+            if milliseconds_remaining_of_block_time == 0 {
                 if let Ok(Some((block, transactions))) = self.propose_new_block() {
                     self.create_next_block_on_timeout = false;
                     return Ok(Some((
@@ -531,31 +525,26 @@ impl Consensus {
                     )));
                 };
             } else {
-                self.reset_timeout.send(
-                    self.config
-                        .consensus
-                        .empty_block_timeout
-                        .saturating_sub(Duration::from_millis(time_since_last_view_change)),
-                )?;
+                self.reset_timeout
+                    .send(Duration::from_millis(milliseconds_remaining_of_block_time))?;
                 return Ok(None);
             }
         }
 
-        // Now consider whether we want to timeout - the timeout duration doubles every time, so it
+        // If we are not leader then consider whether we want to timeout - the timeout duration doubles every time, so it
         // Should eventually have all nodes on the same view
-
-        if time_since_last_view_change < exponential_backoff_timeout {
+        if milliseconds_since_last_view_change < exponential_backoff_timeout {
             trace!(
                 "Not proceeding with view change. Current view: {} - time since last: {}, timeout requires: {}",
                 view,
-                time_since_last_view_change,
+                milliseconds_since_last_view_change,
                 exponential_backoff_timeout
             );
 
             // Resend NewView message for this view if timeout period is a multiple of consensus_timeout
-            if (time_since_last_view_change
+            if (milliseconds_since_last_view_change
                 > self.config.consensus.consensus_timeout.as_millis() as u64)
-                && (Duration::from_millis(time_since_last_view_change).as_secs()
+                && (Duration::from_millis(milliseconds_since_last_view_change).as_secs()
                     % self.config.consensus.consensus_timeout.as_secs())
                     == 0
             {
@@ -575,7 +564,7 @@ impl Consensus {
             return Ok(None);
         }
 
-        trace!("Considering view change: view: {} time since: {} timeout: {} last known view: {} last hash: {}", view, time_since_last_view_change, exponential_backoff_timeout, self.high_qc.view, self.head_block().hash());
+        trace!("Considering view change: view: {} time since: {} timeout: {} last known view: {} last hash: {}", view, milliseconds_since_last_view_change, exponential_backoff_timeout, self.high_qc.view, self.head_block().hash());
 
         let block = self.get_block(&self.high_qc.block_hash)?.ok_or_else(|| {
             anyhow!("missing block corresponding to our high qc - this should never happen")
@@ -610,29 +599,27 @@ impl Consensus {
         Ok(Some(new_view))
     }
 
+    /// All values returned in milliseconds
     pub fn get_consensus_timeout_params(&self) -> Result<(u64, u64, u64)> {
-        let time_since_last_view_change = SystemTime::now()
+        let milliseconds_since_last_view_change = SystemTime::now()
             .duration_since(self.view_updated_at)
-            .unwrap_or_default()
-            .as_millis() as u64;
-        let exponential_backoff_timeout = self.exponential_backoff_timeout(self.get_view()?);
-
-        let minimum_time_left_for_empty_block = self
+            .unwrap_or_default();
+        let mut milliseconds_remaining_of_block_time = self
             .config
             .consensus
-            .minimum_time_left_for_empty_block
-            .as_millis() as u64;
+            .block_time
+            .saturating_sub(milliseconds_since_last_view_change);
 
-        trace!(
-            time_since_last_view_change,
-            exponential_backoff_timeout,
-            minimum_time_left_for_empty_block,
-        );
+        // In order to maintain close to 1 second block times we broadcast 1-TIME_TO_ALLOW_PROPOSAL_BROADCAST seconds after the previous block to allow for network messages and block processing
+        if self.config.consensus.block_time > TIME_TO_ALLOW_PROPOSAL_BROADCAST {
+            milliseconds_remaining_of_block_time = milliseconds_remaining_of_block_time
+                .saturating_sub(TIME_TO_ALLOW_PROPOSAL_BROADCAST);
+        }
 
         Ok((
-            time_since_last_view_change,
-            exponential_backoff_timeout,
-            minimum_time_left_for_empty_block,
+            milliseconds_since_last_view_change.as_millis() as u64,
+            milliseconds_remaining_of_block_time.as_millis() as u64,
+            self.exponential_backoff_timeout(self.get_view()?),
         ))
     }
 
@@ -1409,24 +1396,14 @@ impl Consensus {
         // Assemble new block with whatever is in the mempool
         while let Some(tx) = self.transaction_pool.best_transaction(&state)? {
             // First - check if we have time left to process txns and give enough time for block propagation
-            let (
-                time_since_last_view_change,
-                exponential_backoff_timeout,
-                minimum_time_left_for_empty_block,
-            ) = self.get_consensus_timeout_params()?;
-
-            if time_since_last_view_change + minimum_time_left_for_empty_block
-                >= exponential_backoff_timeout
-            {
+            let (_, milliseconds_remaining_of_block_time, _) =
+                self.get_consensus_timeout_params()?;
+
+            if milliseconds_remaining_of_block_time == 0 {
                 debug!(
-                    time_since_last_view_change,
-                    exponential_backoff_timeout,
-                    minimum_time_left_for_empty_block,
-                    "timeout proposal {} for view {}",
+                    "stopped adding txs to block {} because block time is reached",
                     proposal.header.number,
-                    proposal.header.view,
                 );
-                // don't have time
                 break;
             }
 
@@ -1529,26 +1506,21 @@ impl Consensus {
     /// Either propose now or set timeout to allow for txs to come in.
     fn ready_for_block_proposal(&mut self) -> Result<Option<(Block, Vec<VerifiedTransaction>)>> {
         // Check if there's enough time to wait on a timeout and then propagate an empty block in the network before other participants trigger NewView
-        let (
-            time_since_last_view_change,
-            exponential_backoff_timeout,
-            minimum_time_left_for_empty_block,
-        ) = self.get_consensus_timeout_params()?;
+        let (milliseconds_since_last_view_change, milliseconds_remaining_of_block_time, _) =
+            self.get_consensus_timeout_params()?;
 
-        if time_since_last_view_change + minimum_time_left_for_empty_block
-            >= exponential_backoff_timeout
-        {
+        if milliseconds_remaining_of_block_time == 0 {
             return self.propose_new_block();
         }
 
-        // Reset the timeout and wake up again once it has been at least `empty_block_timeout` since
+        // Reset the timeout and wake up again once it has been at least `block_time` since
         // the last view change. At this point we should be ready to produce a new block.
         self.create_next_block_on_timeout = true;
         self.reset_timeout.send(
             self.config
                 .consensus
-                .empty_block_timeout
-                .saturating_sub(Duration::from_millis(time_since_last_view_change)),
+                .block_time
+                .saturating_sub(Duration::from_millis(milliseconds_since_last_view_change)),
         )?;
         trace!(
             "will propose new proposal on timeout for view {}",
@@ -1607,15 +1579,10 @@ impl Consensus {
 
         for txn in pending.into_iter() {
             // First - check for time
-            let (
-                time_since_last_view_change,
-                exponential_backoff_timeout,
-                minimum_time_left_for_empty_block,
-            ) = self.get_consensus_timeout_params()?;
-
-            if time_since_last_view_change + minimum_time_left_for_empty_block
-                >= exponential_backoff_timeout
-            {
+            let (_, milliseconds_remaining_of_block_time, _) =
+                self.get_consensus_timeout_params()?;
+
+            if milliseconds_remaining_of_block_time == 0 {
                 break;
             }
 

zilliqa/src/constants.rs

@@ -68,3 +68,7 @@ pub const EXAMINE_BLOCKS_PER_FORK_COUNT: usize = 16;
 pub const SCILLA_TRANSFER: ScillaGas = ScillaGas(50);
 pub const SCILLA_INVOKE_CHECKER: ScillaGas = ScillaGas(100);
 pub const SCILLA_INVOKE_RUNNER: ScillaGas = ScillaGas(300);
+
+// Consensus
+// Roughly how long to allow between finish propocessing of a Proposal and it being received by peers
+pub const TIME_TO_ALLOW_PROPOSAL_BROADCAST: Duration = Duration::from_millis(100);

zilliqa/tests/it/main.rs

@@ -67,8 +67,7 @@ use zilliqa::{
     cfg::{
         allowed_timestamp_skew_default, block_request_batch_size_default,
         block_request_limit_default, eth_chain_id_default, failed_request_sleep_duration_default,
-        max_blocks_in_flight_default, max_rpc_response_size_default,
-        minimum_time_left_for_empty_block_default, scilla_address_default,
+        max_blocks_in_flight_default, max_rpc_response_size_default, scilla_address_default,
         scilla_ext_libs_path_default, scilla_stdlib_dir_default, state_cache_size_default,
         state_rpc_limit_default, total_native_token_supply_default, Amount, ApiServer, Checkpoint,
         ConsensusConfig, ContractUpgradesBlockHeights, Forks, GenesisDeposit, NodeConfig,
@@ -331,10 +330,9 @@ impl Network {
                 genesis_deposits: genesis_deposits.clone(),
                 is_main: send_to_parent.is_none(),
                 consensus_timeout: Duration::from_secs(5),
-                minimum_time_left_for_empty_block: minimum_time_left_for_empty_block_default(),
                 // Give a genesis account 1 billion ZIL.
                 genesis_accounts: Self::genesis_accounts(&genesis_key),
-                empty_block_timeout: Duration::from_millis(25),
+                block_time: Duration::from_millis(25),
                 scilla_address: scilla_address.clone(),
                 scilla_stdlib_dir: scilla_stdlib_dir.clone(),
                 scilla_ext_libs_path: scilla_ext_libs_path_default(),
@@ -473,15 +471,14 @@ impl Network {
                 is_main: self.is_main(),
                 consensus_timeout: Duration::from_secs(5),
                 genesis_accounts: Self::genesis_accounts(&self.genesis_key),
-                empty_block_timeout: Duration::from_millis(25),
+                block_time: Duration::from_millis(25),
                 local_address: "host.docker.internal".to_owned(),
                 rewards_per_hour: 204_000_000_000_000_000_000_000u128.into(),
                 blocks_per_hour: 3600 * 40,
                 minimum_stake: 32_000_000_000_000_000_000u128.into(),
                 eth_block_gas_limit: EvmGas(84000000),
                 gas_price: 4_761_904_800_000u128.into(),
                 main_shard_id: None,
-                minimum_time_left_for_empty_block: minimum_time_left_for_empty_block_default(),
                 scilla_address: scilla_address_default(),
                 blocks_per_epoch: self.blocks_per_epoch,
                 epochs_per_checkpoint: 1,

zq2-devnet.yaml

@@ -8,6 +8,6 @@ roles:
 - checkpoint
 - persistence
 versions:
-  zq2: v0.4.0
+  zq2: v0.5.1
   otterscan: develop
   spout: main