validator: properly handle gas limit checks

bin/darkfid/darkfid_config.toml

@@ -12,6 +12,9 @@ rpc_listen = "tcp://127.0.0.1:8340"
 # Blockchain network to use
 network = "testnet"
 
+# Garbage collection task transactions batch size
+txs_batch_size = 50
+
 # Localnet blockchain network configuration
 [network_config."localnet"]
 # Path to the blockchain database directory

bin/darkfid/src/main.rs

@@ -90,6 +90,10 @@ struct Args {
     /// Blockchain network to use
     network: String,
 
+    #[structopt(long, default_value = "50")]
+    /// Garbage collection task transactions batch size
+    txs_batch_size: usize,
+
     #[structopt(short, long)]
     /// Set log file to ouput into
     log: Option<String>,
@@ -176,6 +180,8 @@ pub struct Darkfid {
     validator: ValidatorPtr,
     /// Flag to specify node is a miner
     miner: bool,
+    /// Garbage collection task transactions batch size
+    txs_batch_size: usize,
     /// A map of various subscribers exporting live info from the blockchain
     subscribers: HashMap<&'static str, JsonSubscriber>,
     /// JSON-RPC connection tracker
@@ -189,6 +195,7 @@ impl Darkfid {
         p2p: P2pPtr,
         validator: ValidatorPtr,
         miner: bool,
+        txs_batch_size: usize,
         subscribers: HashMap<&'static str, JsonSubscriber>,
         rpc_client: Option<Mutex<MinerRpcCLient>>,
     ) -> Self {
@@ -196,6 +203,7 @@ impl Darkfid {
             p2p,
             validator,
             miner,
+            txs_batch_size,
             subscribers,
             rpc_connections: Mutex::new(HashSet::new()),
             rpc_client,
@@ -274,9 +282,15 @@ async fn realmain(args: Args, ex: Arc<smol::Executor<'static>>) -> Result<()> {
     };
 
     // Initialize node
-    let darkfid =
-        Darkfid::new(p2p.clone(), validator, blockchain_config.miner, subscribers, rpc_client)
-            .await;
+    let darkfid = Darkfid::new(
+        p2p.clone(),
+        validator,
+        blockchain_config.miner,
+        args.txs_batch_size,
+        subscribers,
+        rpc_client,
+    )
+    .await;
     let darkfid = Arc::new(darkfid);
     info!(target: "darkfid", "Node initialized successfully!");
 

bin/darkfid/src/task/garbage_collect.rs

@@ -18,11 +18,7 @@
 
 use std::sync::Arc;
 
-use darkfi::{
-    error::TxVerifyFailed,
-    validator::{consensus::TXS_CAP, verification::verify_transactions},
-    Error, Result,
-};
+use darkfi::{error::TxVerifyFailed, validator::verification::verify_transactions, Error, Result};
 use darkfi_sdk::crypto::MerkleTree;
 use log::{debug, error, info};
 
@@ -35,7 +31,7 @@ pub async fn garbage_collect_task(node: Arc<Darkfid>) -> Result<()> {
     // Grab all current unproposed transactions.  We verify them in batches,
     // to not load them all in memory.
     let (mut last_checked, mut txs) =
-        match node.validator.blockchain.transactions.get_after_pending(0, TXS_CAP) {
+        match node.validator.blockchain.transactions.get_after_pending(0, node.txs_batch_size) {
             Ok(pair) => pair,
             Err(e) => {
                 error!(
@@ -146,7 +142,7 @@ pub async fn garbage_collect_task(node: Arc<Darkfid>) -> Result<()> {
             .validator
             .blockchain
             .transactions
-            .get_after_pending(last_checked + TXS_CAP as u64, TXS_CAP)
+            .get_after_pending(last_checked + node.txs_batch_size as u64, node.txs_batch_size)
         {
             Ok(pair) => pair,
             Err(e) => {

bin/darkfid/src/task/miner.rs

@@ -356,7 +356,7 @@ async fn generate_next_block(
     let next_block_height = last_proposal.block.header.height + 1;
 
     // Grab forks' unproposed transactions
-    let (mut txs, fees, _) = extended_fork
+    let (mut txs, _, fees) = extended_fork
         .unproposed_txs(&extended_fork.blockchain, next_block_height, block_target, verify_fees)
         .await?;
 

bin/darkfid/src/tests/harness.rs

@@ -244,7 +244,7 @@ pub async fn generate_node(
     subscribers.insert("proposals", JsonSubscriber::new("blockchain.subscribe_proposals"));
 
     let p2p = spawn_p2p(settings, &validator, &subscribers, ex.clone()).await;
-    let node = Darkfid::new(p2p.clone(), validator, miner, subscribers, None).await;
+    let node = Darkfid::new(p2p.clone(), validator, miner, 50, subscribers, None).await;
 
     p2p.start().await?;
 

bin/darkfid/src/tests/unproposed_txs.rs

@@ -95,7 +95,7 @@ async fn simulate_unproposed_txs(
     let best_fork = &forks[best_fork_index(&forks)?];
 
     // Retrieve unproposed transactions
-    let (tx, _, total_gas_used) = best_fork
+    let (tx, total_gas_used, _) = best_fork
         .unproposed_txs(
             &best_fork.clone().blockchain,
             current_block_height,

src/contract/money/tests/delayed_tx.rs

@@ -145,7 +145,8 @@ fn delayed_tx() -> Result<()> {
                 false,
                 false,
             )
-            .await?;
+            .await?
+            .0;
 
         let coin = &output_coins[0];
         let change_value = coin.note.value - gas_used;

src/contract/test-harness/src/money_fee.rs

@@ -227,7 +227,8 @@ impl TestHarness {
                 false,
                 false,
             )
-            .await?;
+            .await?
+            .0;
 
         // Knowing the total gas, we can now find an OwnCoin of enough value
         // so that we can create a valid Money::Fee call.

src/error.rs

@@ -291,9 +291,6 @@ pub enum Error {
     #[error("Proposal contains missmatched headers")]
     ProposalHeadersMissmatchError,
 
-    #[error("Proposal contains more transactions than configured cap")]
-    ProposalTxsExceedCapError,
-
     #[error("Unable to verify transfer transaction")]
     TransferTxVerification,
 

src/validator/consensus.rs

@@ -41,9 +41,6 @@ use crate::{
 };
 
 // Consensus configuration
-/// Block/proposal maximum transactions, exluding producer transaction
-pub const TXS_CAP: usize = 50;
-
 /// Average amount of gas consumed during transaction execution, derived by the Gas Analyzer
 const GAS_TX_AVG: u64 = 23_822_290;
 
@@ -103,7 +100,7 @@ impl Consensus {
 
     /// Given a proposal, the node verifys it and finds which fork it extends.
     /// If the proposal extends the canonical blockchain, a new fork chain is created.
-    pub async fn append_proposal(&self, proposal: &Proposal) -> Result<()> {
+    pub async fn append_proposal(&self, proposal: &Proposal, verify_fees: bool) -> Result<()> {
         debug!(target: "validator::consensus::append_proposal", "Appending proposal {}", proposal.hash);
 
         // Check if proposal already exists
@@ -120,7 +117,7 @@ impl Consensus {
         drop(lock);
 
         // Verify proposal and grab corresponding fork
-        let (mut fork, index) = verify_proposal(self, proposal).await?;
+        let (mut fork, index) = verify_proposal(self, proposal, verify_fees).await?;
 
         // Append proposal to the fork
         fork.append_proposal(proposal).await?;
@@ -620,7 +617,7 @@ impl Fork {
     }
 
     /// Auxiliary function to retrieve unproposed valid transactions,
-    /// along with their total paid fees and total gas used.
+    /// along with their total gas used and total paid fees.
     pub async fn unproposed_txs(
         &self,
         blockchain: &Blockchain,
@@ -637,8 +634,8 @@ impl Fork {
         let mut tree = MerkleTree::new(1);
 
         // Total gas accumulators
-        let mut total_gas_paid = 0;
         let mut total_gas_used = 0;
+        let mut total_gas_paid = 0;
 
         // Map of ZK proof verifying keys for the current transaction batch
         let mut vks: HashMap<[u8; 32], HashMap<String, VerifyingKey>> = HashMap::new();
@@ -697,14 +694,14 @@ impl Fork {
             }
 
             // Update accumulated total gas
-            total_gas_paid += tx_gas_paid;
             total_gas_used += tx_gas_used;
+            total_gas_paid += tx_gas_paid;
 
             // Push the tx hash into the unproposed transactions vector
             unproposed_txs.push(unproposed_tx);
         }
 
-        Ok((unproposed_txs, total_gas_paid, total_gas_used))
+        Ok((unproposed_txs, total_gas_used, total_gas_paid))
     }
 
     /// Auxiliary function to create a full clone using BlockchainOverlay::full_clone.

src/validator/mod.rs

@@ -322,7 +322,7 @@ impl Validator {
         let append_lock = self.consensus.append_lock.write().await;
 
         // Execute append
-        let result = self.consensus.append_proposal(proposal).await;
+        let result = self.consensus.append_proposal(proposal, self.verify_fees).await;
 
         // Release append lock
         drop(append_lock);
@@ -512,7 +512,7 @@ impl Validator {
         // Validate and insert each block
         for block in blocks {
             // Verify block
-            match verify_block(&overlay, &module, block, previous).await {
+            match verify_block(&overlay, &module, block, previous, self.verify_fees).await {
                 Ok(()) => { /* Do nothing */ }
                 // Skip already existing block
                 Err(Error::BlockAlreadyExists(_)) => {
@@ -582,7 +582,7 @@ impl Validator {
     /// the state transitions to the database, and a boolean called `verify_fees` to
     /// overwrite the nodes configured `verify_fees` flag.
     ///
-    /// Returns the total gas used for the given transactions.
+    /// Returns the total gas used and total paid fees for the given transactions.
     /// Note: this function should only be used in tests.
     pub async fn add_test_transactions(
         &self,
@@ -591,7 +591,7 @@ impl Validator {
         block_target: u32,
         write: bool,
         verify_fees: bool,
-    ) -> Result<u64> {
+    ) -> Result<(u64, u64)> {
         debug!(target: "validator::add_transactions", "Instantiating BlockchainOverlay");
         let overlay = BlockchainOverlay::new(&self.blockchain)?;
 
@@ -614,17 +614,17 @@ impl Validator {
             return Err(e)
         }
 
-        let gas_used = verify_result.unwrap();
+        let gas_values = verify_result.unwrap();
 
         if !write {
             debug!(target: "validator::add_transactions", "Skipping apply of state updates because write=false");
             overlay.purge_new_trees()?;
-            return Ok(gas_used)
+            return Ok(gas_values)
         }
 
         debug!(target: "validator::add_transactions", "Applying overlay changes");
         overlay.apply()?;
-        Ok(gas_used)
+        Ok(gas_values)
     }
 
     /// Validate a producer `Transaction` and apply it if valid.
@@ -711,7 +711,7 @@ impl Validator {
         // Validate and insert each block
         for block in &blocks[1..] {
             // Verify block
-            if verify_block(&overlay, &module, block, previous).await.is_err() {
+            if verify_block(&overlay, &module, block, previous, self.verify_fees).await.is_err() {
                 error!(target: "validator::validate_blockchain", "Erroneous block found in set");
                 overlay.lock().unwrap().overlay.lock().unwrap().purge_new_trees()?;
                 return Err(Error::BlockIsInvalid(block.hash().as_string()))

src/validator/verification.rs

@@ -42,7 +42,7 @@ use crate::{
     runtime::vm_runtime::Runtime,
     tx::{Transaction, MAX_TX_CALLS, MIN_TX_CALLS},
     validator::{
-        consensus::{Consensus, Fork, Proposal, TXS_CAP},
+        consensus::{Consensus, Fork, Proposal, GAS_LIMIT_UNPROPOSED_TXS},
         fees::{circuit_gas_use, PALLAS_SCHNORR_SIGNATURE_FEE},
         pow::PoWModule,
     },
@@ -86,7 +86,8 @@ pub async fn verify_genesis_block(
         return Err(TxVerifyFailed::ErroneousTxs(vec![producer_tx.clone()]).into())
     }
 
-    // Verify transactions, exluding producer(last) one
+    // Verify transactions, exluding producer(last) one/
+    // Genesis block doesn't check for fees
     let mut tree = MerkleTree::new(1);
     let txs = &block.txs[..block.txs.len() - 1];
     if let Err(e) =
@@ -177,6 +178,7 @@ pub async fn verify_block(
     module: &PoWModule,
     block: &BlockInfo,
     previous: &BlockInfo,
+    verify_fees: bool,
 ) -> Result<()> {
     let block_hash = block.hash();
     debug!(target: "validator::verification::verify_block", "Validating block {}", block_hash);
@@ -197,8 +199,15 @@ pub async fn verify_block(
     // Verify transactions, exluding producer(last) one
     let mut tree = MerkleTree::new(1);
     let txs = &block.txs[..block.txs.len() - 1];
-    let e = verify_transactions(overlay, block.header.height, module.target, txs, &mut tree, false)
-        .await;
+    let e = verify_transactions(
+        overlay,
+        block.header.height,
+        module.target,
+        txs,
+        &mut tree,
+        verify_fees,
+    )
+    .await;
     if let Err(e) = e {
         warn!(
             target: "validator::verification::verify_block",
@@ -867,8 +876,9 @@ async fn apply_transaction(
 
 /// Verify a set of [`Transaction`] in sequence and apply them if all are valid.
 /// In case any of the transactions fail, they will be returned to the caller as an error.
-/// If all transactions are valid, the function will return the accumulated gas used from
-/// all the transactions. Additionally, their hash is appended to the provided Merkle tree.
+/// If all transactions are valid, the function will return the total gas used and total
+/// paid fees from all the transactions. Additionally, their hash is appended to the provided
+/// Merkle tree.
 pub async fn verify_transactions(
     overlay: &BlockchainOverlayPtr,
     verifying_block_height: u32,
@@ -876,17 +886,18 @@ pub async fn verify_transactions(
     txs: &[Transaction],
     tree: &mut MerkleTree,
     verify_fees: bool,
-) -> Result<u64> {
+) -> Result<(u64, u64)> {
     debug!(target: "validator::verification::verify_transactions", "Verifying {} transactions", txs.len());
     if txs.is_empty() {
-        return Ok(0)
+        return Ok((0, 0))
     }
 
     // Tracker for failed txs
     let mut erroneous_txs = vec![];
 
-    // Gas accumulator
-    let mut gas_used = 0;
+    // Total gas accumulators
+    let mut total_gas_used = 0;
+    let mut total_gas_paid = 0;
 
     // Map of ZK proof verifying keys for the current transaction batch
     let mut vks: HashMap<[u8; 32], HashMap<String, VerifyingKey>> = HashMap::new();
@@ -901,7 +912,7 @@ pub async fn verify_transactions(
     // Iterate over transactions and attempt to verify them
     for tx in txs {
         overlay.lock().unwrap().checkpoint();
-        match verify_transaction(
+        let (tx_gas_used, tx_gas_paid) = match verify_transaction(
             overlay,
             verifying_block_height,
             block_target,
@@ -912,20 +923,36 @@ pub async fn verify_transactions(
         )
         .await
         {
-            Ok((gas, _)) => gas_used += gas,
+            Ok(gas_values) => gas_values,
             Err(e) => {
                 warn!(target: "validator::verification::verify_transactions", "Transaction verification failed: {}", e);
                 erroneous_txs.push(tx.clone());
                 overlay.lock().unwrap().revert_to_checkpoint()?;
+                continue
             }
+        };
+
+        // Calculate current accumulated gas usage
+        let accumulated_gas_usage = total_gas_used + tx_gas_used;
+
+        // Check gas limit - if accumulated gas used exceeds it, break out of loop
+        if accumulated_gas_usage > GAS_LIMIT_UNPROPOSED_TXS {
+            warn!(target: "validator::verification::verify_transactions", "Transaction {} exceeds configured transaction gas limit: {} - {}", tx.hash(), accumulated_gas_usage, GAS_LIMIT_UNPROPOSED_TXS);
+            erroneous_txs.push(tx.clone());
+            overlay.lock().unwrap().revert_to_checkpoint()?;
+            break
         }
+
+        // Update accumulated total gas
+        total_gas_used += tx_gas_used;
+        total_gas_paid += tx_gas_paid;
     }
 
     if !erroneous_txs.is_empty() {
         return Err(TxVerifyFailed::ErroneousTxs(erroneous_txs).into())
     }
 
-    Ok(gas_used)
+    Ok((total_gas_used, total_gas_paid))
 }
 
 /// Apply given set of [`Transaction`] in sequence, without formal verification.
@@ -974,6 +1001,7 @@ async fn apply_transactions(
 pub async fn verify_proposal(
     consensus: &Consensus,
     proposal: &Proposal,
+    verify_fees: bool,
 ) -> Result<(Fork, Option<usize>)> {
     // Check if proposal hash matches actual one (1)
     let proposal_hash = proposal.block.hash();
@@ -985,16 +1013,6 @@ pub async fn verify_proposal(
         return Err(Error::ProposalHashesMissmatchError)
     }
 
-    // Check that proposal transactions don't exceed limit (2)
-    if proposal.block.txs.len() > TXS_CAP + 1 {
-        warn!(
-            target: "validator::verification::verify_pow_proposal", "Received proposal transactions exceed configured cap: {} - {}",
-            proposal.block.txs.len(),
-            TXS_CAP
-        );
-        return Err(Error::ProposalTxsExceedCapError)
-    }
-
     // Check if proposal extends any existing forks
     let (fork, index) = consensus.find_extended_fork(proposal).await?;
 
@@ -1002,7 +1020,10 @@ pub async fn verify_proposal(
     let previous = fork.overlay.lock().unwrap().last_block()?;
 
     // Verify proposal block (3)
-    if verify_block(&fork.overlay, &fork.module, &proposal.block, &previous).await.is_err() {
+    if verify_block(&fork.overlay, &fork.module, &proposal.block, &previous, verify_fees)
+        .await
+        .is_err()
+    {
         error!(target: "validator::verification::verify_pow_proposal", "Erroneous proposal block found");
         fork.overlay.lock().unwrap().overlay.lock().unwrap().purge_new_trees()?;
         return Err(Error::BlockIsInvalid(proposal.hash.as_string()))