From a21c0a8830ab0bd3f85fff1bc496369344cdc9b6 Mon Sep 17 00:00:00 2001 From: tommytrg Date: Thu, 19 Oct 2023 11:01:02 +0200 Subject: [PATCH] PR comments --- Cargo.lock | 19 ++++-- data_structures/src/error.rs | 22 ++++--- data_structures/src/transaction.rs | 19 +----- data_structures/src/transaction_factory.rs | 3 + schemas/witnet/witnet.proto | 2 +- validations/Cargo.toml | 2 +- validations/src/tests/mod.rs | 14 ++--- validations/src/validations.rs | 68 +++++++++------------- 8 files changed, 67 insertions(+), 82 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2f0d09be9..93ea7786f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1707,6 +1707,15 @@ dependencies = [ "either", ] +[[package]] +name = "itertools" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b1c173a5686ce8bfa551b3563d0c2170bf24ca44da99c7ca4bfdab5418c3fe57" +dependencies = [ + "either", +] + [[package]] name = "itoa" version = "1.0.6" @@ -4967,7 +4976,7 @@ dependencies = [ "failure", "futures 0.3.28", "hex", - "itertools", + "itertools 0.8.2", "lazy_static", "log 0.4.19", "num-format", @@ -5083,7 +5092,7 @@ dependencies = [ "failure", "futures 0.3.28", "hex", - "itertools", + "itertools 0.8.2", "lazy_static", "log 0.4.19", "num-traits", @@ -5149,7 +5158,7 @@ dependencies = [ "futures-util", "glob", "hex", - "itertools", + "itertools 0.8.2", "jsonrpc-core 18.0.0", "jsonrpc-pubsub 18.0.0", "log 0.4.19", @@ -5272,7 +5281,7 @@ dependencies = [ "bencher", "failure", "hex", - "itertools", + "itertools 0.11.0", "log 0.4.19", "url", "witnet_config", @@ -5296,7 +5305,7 @@ dependencies = [ "futures 0.3.28", "futures-util", "hex", - "itertools", + "itertools 0.8.2", "jsonrpc-core 15.1.0", "jsonrpc-pubsub 15.1.0", "log 0.4.19", diff --git a/data_structures/src/error.rs b/data_structures/src/error.rs index a19457cd1..fca6ee194 100644 --- a/data_structures/src/error.rs +++ b/data_structures/src/error.rs @@ -278,14 +278,17 @@ pub enum TransactionError { max_weight: u32, dr_output: Box, }, - /// Stake weight limit exceeded + /// Stake amount below minimum #[fail( - display = "Stake ({}) doesn't reach the minimum amount ({})", + display = "The amount of coins in stake ({}) is less than the minimum allowed ({})", min_stake, stake )] - MinStakeNotReached { min_stake: u64, stake: u64 }, - /// An stake output with zero value does not make sense - #[fail(display = "Transaction {} has a zero value stake output", tx_hash)] + StakeBelowMinimum { min_stake: u64, stake: u64 }, + /// A stake output with zero value does not make sense + #[fail( + display = "Transaction {} contains a stake output with zero value", + tx_hash + )] ZeroValueStakeOutput { tx_hash: Hash }, #[fail( display = "The reward-to-collateral ratio for this data request is {}, but must be equal or less than {}", @@ -410,15 +413,18 @@ pub enum BlockError { weight, max_weight )] TotalDataRequestWeightLimitExceeded { weight: u32, max_weight: u32 }, - /// Stake weight limit exceeded + /// Stake weight limit exceeded by a block candidate #[fail( display = "Total weight of Stake Transactions in a block ({}) exceeds the limit ({})", weight, max_weight )] TotalStakeWeightLimitExceeded { weight: u32, max_weight: u32 }, /// Repeated operator Stake - #[fail(display = "A single operator is staking more than once: ({}) ", pkh)] - RepeatedStakeOperator { pkh: Hash }, + #[fail( + display = "A single operator is receiving stake more than once in a block: ({}) ", + pkh + )] + RepeatedStakeOperator { pkh: PublicKeyHash }, /// Missing expected tallies #[fail( display = "{} expected tally transactions are missing in block candidate {}", diff --git a/data_structures/src/transaction.rs b/data_structures/src/transaction.rs index d9974ffc4..5ac3054d1 100644 --- a/data_structures/src/transaction.rs +++ b/data_structures/src/transaction.rs @@ -697,6 +697,7 @@ pub struct StakeTransaction { pub body: StakeTransactionBody, pub signatures: Vec, } + impl StakeTransaction { // Creates a new stake transaction. pub fn new(body: StakeTransactionBody, signatures: Vec) -> Self { @@ -704,8 +705,8 @@ impl StakeTransaction { } /// Returns the weight of a stake transaction. - /// This is the weight that will be used to calculate - /// how many transactions can fit inside one block + /// This is the weight that will be used to calculate how many transactions can fit inside one + /// block pub fn weight(&self) -> u32 { self.body.weight() } @@ -724,20 +725,6 @@ pub struct StakeTransactionBody { } impl StakeTransactionBody { - /// Creates a new stake transaction body. - pub fn new( - inputs: Vec, - output: StakeOutput, - change: Option, - ) -> Self { - StakeTransactionBody { - inputs, - output, - change, - hash: MemoHash::new(), - } - } - /// Stake transaction weight. It is calculated as: /// /// ```text diff --git a/data_structures/src/transaction_factory.rs b/data_structures/src/transaction_factory.rs index 57687f3cb..9b31e444e 100644 --- a/data_structures/src/transaction_factory.rs +++ b/data_structures/src/transaction_factory.rs @@ -537,9 +537,12 @@ pub fn transaction_outputs_sum(outputs: &[ValueTransferOutput]) -> Result Result { + // TODO: add stake transaction factory logic here !unimplemented!() } + #[cfg(test)] mod tests { use std::{ diff --git a/schemas/witnet/witnet.proto b/schemas/witnet/witnet.proto index 11883f872..d08283acb 100644 --- a/schemas/witnet/witnet.proto +++ b/schemas/witnet/witnet.proto @@ -239,7 +239,7 @@ message StakeOutput { message StakeTransactionBody { repeated Input inputs = 1; StakeOutput output = 2; - ValueTransferOutput change = 3; + optional ValueTransferOutput change = 3; } message StakeTransaction { diff --git a/validations/Cargo.toml b/validations/Cargo.toml index eb00b2011..6cf51b7b3 100644 --- a/validations/Cargo.toml +++ b/validations/Cargo.toml @@ -8,7 +8,7 @@ workspace = ".." [dependencies] failure = "0.1.8" -itertools = "0.8.2" +itertools = "0.11.0" log = "0.4.8" url = "2.2.2" diff --git a/validations/src/tests/mod.rs b/validations/src/tests/mod.rs index 18eb94f54..c1e27ae9a 100644 --- a/validations/src/tests/mod.rs +++ b/validations/src/tests/mod.rs @@ -8457,20 +8457,14 @@ fn st_no_inputs() { let block_number = 0; let utxo_diff = UtxoDiff::new(&utxo_set, block_number); - // Try to create an stake tx with no inputs + // Try to create a stake tx with no inputs let st_output = StakeOutput { value: MIN_STAKE_NANOWITS + 1, authorization: KeyedSignature::default(), }; - // let vto0 = ValueTransferOutput { - // pkh, - // value: 1000, - // time_lock: 0, - // }; + let st_body = StakeTransactionBody::new(Vec::new(), st_output, None); let st_tx = StakeTransaction::new(st_body, vec![]); - // let vt_body = VTTransactionBody::new(vec![], vec![vto0]); - // let vt_tx = VTTransaction::new(vt_body, vec![]); let x = validate_stake_transaction( &st_tx, &utxo_diff, @@ -8523,7 +8517,7 @@ fn st_one_input_but_no_signature() { } #[test] -fn st_min_stake_not_reach() { +fn st_below_min_stake() { let mut signatures_to_verify = vec![]; let utxo_set = UnspentOutputsPool::default(); let block_number = 0; @@ -8551,7 +8545,7 @@ fn st_min_stake_not_reach() { ); assert_eq!( x.unwrap_err().downcast::().unwrap(), - TransactionError::MinStakeNotReached { + TransactionError::StakeBelowMinimum { min_stake: MIN_STAKE_NANOWITS, stake: 1 } diff --git a/validations/src/validations.rs b/validations/src/validations.rs index fab04907f..1224bba14 100644 --- a/validations/src/validations.rs +++ b/validations/src/validations.rs @@ -101,10 +101,7 @@ pub fn dr_transaction_fee( /// Returns the fee of a stake transaction. /// -/// The fee is the difference between the output and the inputs -/// of the transaction. The pool parameter is used to find the -/// outputs pointed by the inputs and that contain the actual -/// their value. +/// The fee is the difference between the output and the inputs of the transaction. pub fn st_transaction_fee( st_tx: &StakeTransaction, utxo_diff: &UtxoDiff<'_>, @@ -112,9 +109,15 @@ pub fn st_transaction_fee( epoch_constants: EpochConstants, ) -> Result { let in_value = transaction_inputs_sum(&st_tx.body.inputs, utxo_diff, epoch, epoch_constants)?; - let out_value = &st_tx.body.output.value; + let out_value = &st_tx.body.output.value + - &st_tx + .body + .change + .clone() + .unwrap_or(Default::default()) + .value; - if out_value > &in_value { + if out_value > in_value { Err(TransactionError::NegativeFee.into()) } else { Ok(in_value - out_value) @@ -1113,7 +1116,7 @@ pub fn validate_tally_transaction<'a>( Ok((ta_tx.outputs.iter().collect(), tally_extra_fee)) } -/// Function to validate a stake transaction +/// Function to validate a stake transaction. pub fn validate_stake_transaction<'a>( st_tx: &'a StakeTransaction, utxo_diff: &UtxoDiff<'_>, @@ -1130,9 +1133,9 @@ pub fn validate_stake_transaction<'a>( ), failure::Error, > { - // Check that the stake is greater than the min allowed + // Check that the amount of coins to stake is equal or greater than the minimum allowed if st_tx.body.output.value < MIN_STAKE_NANOWITS { - return Err(TransactionError::MinStakeNotReached { + return Err(TransactionError::StakeBelowMinimum { min_stake: MIN_STAKE_NANOWITS, stake: st_tx.body.output.value, } @@ -1759,28 +1762,20 @@ pub fn validate_block_transactions( let mut st_weight: u32 = 0; // Check if the block contains more than one stake tx from the same operator - for i in 1..block.txns.stake_txns.len() { - let found = block.txns.stake_txns[i..].iter().find(|stake_tx| { - let stake_tx_aux = &block.txns.stake_txns[i - 1]; - - stake_tx.body.output.authorization.public_key - == stake_tx_aux.body.output.authorization.public_key - }); + let duplicate = block + .txns + .stake_txns + .clone() + .into_iter() + .map(|stake_tx| stake_tx.body.output.authorization.public_key) + .duplicates() + .next(); - // TODO: refactor - if found.is_some() { - return Err(BlockError::RepeatedStakeOperator { - pkh: found - .unwrap() - .body - .output - .authorization - .public_key - .pkh() - .hash(), - } - .into()); + if duplicate.is_some() { + return Err(BlockError::RepeatedStakeOperator { + pkh: duplicate.unwrap().pkh(), } + .into()); } for transaction in &block.txns.stake_txns { @@ -1805,20 +1800,11 @@ pub fn validate_block_transactions( } st_weight = acc_weight; - // TODO: refactor - if change.is_some() { - let mut outputs: Vec<&ValueTransferOutput> = Vec::new(); - let val = change.clone().unwrap(); - outputs.push(&val); - update_utxo_diff(&mut utxo_diff, inputs, outputs, transaction.hash()); - } else { - update_utxo_diff(&mut utxo_diff, inputs, Vec::new(), transaction.hash()); - } + let outputs = change.into_iter().collect_vec(); + update_utxo_diff(&mut utxo_diff, inputs, outputs, transaction.hash()); // Add new hash to merkle tree - let txn_hash = transaction.hash(); - let Hash::SHA256(sha) = txn_hash; - st_mt.push(Sha256(sha)); + st_mt.push(transaction.hash().into()); // TODO: Move validations to a visitor // // Execute visitor