Merge pull request #133 from SurfingNerd/i132-invalid-public-keys

I132 invalid public keys

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

@@ -179,6 +179,13 @@ pub struct KeyPairWrapper {
     pub inner: Arc<RwLock<Option<Box<dyn EngineSigner>>>>,
 }
 
+impl PublicWrapper {
+    /// Check if the public key is valid.
+    pub fn is_valid(&self) -> bool {
+        self.encrypt(b"a", &mut rand::thread_rng()).is_ok()
+    }
+}
+
 impl<'a> PublicKey for PublicWrapper {
     type Error = crypto::publickey::Error;
     type SecretKey = KeyPairWrapper;

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

@@ -1009,6 +1009,7 @@ impl HoneyBadgerBFT {
             Some(client_arc) => {
                 if self.is_syncing(&client_arc) {
                     // we are syncing - do not do anything.
+                    trace!(target: "engine", "do_validator_engine_actions: skipping because we are syncing.");
                     return Ok(());
                 }
 

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

@@ -16,12 +16,14 @@ use engines::{
     },
     signer::EngineSigner,
 };
-use ethereum_types::{Address, U256};
+use ethereum_types::{Address, Public, U256};
 use itertools::Itertools;
 use parking_lot::RwLock;
 use std::{collections::BTreeMap, sync::Arc};
 use types::ids::BlockId;
 
+use crate::client::BlockChainClient;
+
 pub struct KeygenTransactionSender {
     last_keygen_mode: KeyGenMode,
     keygen_mode_counter: u64,
@@ -36,6 +38,21 @@ enum ShouldSendKeyAnswer {
     Yes,
 }
 
+#[derive(Debug)]
+pub enum KeyGenError {
+    NoSigner,
+    NoFullClient,
+    NoPartToWrite,
+    CallError(CallError),
+    Unexpected,
+}
+
+impl From<CallError> for KeyGenError {
+    fn from(e: CallError) -> Self {
+        KeyGenError::CallError(e)
+    }
+}
+
 static KEYGEN_TRANSACTION_SEND_DELAY: u64 = 3;
 static KEYGEN_TRANSACTION_RESEND_DELAY: u64 = 10;
 
@@ -102,17 +119,17 @@ impl KeygenTransactionSender {
         &mut self,
         client: &dyn EngineClient,
         signer: &Arc<RwLock<Option<Box<dyn EngineSigner>>>>,
-    ) -> Result<(), CallError> {
+    ) -> Result<(), KeyGenError> {
         // If we have no signer there is nothing for us to send.
         let address = match signer.read().as_ref() {
             Some(signer) => signer.address(),
             None => {
                 warn!(target: "engine", "Could not send keygen transactions, because signer module could not be retrieved");
-                return Err(CallError::ReturnValueInvalid);
+                return Err(KeyGenError::NoSigner);
             }
         };
 
-        let full_client = client.as_full_client().ok_or(CallError::NotFullClient)?;
+        let full_client = client.as_full_client().ok_or(KeyGenError::NoFullClient)?;
 
         // If the chain is still syncing, do not send Parts or Acks.
         if full_client.is_major_syncing() {
@@ -122,30 +139,82 @@ impl KeygenTransactionSender {
 
         trace!(target:"engine", " get_validator_pubkeys...");
 
-        let vmap = get_validator_pubkeys(&*client, BlockId::Latest, ValidatorType::Pending)?;
+        let vmap = get_validator_pubkeys(&*client, BlockId::Latest, ValidatorType::Pending)
+            .map_err(|e| KeyGenError::CallError(e))?;
         let pub_keys: BTreeMap<_, _> = vmap
             .values()
             .map(|p| (*p, PublicWrapper { inner: p.clone() }))
             .collect();
 
+        let pub_keys_arc = Arc::new(pub_keys);
+        let upcoming_epoch =
+            get_posdao_epoch(client, BlockId::Latest).map_err(|e| KeyGenError::CallError(e))? + 1;
+
+        //let pub_key_len = pub_keys.len();
         // if synckeygen creation fails then either signer or validator pub keys are problematic.
         // Todo: We should expect up to f clients to write invalid pub keys. Report and re-start pending validator set selection.
-        let (mut synckeygen, part) = engine_signer_to_synckeygen(signer, Arc::new(pub_keys))
-            .map_err(|e| {
-                warn!(target:"engine", "engine_signer_to_synckeygen error {:?}", e);
-                CallError::ReturnValueInvalid
-            })?;
+        let (mut synckeygen, part) = match engine_signer_to_synckeygen(signer, pub_keys_arc.clone())
+        {
+            Ok((mut synckeygen_, part_)) => (synckeygen_, part_),
+            Err(e) => {
+                warn!(target:"engine", "engine_signer_to_synckeygen pub keys count {:?} error {:?}", pub_keys_arc.len(), e);
+                //let mut failure_pub_keys: Vec<Public> = Vec::new();
+                let mut failure_pub_keys: Vec<u8> = Vec::new();
+                pub_keys_arc.iter().for_each(|(k, v)| {
+                    warn!(target:"engine", "pub key {}", k.as_bytes().iter().join(""));
+
+                    if !v.is_valid() {
+                        warn!(target:"engine", "INVALID pub key {}", k);
+
+                        // append the bytes of the public key to the failure_pub_keys.
+                        k.as_bytes().iter().for_each(|b| {
+                            failure_pub_keys.push(*b);
+                        });
+                    }
+                });
+
+                // if we should send our parts, we will send the public keys of the troublemakers instead.
+
+                match self
+                    .should_send_part(client, &address)
+                    .map_err(|e| KeyGenError::CallError(e))?
+                {
+                    ShouldSendKeyAnswer::NoNotThisKeyGenMode => {
+                        return Err(KeyGenError::Unexpected)
+                    }
+                    ShouldSendKeyAnswer::NoWaiting => return Err(KeyGenError::Unexpected),
+                    ShouldSendKeyAnswer::Yes => {
+                        let serialized_part = match bincode::serialize(&failure_pub_keys) {
+                            Ok(part) => part,
+                            Err(e) => {
+                                warn!(target:"engine", "could not serialize part: {:?}", e);
+                                return Err(KeyGenError::Unexpected);
+                            }
+                        };
+
+                        send_part_transaction(
+                            full_client,
+                            client,
+                            &address,
+                            upcoming_epoch,
+                            serialized_part,
+                        )?;
+                        trace!(target:"engine", "PART Transaction send for moving forward key gen phase");
+                        return Ok(());
+                    }
+                }
+            }
+        };
 
         // If there is no part then we are not part of the pending validator set and there is nothing for us to do.
         let part_data = match part {
             Some(part) => part,
             None => {
                 warn!(target:"engine", "no part to write.");
-                return Err(CallError::ReturnValueInvalid);
+                return Err(KeyGenError::NoPartToWrite);
             }
         };
 
-        let upcoming_epoch = get_posdao_epoch(client, BlockId::Latest)? + 1;
         trace!(target:"engine", "preparing to send PARTS for upcoming epoch: {}", upcoming_epoch);
 
         // Check if we already sent our part.
@@ -155,36 +224,17 @@ impl KeygenTransactionSender {
                     Ok(part) => part,
                     Err(e) => {
                         warn!(target:"engine", "could not serialize part: {:?}", e);
-                        return Err(CallError::ReturnValueInvalid);
+                        return Err(KeyGenError::Unexpected);
                     }
                 };
-                let serialized_part_len = serialized_part.len();
-                let current_round = get_current_key_gen_round(client)?;
-                let write_part_data = key_history_contract::functions::write_part::call(
+
+                send_part_transaction(
+                    full_client,
+                    client,
+                    &address,
                     upcoming_epoch,
-                    current_round,
                     serialized_part,
-                );
-
-                // the required gas values have been approximated by
-                // experimenting and it's a very rough estimation.
-                // it can be further fine tuned to be just above the real consumption.
-                // ACKs require much more gas,
-                // and usually run into the gas limit problems.
-                let gas: usize = serialized_part_len * 800 + 100_000;
-
-                let part_transaction =
-                    TransactionRequest::call(*KEYGEN_HISTORY_ADDRESS, write_part_data.0)
-                        .gas(U256::from(gas))
-                        .nonce(full_client.nonce(&address, BlockId::Latest).unwrap())
-                        .gas_price(U256::from(10000000000u64));
-                full_client
-                    .transact_silently(part_transaction)
-                    .map_err(|e| {
-                        warn!(target:"engine", "could not transact_silently: {:?}", e);
-                        CallError::ReturnValueInvalid
-                    })?;
-
+                )?;
                 trace!(target:"engine", "PART Transaction send.");
                 return Ok(());
             }
@@ -213,7 +263,7 @@ impl KeygenTransactionSender {
 					}
 					Err(err) => {
 						error!(target:"engine", "could not retrieve part for {} call failed. Error: {:?}", *v, err);
-						return Err(err);
+						return Err(KeyGenError::CallError(err));
 					}
 				}
             );
@@ -230,7 +280,7 @@ impl KeygenTransactionSender {
                 for ack in acks {
                     let ack_to_push = match bincode::serialize(&ack) {
                         Ok(serialized_ack) => serialized_ack,
-                        Err(_) => return Err(CallError::ReturnValueInvalid),
+                        Err(e) => return Err(KeyGenError::Unexpected),
                     };
                     total_bytes_for_acks += ack_to_push.len();
                     serialized_acks.push(ack_to_push);
@@ -263,3 +313,36 @@ impl KeygenTransactionSender {
         Ok(())
     }
 }
+
+fn send_part_transaction(
+    full_client: &dyn BlockChainClient,
+    client: &dyn EngineClient,
+    mining_address: &Address,
+    upcoming_epoch: U256,
+    data: Vec<u8>,
+) -> Result<(), KeyGenError> {
+    // the required gas values have been approximated by
+    // experimenting and it's a very rough estimation.
+    // it can be further fine tuned to be just above the real consumption.
+    // ACKs require much more gas,
+    // and usually run into the gas limit problems.
+    let gas: usize = data.len() * 800 + 100_000;
+
+    let nonce = full_client.nonce(&mining_address, BlockId::Latest).unwrap();
+    let current_round = get_current_key_gen_round(client)?;
+    let write_part_data =
+        key_history_contract::functions::write_part::call(upcoming_epoch, current_round, data);
+
+    let part_transaction = TransactionRequest::call(*KEYGEN_HISTORY_ADDRESS, write_part_data.0)
+        .gas(U256::from(gas))
+        .nonce(nonce)
+        .gas_price(U256::from(10000000000u64));
+    full_client
+        .transact_silently(part_transaction)
+        .map_err(|e| {
+            warn!(target:"engine", "could not transact_silently: {:?}", e);
+            CallError::ReturnValueInvalid
+        })?;
+
+    return Ok(());
+}