Crescendo Cleanup - Part 2 (#754)

* Get rid of some ghostdag_k.get(..)

* Move check_coinbase_outputs_limit back to check_coinbase_in_isolation

* Get rid of more ForkedParam.get(..)

* Move checks from validate_tx_in_header_context to validate_tx_in_isolation

* Remove check_transaction_payload

* Get rid of more ForkedParam.get(..)

* Remove crescendo activation logic from validate_body_in_isolation

* Get rid of some calls to ForkActivation.isActive

* Get rid of some pruning constants sanity checks and use of lower_bound()

* Get rid of calls to ForkedParam.upper_bound

* Remove some uses of ForkActivation

* fmt

* Address comments

Author: someone235

components/addressmanager/src/lib.rs

@@ -535,7 +535,11 @@ mod address_store_with_cache {
             assert_eq!(iter.count(), 0);
         }
 
+        // This test is indeterminate, so it is ignored by default.
+        // Every developer that changes the logic of the address manager should run this test locally before sending a PR.
+        // TODO: Maybe change statistical parameters to reduce the failure rate?
         #[test]
+        #[ignore]
         fn test_network_distribution_weighting() {
             kaspa_core::log::try_init_logger("info");
 

consensus/core/src/config/constants.rs

@@ -166,7 +166,7 @@ pub mod perf {
     impl PerfParams {
         pub fn adjust_to_consensus_params(&mut self, consensus_params: &Params) {
             // Allow caching up to 10x over the baseline
-            self.block_data_cache_size *= consensus_params.bps().upper_bound().clamp(1, 10) as usize;
+            self.block_data_cache_size *= consensus_params.bps().after().clamp(1, 10) as usize;
         }
     }
 }

consensus/core/src/errors/block.rs

@@ -97,9 +97,6 @@ pub enum RuleError {
     #[error("coinbase blue score of {0} is not the expected value of {1}")]
     BadCoinbasePayloadBlueScore(u64, u64),
 
-    #[error("coinbase mass commitment field is not zero")]
-    CoinbaseNonZeroMassCommitment,
-
     #[error("transaction in isolation validation failed for tx {0}: {1}")]
     TxInIsolationValidationFailed(TransactionId, TxRuleError),
 

consensus/core/src/errors/tx.rs

@@ -15,9 +15,6 @@ pub enum TxRuleError {
     #[error("transaction has non zero gas value")]
     TxHasGas,
 
-    #[error("a non coinbase transaction has a payload")]
-    NonCoinbaseTxHasPayload,
-
     #[error("transaction version {0} is unknown")]
     UnknownTxVersion(u16),
 
@@ -45,6 +42,9 @@ pub enum TxRuleError {
     #[error("script public key of coinbase output #{0} is too long")]
     CoinbaseScriptPublicKeyTooLong(usize),
 
+    #[error("coinbase mass commitment field is not zero")]
+    CoinbaseNonZeroMassCommitment,
+
     #[error(
         "transaction input #{0} tried to spend coinbase outpoint {1} with daa score of {2} 
     while the merging block daa score is {3} and the coinbase maturity period of {4} hasn't passed yet"

consensus/core/src/mass/mod.rs

@@ -433,7 +433,7 @@ mod tests {
         */
         for net in NetworkType::iter() {
             let params: Params = net.into();
-            let max_spk_len = (params.max_script_public_key_len().upper_bound() as u64)
+            let max_spk_len = (params.max_script_public_key_len().after() as u64)
                 .min(params.max_block_mass.div_ceil(params.mass_per_script_pub_key_byte));
             let max_plurality = (UTXO_CONST_STORAGE + max_spk_len).div_ceil(UTXO_UNIT_SIZE); // see utxo_plurality
             let product = params.storage_mass_parameter.checked_mul(max_plurality).and_then(|x| x.checked_mul(max_plurality));

consensus/src/consensus/mod.rs

@@ -971,7 +971,7 @@ impl ConsensusApi for Consensus {
     // max_blocks has to be greater than the merge set size limit
     fn get_hashes_between(&self, low: Hash, high: Hash, max_blocks: usize) -> ConsensusResult<(Vec<Hash>, Hash)> {
         let _guard = self.pruning_lock.blocking_read();
-        assert!(max_blocks as u64 > self.config.mergeset_size_limit().upper_bound());
+        assert!(max_blocks as u64 > self.config.mergeset_size_limit().after());
         self.validate_block_exists(low)?;
         self.validate_block_exists(high)?;
 
@@ -1187,9 +1187,7 @@ impl ConsensusApi for Consensus {
             return Err(ConsensusError::UnexpectedPruningPoint);
         }
 
-        // [Crescendo]: get ghostdag k based on the pruning point's DAA score. The off-by-one of not going by selected parent
-        // DAA score is not important here as we simply increase K one block earlier which is more conservative (saving/sending more data)
-        let ghostdag_k = self.config.ghostdag_k().get(self.headers_store.get_daa_score(pruning_point).unwrap());
+        let ghostdag_k = self.config.ghostdag_k().after();
 
         // Note: the method `get_ghostdag_chain_k_depth` might return a partial chain if data is missing.
         // Ideally this node when synced would validate it got all of the associated data up to k blocks

consensus/src/consensus/services.rs

@@ -150,6 +150,7 @@ impl ConsensusServices {
             params.max_script_public_key_len(),
             params.coinbase_payload_script_public_key_max_len,
             params.coinbase_maturity(),
+            params.ghostdag_k().after(),
             tx_script_cache_counters,
             mass_calculator.clone(),
             params.crescendo_activation,

consensus/src/consensus/storage.rs

@@ -83,9 +83,8 @@ impl ConsensusStorage {
         let perf_params = &config.perf;
 
         // Lower and upper bounds
-        // [Crescendo]: all usages of pruning upper bounds also bound by actual memory bytes, so we can safely use the larger values
-        let pruning_depth = params.pruning_depth().upper_bound() as usize;
-        let pruning_size_for_caches = pruning_depth + params.finality_depth().upper_bound() as usize; // Upper bound for any block/header related data
+        let pruning_depth = params.pruning_depth().after() as usize;
+        let pruning_size_for_caches = pruning_depth + params.finality_depth().after() as usize; // Upper bound for any block/header related data
         let level_lower_bound = 2 * params.pruning_proof_m as usize; // Number of items lower bound for level-related caches
 
         // Budgets in bytes. All byte budgets overall sum up to ~1GB of memory (which obviously takes more low level alloc space)

consensus/src/pipeline/body_processor/body_validation_in_context.rs

@@ -1,7 +1,7 @@
 use super::BlockBodyProcessor;
 use crate::{
     errors::{BlockProcessResult, RuleError},
-    model::stores::{ghostdag::GhostdagStoreReader, headers::HeaderStoreReader, statuses::StatusesStoreReader},
+    model::stores::statuses::StatusesStoreReader,
     processes::{
         transaction_validator::{
             tx_validation_in_header_context::{LockTimeArg, LockTimeType},
@@ -10,7 +10,7 @@ use crate::{
         window::WindowManager,
     },
 };
-use kaspa_consensus_core::{block::Block, errors::tx::TxRuleError};
+use kaspa_consensus_core::block::Block;
 use kaspa_database::prelude::StoreResultExtensions;
 use kaspa_hashes::Hash;
 use once_cell::unsync::Lazy;
@@ -19,7 +19,6 @@ use std::sync::Arc;
 impl BlockBodyProcessor {
     pub fn validate_body_in_context(self: &Arc<Self>, block: &Block) -> BlockProcessResult<()> {
         self.check_parent_bodies_exist(block)?;
-        self.check_coinbase_outputs_limit(block)?;
         self.check_coinbase_blue_score_and_subsidy(block)?;
         self.check_block_transactions_in_context(block)
     }
@@ -35,7 +34,7 @@ impl BlockBodyProcessor {
                 // We only evaluate the pmt calculation when actually needed
                 LockTimeType::Time => LockTimeArg::MedianTime((*lazy_pmt_res).clone()?),
             };
-            if let Err(e) = self.transaction_validator.validate_tx_in_header_context(tx, block.header.daa_score, lock_time_arg) {
+            if let Err(e) = self.transaction_validator.validate_tx_in_header_context(tx, lock_time_arg) {
                 return Err(RuleError::TxInContextFailed(tx.id(), e));
             };
         }
@@ -61,32 +60,6 @@ impl BlockBodyProcessor {
         Ok(())
     }
 
-    fn check_coinbase_outputs_limit(&self, block: &Block) -> BlockProcessResult<()> {
-        // [Crescendo]: coinbase_outputs_limit depends on ghostdag k and thus depends on fork activation
-        // which makes it header contextual.
-        //
-        // TODO (post HF): move this check back to transaction in isolation validation
-
-        // [Crescendo]: Ghostdag k activation is decided based on selected parent DAA score
-        // so we follow the same methodology for coinbase output limit (which is driven from the
-        // actual bound on the number of blue blocks in the mergeset).
-        //
-        // Note that body validation in context is not called for trusted blocks, so we can safely assume
-        // the selected parent exists and its daa score is accessible
-        let selected_parent = self.ghostdag_store.get_selected_parent(block.hash()).unwrap();
-        let selected_parent_daa_score = self.headers_store.get_daa_score(selected_parent).unwrap();
-        let coinbase_outputs_limit = self.ghostdag_k.get(selected_parent_daa_score) as u64 + 2;
-
-        let tx = &block.transactions[0];
-        if tx.outputs.len() as u64 > coinbase_outputs_limit {
-            return Err(RuleError::TxInIsolationValidationFailed(
-                tx.id(),
-                TxRuleError::CoinbaseTooManyOutputs(tx.outputs.len(), coinbase_outputs_limit),
-            ));
-        }
-        Ok(())
-    }
-
     fn check_coinbase_blue_score_and_subsidy(self: &Arc<Self>, block: &Block) -> BlockProcessResult<()> {
         match self.coinbase_manager.deserialize_coinbase_payload(&block.transactions[0].payload) {
             Ok(data) => {

consensus/src/pipeline/body_processor/body_validation_in_isolation.rs

@@ -11,14 +11,11 @@ use kaspa_consensus_core::{
 
 impl BlockBodyProcessor {
     pub fn validate_body_in_isolation(self: &Arc<Self>, block: &Block) -> BlockProcessResult<Mass> {
-        let crescendo_activated = self.crescendo_activation.is_active(block.header.daa_score);
-
         Self::check_has_transactions(block)?;
         Self::check_hash_merkle_root(block)?;
         Self::check_only_one_coinbase(block)?;
         self.check_transactions_in_isolation(block)?;
-        self.check_coinbase_has_zero_mass(block, crescendo_activated)?;
-        let mass = self.check_block_mass(block, crescendo_activated)?;
+        let mass = self.check_block_mass(block)?;
         self.check_duplicate_transactions(block)?;
         self.check_block_double_spends(block)?;
         self.check_no_chained_transactions(block)?;
@@ -63,56 +60,36 @@ impl BlockBodyProcessor {
         Ok(())
     }
 
-    fn check_coinbase_has_zero_mass(&self, block: &Block, crescendo_activated: bool) -> BlockProcessResult<()> {
-        // TODO (post HF): move to check_coinbase_in_isolation
-        if crescendo_activated && block.transactions[0].mass() > 0 {
-            return Err(RuleError::CoinbaseNonZeroMassCommitment);
-        }
-        Ok(())
-    }
-
-    fn check_block_mass(self: &Arc<Self>, block: &Block, crescendo_activated: bool) -> BlockProcessResult<Mass> {
-        if crescendo_activated {
-            let mut total_compute_mass: u64 = 0;
-            let mut total_transient_mass: u64 = 0;
-            let mut total_storage_mass: u64 = 0;
-            for tx in block.transactions.iter() {
-                // Calculate the non-contextual masses
-                let NonContextualMasses { compute_mass, transient_mass } = self.mass_calculator.calc_non_contextual_masses(tx);
-
-                // Read the storage mass commitment. This value cannot be computed here w/o UTXO context
-                // so we use the commitment. Later on, when the transaction is verified in context, we use
-                // the context to calculate the expected storage mass and verify it matches this commitment
-                let storage_mass_commitment = tx.mass();
-
-                // Sum over the various masses separately
-                total_compute_mass = total_compute_mass.saturating_add(compute_mass);
-                total_transient_mass = total_transient_mass.saturating_add(transient_mass);
-                total_storage_mass = total_storage_mass.saturating_add(storage_mass_commitment);
-
-                // Verify all limits
-                if total_compute_mass > self.max_block_mass {
-                    return Err(RuleError::ExceedsComputeMassLimit(total_compute_mass, self.max_block_mass));
-                }
-                if total_transient_mass > self.max_block_mass {
-                    return Err(RuleError::ExceedsTransientMassLimit(total_transient_mass, self.max_block_mass));
-                }
-                if total_storage_mass > self.max_block_mass {
-                    return Err(RuleError::ExceedsStorageMassLimit(total_storage_mass, self.max_block_mass));
-                }
+    fn check_block_mass(self: &Arc<Self>, block: &Block) -> BlockProcessResult<Mass> {
+        let mut total_compute_mass: u64 = 0;
+        let mut total_transient_mass: u64 = 0;
+        let mut total_storage_mass: u64 = 0;
+        for tx in block.transactions.iter() {
+            // Calculate the non-contextual masses
+            let NonContextualMasses { compute_mass, transient_mass } = self.mass_calculator.calc_non_contextual_masses(tx);
+
+            // Read the storage mass commitment. This value cannot be computed here w/o UTXO context
+            // so we use the commitment. Later on, when the transaction is verified in context, we use
+            // the context to calculate the expected storage mass and verify it matches this commitment
+            let storage_mass_commitment = tx.mass();
+
+            // Sum over the various masses separately
+            total_compute_mass = total_compute_mass.saturating_add(compute_mass);
+            total_transient_mass = total_transient_mass.saturating_add(transient_mass);
+            total_storage_mass = total_storage_mass.saturating_add(storage_mass_commitment);
+
+            // Verify all limits
+            if total_compute_mass > self.max_block_mass {
+                return Err(RuleError::ExceedsComputeMassLimit(total_compute_mass, self.max_block_mass));
+            }
+            if total_transient_mass > self.max_block_mass {
+                return Err(RuleError::ExceedsTransientMassLimit(total_transient_mass, self.max_block_mass));
             }
-            Ok((NonContextualMasses::new(total_compute_mass, total_transient_mass), ContextualMasses::new(total_storage_mass)))
-        } else {
-            let mut total_mass: u64 = 0;
-            for tx in block.transactions.iter() {
-                let compute_mass = self.mass_calculator.calc_non_contextual_masses(tx).compute_mass;
-                total_mass = total_mass.saturating_add(compute_mass);
-                if total_mass > self.max_block_mass {
-                    return Err(RuleError::ExceedsComputeMassLimit(total_mass, self.max_block_mass));
-                }
+            if total_storage_mass > self.max_block_mass {
+                return Err(RuleError::ExceedsStorageMassLimit(total_storage_mass, self.max_block_mass));
             }
-            Ok((NonContextualMasses::new(total_mass, 0), ContextualMasses::new(0)))
         }
+        Ok((NonContextualMasses::new(total_compute_mass, total_transient_mass), ContextualMasses::new(total_storage_mass)))
     }
 
     fn check_block_double_spends(self: &Arc<Self>, block: &Block) -> BlockProcessResult<()> {