Implemented keygen transactions sending delays.

crates/ethcore/src/engines/hbbft/contracts/keygen_history.rs

@@ -86,16 +86,6 @@ pub fn synckeygen_to_network_info(
     ))
 }
 
-pub fn has_part_of_address_data(
-    client: &dyn EngineClient,
-    address: Address,
-) -> Result<bool, CallError> {
-    let c = BoundContract::bind(client, BlockId::Latest, *KEYGEN_HISTORY_ADDRESS);
-    let serialized_part = call_const_key_history!(c, parts, address)?;
-    //println!("Part for address {}: {:?}", address, serialized_part);
-    Ok(!serialized_part.is_empty())
-}
-
 pub fn part_of_address(
     client: &dyn EngineClient,
     address: Address,

crates/ethcore/src/engines/hbbft/contracts/validator_set.rs

@@ -79,6 +79,30 @@ pub fn is_pending_validator(
     call_const_validator!(c, is_pending_validator, staking_address.clone())
 }
 
+#[derive(PartialEq)]
+pub enum KeyGenMode {
+    WritePart,
+    WriteAck,
+    Other,
+}
+
+pub fn get_pending_validator_key_generation_mode(
+    client: &dyn EngineClient,
+    mining_address: &Address,
+) -> Result<KeyGenMode, CallError> {
+    let c = BoundContract::bind(client, BlockId::Latest, *VALIDATOR_SET_ADDRESS);
+    let key_gen_mode = call_const_validator!(
+        c,
+        get_pending_validator_key_generation_mode,
+        mining_address.clone()
+    )?;
+    Ok(match key_gen_mode.low_u64() {
+        1 => KeyGenMode::WritePart,
+        3 => KeyGenMode::WriteAck,
+        _ => KeyGenMode::Other,
+    })
+}
+
 pub fn get_validator_available_since(
     client: &dyn EngineClient,
     address: &Address,

crates/ethcore/src/engines/hbbft/keygen_transactions.rs

@@ -3,49 +3,85 @@ use engines::{
     hbbft::{
         contracts::{
             keygen_history::{
-                engine_signer_to_synckeygen, has_acks_of_address_data, has_part_of_address_data,
-                key_history_contract, part_of_address, PublicWrapper, KEYGEN_HISTORY_ADDRESS,
+                engine_signer_to_synckeygen, has_acks_of_address_data, key_history_contract,
+                part_of_address, PublicWrapper, KEYGEN_HISTORY_ADDRESS,
             },
             staking::get_posdao_epoch,
-            validator_set::{get_validator_pubkeys, ValidatorType},
+            validator_set::{
+                get_pending_validator_key_generation_mode, get_validator_pubkeys, KeyGenMode,
+                ValidatorType,
+            },
         },
         utils::bound_contract::CallError,
     },
     signer::EngineSigner,
 };
-use ethereum_types::U256;
+use ethereum_types::{Address, U256};
 use itertools::Itertools;
 use parking_lot::RwLock;
-use std::{
-    collections::BTreeMap,
-    sync::{
-        atomic::{AtomicU64, Ordering},
-        Arc,
-    },
-};
+use std::{collections::BTreeMap, sync::Arc};
 use types::ids::BlockId;
 
 pub struct KeygenTransactionSender {
-    last_part_sent: u64,
-    last_acks_sent: u64,
-    resend_delay: u64,
+    last_keygen_mode: KeyGenMode,
+    keygen_mode_counter: u64,
 }
 
+static KEYGEN_TRANSACTION_SEND_DELAY: u64 = 3;
+static KEYGEN_TRANSACTION_RESEND_DELAY: u64 = 10;
+
 impl KeygenTransactionSender {
     pub fn new() -> Self {
         KeygenTransactionSender {
-            last_part_sent: 0,
-            last_acks_sent: 0,
-            resend_delay: 10,
+            last_keygen_mode: KeyGenMode::Other,
+            keygen_mode_counter: 0,
+        }
+    }
+
+    fn should_send(
+        &mut self,
+        client: &dyn EngineClient,
+        mining_address: &Address,
+        mode_to_check: KeyGenMode,
+    ) -> Result<bool, CallError> {
+        let keygen_mode = get_pending_validator_key_generation_mode(client, mining_address)?;
+        if keygen_mode == mode_to_check {
+            if self.last_keygen_mode == mode_to_check {
+                self.keygen_mode_counter += 1;
+                if self.keygen_mode_counter == KEYGEN_TRANSACTION_SEND_DELAY {
+                    return Ok(true);
+                } else if self.keygen_mode_counter > KEYGEN_TRANSACTION_SEND_DELAY {
+                    // Part should have been sent already,
+                    // give the chain time to include the transaction before trying a re-send.
+                    if (self.keygen_mode_counter - KEYGEN_TRANSACTION_SEND_DELAY)
+                        % KEYGEN_TRANSACTION_RESEND_DELAY
+                        == 0
+                    {
+                        return Ok(true);
+                    }
+                }
+            } else {
+                self.last_keygen_mode = mode_to_check;
+                self.keygen_mode_counter = 1;
+            }
         }
+        Ok(false)
     }
 
-    fn part_threshold_reached(&self, block_number: u64) -> bool {
-        self.last_part_sent == 0 || block_number > (self.last_part_sent + self.resend_delay)
+    fn should_send_part(
+        &mut self,
+        client: &dyn EngineClient,
+        mining_address: &Address,
+    ) -> Result<bool, CallError> {
+        self.should_send(client, mining_address, KeyGenMode::WritePart)
     }
 
-    fn acks_threshold_reached(&self, block_number: u64) -> bool {
-        self.last_acks_sent == 0 || block_number > (self.last_acks_sent + self.resend_delay)
+    fn should_send_ack(
+        &mut self,
+        client: &dyn EngineClient,
+        mining_address: &Address,
+    ) -> Result<bool, CallError> {
+        self.should_send(client, mining_address, KeyGenMode::WriteAck)
     }
 
     /// Returns a collection of transactions the pending validator has to submit in order to
@@ -55,9 +91,6 @@ impl KeygenTransactionSender {
         client: &dyn EngineClient,
         signer: &Arc<RwLock<Option<Box<dyn EngineSigner>>>>,
     ) -> Result<(), CallError> {
-        static LAST_PART_SENT: AtomicU64 = AtomicU64::new(0);
-        static LAST_ACKS_SENT: AtomicU64 = AtomicU64::new(0);
-
         // If we have no signer there is nothing for us to send.
         let address = match signer.read().as_ref() {
             Some(signer) => signer.address(),
@@ -95,16 +128,10 @@ impl KeygenTransactionSender {
         };
 
         let upcoming_epoch = get_posdao_epoch(client, BlockId::Latest)? + 1;
-        trace!(target:"engine", "preparing to send PARTS for upcomming epoch: {}", upcoming_epoch);
-
-        let cur_block = client
-            .block_number(BlockId::Latest)
-            .ok_or(CallError::ReturnValueInvalid)?;
+        trace!(target:"engine", "preparing to send PARTS for upcoming epoch: {}", upcoming_epoch);
 
         // Check if we already sent our part.
-        if (LAST_PART_SENT.load(Ordering::SeqCst) + 10 < cur_block)
-            && !has_part_of_address_data(client, address)?
-        {
+        if self.should_send_part(client, &address)? {
             let serialized_part = match bincode::serialize(&part_data) {
                 Ok(part) => part,
                 Err(_) => return Err(CallError::ReturnValueInvalid),
@@ -130,7 +157,6 @@ impl KeygenTransactionSender {
             full_client
                 .transact_silently(part_transaction)
                 .map_err(|_| CallError::ReturnValueInvalid)?;
-            LAST_PART_SENT.store(cur_block, Ordering::SeqCst);
         }
 
         trace!(target:"engine", "checking for acks...");
@@ -159,9 +185,7 @@ impl KeygenTransactionSender {
         trace!(target:"engine", "has_acks_of_address_data: {:?}", has_acks_of_address_data(client, address));
 
         // Now we are sure all parts are ready, let's check if we sent our Acks.
-        if (LAST_ACKS_SENT.load(Ordering::SeqCst) + 10 < cur_block)
-            && !has_acks_of_address_data(client, address)?
-        {
+        if self.should_send_ack(client, &address)? {
             let mut serialized_acks = Vec::new();
             let mut total_bytes_for_acks = 0;
 
@@ -191,7 +215,6 @@ impl KeygenTransactionSender {
             full_client
                 .transact_silently(acks_transaction)
                 .map_err(|_| CallError::ReturnValueInvalid)?;
-            LAST_ACKS_SENT.store(cur_block, Ordering::SeqCst);
         }
 
         Ok(())

crates/ethcore/src/engines/hbbft/test/mod.rs

@@ -7,7 +7,7 @@ use super::{
         validator_set::{is_pending_validator, mining_by_staking_address},
     },
     contribution::unix_now_secs,
-    test::hbbft_test_client::{create_hbbft_client, create_hbbft_clients},
+    test::hbbft_test_client::{create_hbbft_client, create_hbbft_clients, HbbftTestClient},
 };
 use client::traits::BlockInfo;
 use crypto::publickey::{Generator, KeyPair, Random, Secret};
@@ -21,7 +21,7 @@ pub mod network_simulator;
 
 lazy_static! {
     static ref MASTER_OF_CEREMONIES_KEYPAIR: KeyPair = KeyPair::from_secret(
-        Secret::from_str("18f059a4d72d166a96c1edfb9803af258a07b5ec862a961b3a1d801f443a1762")
+        Secret::from_str("f3c0abb01d69b061c0415a6d2acef23e63cbed0c83afff18672338ba4d9b4b0e")
             .expect("Secret from hex string must succeed")
     )
     .expect("KeyPair generation from secret must succeed");
@@ -105,6 +105,12 @@ fn test_staking_account_creation() {
     );
 }
 
+fn skip_n_blocks(n: u64, hbbft_client: &mut HbbftTestClient, transactor: &KeyPair) {
+    for _ in 0..n {
+        hbbft_client.create_some_transaction(Some(&transactor));
+    }
+}
+
 #[test]
 fn test_epoch_transition() {
     // Create Master of Ceremonies
@@ -142,21 +148,23 @@ fn test_epoch_transition() {
         U256::from(0)
     );
 
-    // First the validator realizes it is in the next validator set and sends his part.
-    moc.create_some_transaction(Some(&transactor));
-
-    // The part will be included in the block triggered by this transaction, but not part of the global state yet,
-    // so it sends the transaction another time.
-    moc.create_some_transaction(Some(&transactor));
-
-    // Now the part is part of the global chain state, and we send our acks.
-    moc.create_some_transaction(Some(&transactor));
-
-    // The acks will be included in the block triggered by this transaction, but not part of the global state yet.
-    moc.create_some_transaction(Some(&transactor));
-
-    // Now the acks are part of the global block state, and the key generation is complete and the next epoch begins
-    moc.create_some_transaction(Some(&transactor));
+    // We analyze what happens at each block if the key generation transactions are sent with a 3 block delay,
+    // and why it takes exactly 11 Blocks to complete.
+    //
+    // On part writing the following steps happen (remember the transaction send call
+    // happens in "on_close", when the new block is not integrated into the chain state yet!):
+    // Block 1: Client realizes it has to write its part, sets the send delay counter to 1.
+    // Block 2: Client sets the send delay counter to 2
+    // Block 3: Client sets the send delay counter to 3 - sends its Part transaction
+    // Block 4: Part transaction is part of the new block (but not in the chain state yet in the on_close function)
+    // Block 5: Client realizes it has to write its Acks, sets the send delay counter to 1.
+    // Block 6: Client sets the send delay counter to 2
+    // Block 7: Client sets the send delay counter to 3 - sends its Acks transaction
+    // Block 8: Acks transaction is part of the new block (but not in the chain state yet in the on_close function)
+    //   Remember that the Block reward call is also done in the "on_close" function and only respects the state of the *previous* block.
+    //   No epoch transition can happen in Block 10 for that reason, even though all Parts and Acks are sent and part of a block at that point.
+    // Block 9: In the "on_close" function all Parts and Acks are now on the chain state and the block reward system call request the epoch change.
+    skip_n_blocks(9, &mut moc, &transactor);
 
     // At this point we should be in the new epoch.
     assert_eq!(