review comments

base_layer/core/src/consensus/consensus_constants.rs

@@ -38,7 +38,6 @@ use crate::{
     transactions::{
         tari_amount::{uT, MicroMinotari},
         transaction_components::{
-            CoinBaseExtra,
             OutputFeatures,
             OutputFeaturesVersion,
             OutputType,
@@ -110,6 +109,8 @@ pub struct ConsensusConstants {
     permitted_output_types: &'static [OutputType],
     /// The allowlist of range proof types
     permitted_range_proof_types: [(OutputType, &'static [RangeProofType]); 5],
+    /// Coinbase outputs are allowed to have metadata, but it has the following length limit
+    coinbase_output_features_extra_max_length: u32,
     /// Maximum number of token elements permitted in covenants
     max_covenant_length: u32,
     /// Epoch duration in blocks
@@ -238,15 +239,18 @@ impl ConsensusConstants {
 
     fn calculate_1_output_kernel_weight(&self) -> std::io::Result<u64> {
         let output_features = OutputFeatures { ..Default::default() };
+        let max_extra_size = self.coinbase_output_features_extra_max_length() as usize;
 
         let features_and_scripts_size = self.transaction_weight.round_up_features_and_scripts_size(
-            output_features.get_serialized_size()? +
-                CoinBaseExtra::default().max_size() +
-                script![Nop].get_serialized_size()?,
+            output_features.get_serialized_size()? + max_extra_size + script![Nop].get_serialized_size()?,
         );
         Ok(self.transaction_weight.calculate(1, 0, 1, features_and_scripts_size))
     }
 
+    pub fn coinbase_output_features_extra_max_length(&self) -> u32 {
+        self.coinbase_output_features_extra_max_length
+    }
+
     /// The amount of PoW algorithms used by the Tari chain.
     pub fn pow_algo_count(&self) -> u64 {
         self.proof_of_work.len() as u64
@@ -411,6 +415,7 @@ impl ConsensusConstants {
             vn_registration_min_deposit_amount: MicroMinotari(0),
             vn_registration_lock_height: 0,
             vn_registration_shuffle_interval: VnEpoch(100),
+            coinbase_output_features_extra_max_length: 64,
         }];
         #[cfg(any(test, debug_assertions))]
         assert_hybrid_pow_constants(&consensus_constants, &[120], &[50], &[50]);
@@ -476,6 +481,7 @@ impl ConsensusConstants {
             vn_registration_min_deposit_amount: MicroMinotari(0),
             vn_registration_lock_height: 0,
             vn_registration_shuffle_interval: VnEpoch(100),
+            coinbase_output_features_extra_max_length: 64,
         }];
         #[cfg(any(test, debug_assertions))]
         assert_hybrid_pow_constants(&consensus_constants, &[target_time], &[randomx_split], &[sha3x_split]);
@@ -532,6 +538,7 @@ impl ConsensusConstants {
             vn_registration_min_deposit_amount: MicroMinotari(0),
             vn_registration_lock_height: 0,
             vn_registration_shuffle_interval: VnEpoch(100),
+            coinbase_output_features_extra_max_length: 64,
         }];
         #[cfg(any(test, debug_assertions))]
         assert_hybrid_pow_constants(&consensus_constants, &[120], &[50], &[50]);
@@ -588,6 +595,7 @@ impl ConsensusConstants {
             vn_registration_min_deposit_amount: MicroMinotari(0),
             vn_registration_lock_height: 0,
             vn_registration_shuffle_interval: VnEpoch(100),
+            coinbase_output_features_extra_max_length: 64,
         }];
         #[cfg(any(test, debug_assertions))]
         assert_hybrid_pow_constants(&consensus_constants, &[120], &[50], &[50]);
@@ -638,6 +646,7 @@ impl ConsensusConstants {
             vn_registration_min_deposit_amount: MicroMinotari(0),
             vn_registration_lock_height: 0,
             vn_registration_shuffle_interval: VnEpoch(100),
+            coinbase_output_features_extra_max_length: 64,
         }];
         #[cfg(any(test, debug_assertions))]
         assert_hybrid_pow_constants(&consensus_constants, &[120], &[50], &[50]);
@@ -690,6 +699,7 @@ impl ConsensusConstants {
             vn_registration_min_deposit_amount: MicroMinotari(0),
             vn_registration_lock_height: 0,
             vn_registration_shuffle_interval: VnEpoch(100),
+            coinbase_output_features_extra_max_length: 64,
         }];
         #[cfg(any(test, debug_assertions))]
         assert_hybrid_pow_constants(&consensus_constants, &[120], &[50], &[50]);

base_layer/core/src/proof_of_work/monero_rx/pow_data.rs

@@ -203,12 +203,22 @@ impl Display for MoneroPowData {
 #[cfg(test)]
 mod test {
     use borsh::{BorshDeserialize, BorshSerialize};
-    use monero::{consensus::Encodable, BlockHeader, Hash, VarInt};
+    use chacha20poly1305::aead::OsRng;
+    use monero::{blockdata::transaction::RawExtraField, consensus::Encodable, BlockHeader, Hash, VarInt};
+    use tari_common::configuration::Network;
+    use tari_common_types::types::PrivateKey;
+    use tari_crypto::keys::SecretKey;
     use tari_utilities::ByteArray;
     use tiny_keccak::{Hasher, Keccak};
 
     use super::MoneroPowData;
-    use crate::proof_of_work::monero_rx::{merkle_tree::MerkleProof, FixedByteArray};
+    use crate::{
+        consensus::NetworkConsensus,
+        proof_of_work::{
+            monero_rx::{merkle_tree::MerkleProof, FixedByteArray},
+            PowData,
+        },
+    };
 
     #[test]
     fn test_borsh_de_serialization() {
@@ -249,4 +259,51 @@ mod test {
         MoneroPowData::deserialize(buf).unwrap();
         assert_eq!(buf, &[1, 2, 3]);
     }
+
+    #[test]
+    fn max_monero_pow_data_bytes_fits_inside_proof_of_work_pow_data() {
+        let coinbase: monero::Transaction = Default::default();
+        let mut keccak = Keccak::v256();
+        let mut encoder_prefix = Vec::new();
+        coinbase.prefix.version.consensus_encode(&mut encoder_prefix).unwrap();
+        coinbase
+            .prefix
+            .unlock_time
+            .consensus_encode(&mut encoder_prefix)
+            .unwrap();
+        coinbase.prefix.inputs.consensus_encode(&mut encoder_prefix).unwrap();
+        coinbase.prefix.outputs.consensus_encode(&mut encoder_prefix).unwrap();
+        keccak.update(&encoder_prefix);
+
+        for network in [
+            Network::MainNet,
+            Network::StageNet,
+            Network::LocalNet,
+            Network::NextNet,
+            Network::Igor,
+            Network::Esmeralda,
+        ] {
+            for consensus_constants in NetworkConsensus::from(network).create_consensus_constants() {
+                let monero_pow_data = MoneroPowData {
+                    header: BlockHeader {
+                        major_version: VarInt(u64::MAX),
+                        minor_version: VarInt(u64::MAX),
+                        timestamp: VarInt(u64::MAX),
+                        prev_id: Hash::new(PrivateKey::random(&mut OsRng).to_vec()),
+                        nonce: u32::MAX,
+                    },
+                    randomx_key: FixedByteArray::default(),
+                    transaction_count: u16::MAX,
+                    merkle_root: Hash::new(PrivateKey::random(&mut OsRng).to_vec()),
+                    coinbase_merkle_proof: MerkleProof::default(),
+                    coinbase_tx_extra: RawExtraField(vec![1u8; consensus_constants.max_extra_field_size()]),
+                    coinbase_tx_hasher: keccak.clone(),
+                    aux_chain_merkle_proof: MerkleProof::default(),
+                };
+                let mut buf = Vec::new();
+                monero_pow_data.serialize(&mut buf).unwrap();
+                assert!(buf.len() <= PowData::default().max_size());
+            }
+        }
+    }
 }

base_layer/core/src/transactions/aggregated_body.rs

@@ -327,11 +327,18 @@ impl AggregateBody {
         Ok(())
     }
 
-    pub fn verify_non_coinbase_has_coinbase_extra_empty(&self) -> Result<(), TransactionError> {
+    pub fn check_output_features(&self, max_coinbase_metadata_size: u32) -> Result<(), TransactionError> {
         for output in self.outputs() {
             if !output.is_coinbase() && !output.features.coinbase_extra.is_empty() {
                 return Err(TransactionError::NonCoinbaseHasOutputFeaturesCoinbaseExtra);
             }
+
+            if output.is_coinbase() && output.features.coinbase_extra.len() > max_coinbase_metadata_size as usize {
+                return Err(TransactionError::InvalidOutputFeaturesCoinbaseExtraSize {
+                    len: output.features.coinbase_extra.len(),
+                    max: max_coinbase_metadata_size,
+                });
+            }
         }
 
         Ok(())

base_layer/core/src/validation/block_body/block_body_internal_validator.rs

@@ -25,7 +25,7 @@ use tari_utilities::hex::Hex;
 
 use crate::{
     blocks::Block,
-    consensus::ConsensusManager,
+    consensus::{ConsensusConstants, ConsensusManager},
     transactions::{aggregated_body::AggregateBody, CryptoFactories},
     validation::{
         aggregate_body::AggregateBodyInternalConsistencyValidator,
@@ -80,18 +80,23 @@ fn validate_block_specific_checks(
     consensus_manager: &ConsensusManager,
     factories: &CryptoFactories,
 ) -> Result<(), ValidationError> {
+    let constants = consensus_manager.consensus_constants(block.header.height);
     if block.header.height == 0 {
         warn!(target: LOG_TARGET, "Attempt to validate genesis block");
         return Err(ValidationError::ValidatingGenesis);
     }
     check_coinbase_output(block, consensus_manager, factories)?;
-    check_coinbase_output_features(&block.body)?;
+    check_coinbase_output_features(&block.body, constants)?;
 
     Ok(())
 }
 
-fn check_coinbase_output_features(body: &AggregateBody) -> Result<(), ValidationError> {
-    body.verify_non_coinbase_has_coinbase_extra_empty()
+fn check_coinbase_output_features(
+    body: &AggregateBody,
+    consensus_constants: &ConsensusConstants,
+) -> Result<(), ValidationError> {
+    let max_coinbase_metadata_size = consensus_constants.coinbase_output_features_extra_max_length();
+    body.check_output_features(max_coinbase_metadata_size)
         .map_err(ValidationError::from)
 }
 

base_layer/core/src/validation/transaction/transaction_internal_validator.rs

@@ -77,7 +77,9 @@ impl TransactionInternalConsistencyValidator {
             return Err(ValidationError::OutputTypeNotPermitted { output_type: Coinbase });
         }
 
-        tx.body.verify_non_coinbase_has_coinbase_extra_empty()?;
+        // We can call this function with a constant value, because we've just shown that this is NOT a coinbase, and
+        // only coinbases may have the extra field set (the only field that the fn argument affects).
+        tx.body.check_output_features(1)?;
 
         self.aggregate_body_validator.validate(
             &tx.body,