fix(validations): apply pr comments

data_structures/src/chain.rs

@@ -175,11 +175,6 @@ pub enum BlockError {
         commits, rf
     )]
     MismatchingCommitsNumber { commits: u32, rf: u32 },
-    #[fail(
-        display = "Reveals in block ({}) are not equal to reveals required ({})",
-        reveals, rf
-    )]
-    MismatchingRevealsNumber { reveals: u32, rf: u32 },
 }
 
 /// Struct that keeps a block candidate and its modifications in the blockchain
@@ -479,6 +474,11 @@ pub enum TransactionError {
     InvalidFee { fee: u64, expected_fee: u64 },
     #[fail(display = "Invalid Data Request reward: {}", reward)]
     InvalidDataRequestReward { reward: i64 },
+    #[fail(
+        display = "Invalid Data Request reward ({}) for this number of witnesses ({})",
+        dr_value, witnesses
+    )]
+    InvalidDataRequestValue { dr_value: i64, witnesses: i64 },
     #[fail(display = "Data Request witnesses number is not enough")]
     InsufficientWitnesses,
     #[fail(display = "Reveals from different Data Requests")]

data_structures/src/proto/mod.rs

@@ -25,7 +25,7 @@ pub trait ProtobufConvert: Sized {
         Self::ProtoStruct: Message,
     {
         // Serialize
-        self.to_pb().write_to_bytes().map_err(|e| e.into())
+        self.to_pb().write_to_bytes().map_err(Into::into)
     }
 
     /// Bytes -> ProtoStruct -> Struct
@@ -226,7 +226,7 @@ where
 {
     type ProtoStruct = Vec<T::ProtoStruct>;
     fn to_pb(&self) -> Self::ProtoStruct {
-        self.iter().map(|v| v.to_pb()).collect()
+        self.iter().map(ProtobufConvert::to_pb).collect()
     }
     fn from_pb(pb: Self::ProtoStruct) -> Result<Self, Error> {
         pb.into_iter()

node/src/actors/chain_manager/handlers.rs

@@ -283,7 +283,6 @@ impl Handler<AddTransaction> for ChainManager {
                 &self.chain_state.unspent_outputs_pool,
                 &self.data_request_pool,
                 &mut HashMap::new(),
-                &mut HashMap::new(),
             ) {
                 Ok(_) => {
                     debug!("Transaction added successfully");

node/src/actors/chain_manager/mining.rs

@@ -346,7 +346,7 @@ fn build_block(
         let transaction_fee = match transaction_fee(&transaction.body, unspent_outputs_pool) {
             Ok(x) => x,
             Err(e) => {
-                debug!(
+                warn!(
                     "Error when calculating transaction fee for transaction: {}",
                     e
                 );

p2p/src/sessions/mod.rs

@@ -301,28 +301,28 @@ where
                 self.inbound_unconsolidated
                     .collection
                     .keys()
-                    .map(|k| k.to_string()),
+                    .map(ToString::to_string),
             )
             .chain(std::iter::once("Inbound Consolidated".to_string()))
             .chain(
                 self.inbound_consolidated
                     .collection
                     .keys()
-                    .map(|k| k.to_string()),
+                    .map(ToString::to_string),
             )
             .chain(std::iter::once("Outbound Unconsolidated".to_string()))
             .chain(
                 self.outbound_unconsolidated
                     .collection
                     .keys()
-                    .map(|k| k.to_string()),
+                    .map(ToString::to_string),
             )
             .chain(std::iter::once("Outbound Consolidated".to_string()))
             .chain(
                 self.outbound_consolidated
                     .collection
                     .keys()
-                    .map(|k| k.to_string()),
+                    .map(ToString::to_string),
             )
             .chain(std::iter::once(
                 "Outbound Consolidated Consensus".to_string(),
@@ -331,7 +331,7 @@ where
                 self.outbound_consolidated_consensus
                     .collection
                     .keys()
-                    .map(|k| k.to_string()),
+                    .map(ToString::to_string),
             )
             .collect()
     }

src/json_rpc_client.rs

@@ -182,7 +182,7 @@ fn start_client(config_path: Option<PathBuf>) -> Result<TcpStream, failure::Erro
     info!("Connecting to JSON-RPC server at {}", addr);
     let stream = TcpStream::connect(addr);
 
-    stream.map_err(|e| e.into())
+    stream.map_err(Into::into)
 }
 
 fn send_request<S: Read + Write>(stream: &mut S, request: &str) -> Result<String, io::Error> {

storage/src/backends/crypto.rs

@@ -46,7 +46,7 @@ impl<T: Storage> Storage for Backend<T> {
 
                     cipher::decrypt_aes_cbc(&secret, data, iv)
                         .map(Some)
-                        .map_err(|err| err.into())
+                        .map_err(Into::into)
                 }
                 None => Ok(None),
             })

validations/src/validations.rs

@@ -15,7 +15,7 @@ use witnet_data_structures::{
     serializers::decoders::{TryFrom, TryInto},
 };
 
-use log::debug;
+use log;
 
 use witnet_rad::{run_consensus, script::unpack_radon_script, types::RadonTypes};
 
@@ -30,27 +30,8 @@ pub fn transaction_inputs_sum(
     let mut total_value = 0;
 
     match transaction_tag(tx) {
-        // Commit exception to calculate inputs
         TransactionType::Commit => {
-            match &tx.inputs[0] {
-                Input::DataRequest(dr_input) => {
-                    // Get DataRequest information
-                    let dr_pointer = dr_input.output_pointer();
-                    let dr_output =
-                        pool.get(&dr_pointer)
-                            .ok_or(TransactionError::OutputNotFound {
-                                output: dr_pointer.clone(),
-                            })?;
-
-                    match dr_output {
-                        Output::DataRequest(dr_state) => {
-                            total_value = dr_state.value / u64::from(dr_state.witnesses);
-                        }
-                        _ => Err(TransactionError::InvalidCommitTransaction)?,
-                    }
-                }
-                _ => Err(TransactionError::InvalidCommitTransaction)?,
-            }
+            total_value = calculate_commit_input(&tx, pool)?;
         }
         _ => {
             for input in &tx.inputs {
@@ -68,6 +49,29 @@ pub fn transaction_inputs_sum(
     Ok(total_value)
 }
 
+fn calculate_commit_input(
+    tx: &TransactionBody,
+    pool: &UnspentOutputsPool,
+) -> Result<u64, failure::Error> {
+    match &tx.inputs[0] {
+        Input::DataRequest(dr_input) => {
+            // Get DataRequest information
+            let dr_pointer = dr_input.output_pointer();
+            let dr_output = pool
+                .get(&dr_pointer)
+                .ok_or(TransactionError::OutputNotFound {
+                    output: dr_pointer.clone(),
+                })?;
+
+            match dr_output {
+                Output::DataRequest(dr_state) => Ok(dr_state.value / u64::from(dr_state.witnesses)),
+                _ => Err(TransactionError::InvalidCommitTransaction)?,
+            }
+        }
+        _ => Err(TransactionError::InvalidCommitTransaction)?,
+    }
+}
+
 /// Calculate the sum of the values of the outputs of a transaction.
 pub fn transaction_outputs_sum(tx: &TransactionBody) -> u64 {
     tx.outputs.iter().map(Output::value).sum()
@@ -89,6 +93,7 @@ pub fn transaction_fee(
     if out_value > in_value {
         Err(TransactionError::NegativeFee)?
     } else {
+        log::error!("Calculated fee is {}", in_value - out_value);
         Ok(in_value - out_value)
     }
 
@@ -164,7 +169,7 @@ pub fn validate_consensus(
 
 /// Function to validate a value transfer transaction
 pub fn validate_vt_transaction(_tx: &TransactionBody) -> Result<(), failure::Error> {
-    // TODO(#514): Implement valu transfer validation
+    // TODO(#514): Implement value transfer transaction validation
     Ok(())
 }
 
@@ -185,6 +190,13 @@ pub fn validate_dr_transaction(tx: &TransactionBody) -> Result<(), failure::Erro
         let reveal_fee = dr_output.reveal_fee as i64;
         let tally_fee = dr_output.tally_fee as i64;
 
+        if ((dr_value - tally_fee) % witnesses) != 0 {
+            Err(TransactionError::InvalidDataRequestValue {
+                dr_value,
+                witnesses,
+            })?
+        }
+
         let witness_reward = ((dr_value - tally_fee) / witnesses) - commit_fee - reveal_fee;
         if witness_reward <= 0 {
             Err(TransactionError::InvalidDataRequestReward {
@@ -199,16 +211,25 @@ pub fn validate_dr_transaction(tx: &TransactionBody) -> Result<(), failure::Erro
 }
 
 // Add 1 in the number assigned to a DataRequestOutput
-pub fn update_count<S: ::std::hash::BuildHasher>(
+pub fn increment_witnesses_counter<S: ::std::hash::BuildHasher>(
     hm: &mut WitnessesCounter<S>,
     k: &OutputPointer,
     rf: u32,
 ) {
-    hm.entry(k.clone()).or_insert((0, rf)).0 += 1;
+    hm.entry(k.clone())
+        .or_insert(WitnessesCount {
+            current: 0,
+            target: rf,
+        })
+        .current += 1;
 }
 
 /// HashMap to count commit and reveals transactions need for a Data Request
-pub type WitnessesCounter<S> = HashMap<OutputPointer, (u32, u32), S>;
+pub struct WitnessesCount {
+    current: u32,
+    target: u32,
+}
+pub type WitnessesCounter<S> = HashMap<OutputPointer, WitnessesCount, S>;
 
 /// Function to validate a commit transaction
 pub fn validate_commit_transaction<S: ::std::hash::BuildHasher>(
@@ -247,7 +268,7 @@ pub fn validate_commit_transaction<S: ::std::hash::BuildHasher>(
             }
 
             // Accumulate commits number
-            update_count(
+            increment_witnesses_counter(
                 block_commits,
                 &dr_pointer,
                 u32::from(dr_state.data_request.witnesses),
@@ -260,10 +281,9 @@ pub fn validate_commit_transaction<S: ::std::hash::BuildHasher>(
 }
 
 /// Function to validate a reveal transaction
-pub fn validate_reveal_transaction<S: ::std::hash::BuildHasher>(
+pub fn validate_reveal_transaction(
     tx: &TransactionBody,
     dr_pool: &DataRequestPool,
-    block_reveals: &mut WitnessesCounter<S>,
     fee: u64,
 ) -> Result<(), failure::Error> {
     if (tx.inputs.len() != 1) || (tx.outputs.len() != 1) {
@@ -296,13 +316,6 @@ pub fn validate_reveal_transaction<S: ::std::hash::BuildHasher>(
 
             // TODO: Validate commitment
 
-            // Accumulate commits number
-            update_count(
-                block_reveals,
-                &dr_pointer,
-                u32::from(dr_state.data_request.witnesses),
-            );
-
             Ok(())
         }
         _ => Err(TransactionError::NotCommitInputInReveal)?,
@@ -435,43 +448,42 @@ pub fn validate_transaction<S: ::std::hash::BuildHasher>(
     utxo_set: &UnspentOutputsPool,
     dr_pool: &DataRequestPool,
     block_commits: &mut WitnessesCounter<S>,
-    block_reveals: &mut WitnessesCounter<S>,
 ) -> Result<u64, failure::Error> {
     let transaction_body = &transaction.body;
 
     match transaction_tag(transaction_body) {
         TransactionType::Mint => Err(TransactionError::UnexpectedMint)?,
         TransactionType::InvalidType => Err(TransactionError::NotValidTransaction)?,
         TransactionType::ValueTransfer => {
-            debug!("ValueTransfer Transaction validation");
+            log::debug!("ValueTransfer Transaction validation");
             let fee = transaction_fee(transaction_body, utxo_set)?;
 
             validate_vt_transaction(transaction_body)?;
             Ok(fee)
         }
         TransactionType::DataRequest => {
-            debug!("DataRequest Transaction validation");
+            log::debug!("DataRequest Transaction validation");
             let fee = transaction_fee(transaction_body, utxo_set)?;
 
             validate_dr_transaction(transaction_body)?;
             Ok(fee)
         }
         TransactionType::Commit => {
-            debug!("DataRequest Transaction validation");
+            log::debug!("Commit Transaction validation");
             let fee = transaction_fee(transaction_body, utxo_set)?;
 
             validate_commit_transaction(transaction_body, dr_pool, block_commits, fee)?;
             Ok(fee)
         }
         TransactionType::Reveal => {
-            debug!("DataRequest Transaction validation");
+            log::debug!("Reveal Transaction validation");
             let fee = transaction_fee(transaction_body, utxo_set)?;
 
-            validate_reveal_transaction(transaction_body, dr_pool, block_reveals, fee)?;
+            validate_reveal_transaction(transaction_body, dr_pool, fee)?;
             Ok(fee)
         }
         TransactionType::Tally => {
-            debug!("DataRequest Transaction validation");
+            log::debug!("Tally Transaction validation");
             let fee = transaction_fee(transaction_body, utxo_set)?;
 
             validate_tally_transaction(transaction_body, dr_pool, utxo_set, fee)?;
@@ -487,12 +499,10 @@ pub fn validate_transactions(
     data_request_pool: &DataRequestPool,
     block: &Block,
 ) -> Result<BlockInChain, failure::Error> {
-    let transactions = block.txns.clone();
-
     // Init Progressive merkle tree
     let mut mt = ProgressiveMerkleTree::sha256();
 
-    match transactions.get(0).map(|tx| {
+    match block.txns.get(0).map(|tx| {
         let Hash::SHA256(sha) = tx.hash();
         mt.push(Sha256(sha));
         transaction_tag(&tx.body)
@@ -506,19 +516,17 @@ pub fn validate_transactions(
     let mut remove_later = vec![];
 
     let mut commits_number: WitnessesCounter<_> = HashMap::new();
-    let mut reveals_number: WitnessesCounter<_> = HashMap::new();
 
     // Init total fee
     let mut total_fee = 0;
 
     // TODO: replace for loop with a try_fold
-    for transaction in &transactions[1..] {
+    for transaction in &block.txns[1..] {
         match validate_transaction(
             &transaction,
             &utxo_set,
             &data_request_pool,
             &mut commits_number,
-            &mut reveals_number,
         ) {
             Ok(fee) => {
                 // Add transaction fee
@@ -583,21 +591,11 @@ pub fn validate_transactions(
     utxo_set.insert(mint_output_pointer, mint_output);
 
     // Validate commits number
-    for (count, rf) in commits_number.values() {
-        if count != rf {
+    for WitnessesCount { current, target } in commits_number.values() {
+        if current != target {
             Err(BlockError::MismatchingCommitsNumber {
-                commits: *count,
-                rf: *rf,
-            })?
-        }
-    }
-
-    // Validate reveals number
-    for (count, rf) in reveals_number.values() {
-        if count != rf {
-            Err(BlockError::MismatchingRevealsNumber {
-                reveals: *count,
-                rf: *rf,
+                commits: *current,
+                rf: *target,
             })?
         }
     }