review comments

applications/minotari_merge_mining_proxy/src/block_template_protocol.rs

@@ -233,9 +233,9 @@ impl BlockTemplateProtocol<'_> {
         debug!(target: LOG_TARGET, "Deserializing Blocktemplate Blob into Monero Block",);
         let mut monero_block = monero_rx::deserialize_monero_block_from_hex(&monero_mining_data.blocktemplate_blob)?;
 
-        debug!(target: LOG_TARGET, "Appending Merged Mining Tag",);
+        debug!(target: LOG_TARGET, "Insert Merged Mining Tag",);
         // Add the Tari merge mining tag to the retrieved block template
-        monero_rx::append_merge_mining_tag(&mut monero_block, &tari_block.merge_mining_hash)?;
+        monero_rx::proxy_insert_merge_mining_tag_into_block(&mut monero_block, &tari_block.merge_mining_hash)?;
 
         debug!(target: LOG_TARGET, "Creating blockhashing blob from blocktemplate blob",);
         // Must be done after the tag is inserted since it will affect the hash of the miner tx

applications/minotari_merge_mining_proxy/src/proxy.rs

@@ -243,7 +243,7 @@ impl InnerService {
         for param in params.iter().filter_map(|p| p.as_str()) {
             let monero_block = monero_rx::deserialize_monero_block_from_hex(param)?;
             debug!(target: LOG_TARGET, "Monero block: {}", monero_block);
-            let hash = monero_rx::extract_tari_hash(&monero_block)?.ok_or_else(|| {
+            let hash = monero_rx::proxy_extract_tari_hash_from_block(&monero_block)?.ok_or_else(|| {
                 MmProxyError::MissingDataError("Could not find Minotari header in coinbase".to_string())
             })?;
 

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

@@ -23,7 +23,7 @@ use std::iter;
 
 use log::*;
 use monero::{
-    blockdata::transaction::{ExtraField, SubField},
+    blockdata::transaction::{ExtraField, RawExtraField, SubField},
     consensus,
     cryptonote::hash::Hashable,
     VarInt,
@@ -66,6 +66,23 @@ fn get_random_x_difficulty(input: &[u8], vm: &RandomXVMInstance) -> Result<(Diff
     Ok((difficulty, hash))
 }
 
+// Parsing an extra field from bytes will always return an extra field with sub-fields that could be read, even if it
+// does not represent the original extra field. As per Monero consensus rules, an error here will not represent a
+// failure to deserialize a block, so no need to error here.
+fn parse_extra_field_truncate_on_error(raw_extra_field: &RawExtraField) -> ExtraField {
+    match ExtraField::try_parse(raw_extra_field) {
+        Ok(val) => val,
+        Err(val) => {
+            warn!(
+                target: LOG_TARGET,
+                "Some sub-fields could not be parsed successfully from the Monero coinbase extra field and will be \
+                excluded"
+            );
+            val
+        },
+    }
+}
+
 /// Validates the monero data contained in the given header, making these assetions:
 /// 1. The MoneroPowData is well-formed (i.e. can be deserialized)
 /// 1. The header's merge mining hash is included in the coinbase extra field
@@ -76,16 +93,17 @@ pub fn verify_header(header: &BlockHeader) -> Result<MoneroPowData, MergeMineErr
     let monero_data = MoneroPowData::from_header(header)?;
     let expected_merge_mining_hash = header.merge_mining_hash();
 
-    // Parsing an extra field from bytes will always return a valid extra field, even if it does not represent the
-    // original extra field. As per Monero consensus rules, an error here will not represent a failure to deserialize a
-    // block, so no need to error here.
+    // When verifying the header we only accept valid extra fields.
     let extra_field = match ExtraField::try_parse(&monero_data.coinbase_tx.prefix.extra) {
         Ok(val) => val,
-        Err(val) => val,
+        Err(_) => {
+            return Err(MergeMineError::ValidationError(
+                "Error when parsing the Monero coinbase extra field".to_string(),
+            ));
+        },
     };
 
-    // Check that the Tari MM hash is found in the monero coinbase transaction
-    // and that only 1 tari header is found
+    // Check that the Tari MM hash is found in the Monero coinbase transaction and that only 1 Tari header is found
     let mut is_found = false;
     let mut already_seen_mmfield = false;
     for item in extra_field.0 {
@@ -116,14 +134,9 @@ pub fn verify_header(header: &BlockHeader) -> Result<MoneroPowData, MergeMineErr
 }
 
 /// Extracts the Monero block hash from the coinbase transaction's extra field
-pub fn extract_tari_hash(monero: &monero::Block) -> Result<Option<monero::Hash>, MergeMineError> {
-    // Parsing an extra field from bytes will always return a valid extra field, even if it does not represent the
-    // original extra field. As per Monero consensus rules, an error here will not represent a failure to deserialize a
-    // block, so no need to error here.
-    let extra_field = match ExtraField::try_parse(&monero.miner_tx.prefix.extra) {
-        Ok(val) => val,
-        Err(val) => val,
-    };
+pub fn proxy_extract_tari_hash_from_block(monero: &monero::Block) -> Result<Option<monero::Hash>, MergeMineError> {
+    // When we extract the merge mining hash, we do not care if the extra field can be parsed without error.
+    let extra_field = parse_extra_field_truncate_on_error(&monero.miner_tx.prefix.extra);
     for item in &extra_field.0 {
         if let SubField::MergeMining(_depth, merge_mining_hash) = item {
             return Ok(Some(*merge_mining_hash));
@@ -185,22 +198,20 @@ pub fn create_ordered_transaction_hashes_from_block(block: &monero::Block) -> Ve
         .collect()
 }
 
-/// Appends merge mining hash to a Monero block
-pub fn append_merge_mining_tag<T: AsRef<[u8]>>(block: &mut monero::Block, hash: T) -> Result<(), MergeMineError> {
+/// Inserts merge mining hash into a Monero block
+pub fn proxy_insert_merge_mining_tag_into_block<T: AsRef<[u8]>>(
+    block: &mut monero::Block,
+    hash: T,
+) -> Result<(), MergeMineError> {
     if hash.as_ref().len() != monero::Hash::len_bytes() {
         return Err(MergeMineError::HashingError(format!(
             "Expected source to be {} bytes, but it was {} bytes",
             monero::Hash::len_bytes(),
             hash.as_ref().len()
         )));
     }
-    // Parsing an extra field from bytes will always return a valid extra field, even if it does not represent the
-    // original extra field. As per Monero consensus rules, an error here will not represent a failure to deserialize a
-    // block, so no need to error here.
-    let mut extra_field = match ExtraField::try_parse(&block.miner_tx.prefix.extra) {
-        Ok(val) => val,
-        Err(val) => val,
-    };
+    // When we insert the merge mining tag, we need to make sure that the extra field is valid.
+    let mut extra_field = parse_extra_field_truncate_on_error(&block.miner_tx.prefix.extra);
 
     // Adding more than one merge mining tag is not allowed
     for item in &extra_field.0 {
@@ -216,9 +227,7 @@ pub fn append_merge_mining_tag<T: AsRef<[u8]>>(block: &mut monero::Block, hash:
     // To circumvent this, we create a new extra field by appending the original extra field to the merge mining field
     // instead.
     let hash = monero::Hash::from_slice(hash.as_ref());
-    let mut fields = vec![SubField::MergeMining(Some(VarInt(0)), hash)];
-    fields.append(&mut extra_field.0);
-    extra_field.0 = fields;
+    extra_field.0.insert(0, SubField::MergeMining(Some(VarInt(0)), hash));
 
     block.miner_tx.prefix.extra = extra_field.into();
     Ok(())
@@ -369,7 +378,7 @@ mod test {
             validator_node_mr: FixedHash::zero(),
         };
         let hash = block_header.merge_mining_hash();
-        append_merge_mining_tag(&mut block, hash).unwrap();
+        proxy_insert_merge_mining_tag_into_block(&mut block, hash).unwrap();
         let hashes = create_ordered_transaction_hashes_from_block(&block);
         assert_eq!(hashes.len(), block.tx_hashes.len() + 1);
         let root = tree_hash(&hashes).unwrap();
@@ -425,7 +434,7 @@ mod test {
             validator_node_mr: FixedHash::zero(),
         };
         let hash = block_header.merge_mining_hash();
-        append_merge_mining_tag(&mut block, hash).unwrap();
+        proxy_insert_merge_mining_tag_into_block(&mut block, hash).unwrap();
         let count = 1 + (u16::try_from(block.tx_hashes.len()).unwrap());
         let mut hashes = Vec::with_capacity(count as usize);
         hashes.push(block.miner_tx.hash());
@@ -528,7 +537,7 @@ mod test {
             validator_node_mr: FixedHash::zero(),
         };
         let hash = Hash::null();
-        append_merge_mining_tag(&mut block, hash).unwrap();
+        proxy_insert_merge_mining_tag_into_block(&mut block, hash).unwrap();
         let count = 1 + (u16::try_from(block.tx_hashes.len()).unwrap());
         let mut hashes = Vec::with_capacity(count as usize);
         let mut proof = Vec::with_capacity(count as usize);
@@ -583,24 +592,20 @@ mod test {
             validator_node_mr: FixedHash::zero(),
         };
         let hash = block_header.merge_mining_hash();
-        append_merge_mining_tag(&mut block, hash).unwrap();
+        proxy_insert_merge_mining_tag_into_block(&mut block, hash).unwrap();
         #[allow(clippy::redundant_clone)]
         let mut block_header2 = block_header.clone();
         block_header2.version = 1;
         let hash2 = block_header2.merge_mining_hash();
 
-        // Try via the API
-        assert!(append_merge_mining_tag(&mut block, hash2).is_err());
+        // Try via the API - this will fail because more than one merge mining tag is not allowed
+        assert!(proxy_insert_merge_mining_tag_into_block(&mut block, hash2).is_err());
 
-        // Now bypass the API
-        let mut extra_field = match ExtraField::try_parse(&block.miner_tx.prefix.extra) {
-            Ok(val) => val,
-            Err(val) => val,
-        };
+        // Now bypass the API - this will effectively allow us to insert more than one merge mining tag,
+        // like trying to sneek it in. Later on, when we call `verify_header(&block_header)`, it should fail.
+        let mut extra_field = ExtraField::try_parse(&block.miner_tx.prefix.extra).unwrap();
         let hash = monero::Hash::from_slice(hash.as_ref());
-        let mut fields = vec![SubField::MergeMining(Some(VarInt(0)), hash)];
-        fields.append(&mut extra_field.0);
-        extra_field.0 = fields;
+        extra_field.0.insert(0, SubField::MergeMining(Some(VarInt(0)), hash));
         block.miner_tx.prefix.extra = extra_field.into();
 
         let count = 1 + (u16::try_from(block.tx_hashes.len()).unwrap());
@@ -633,6 +638,63 @@ mod test {
         assert!(details.contains("More than one merge mining tag found in coinbase"));
     }
 
+    #[test]
+    fn test_extra_field_with_parsing_error() {
+        let blocktemplate_blob = "0c0c8cd6a0fa057fe21d764e7abf004e975396a2160773b93712bf6118c3b4959ddd8ee0f76aad0000000002e1ea2701ffa5ea2701d5a299e2abb002028eb3066ced1b2cc82ea046f3716a48e9ae37144057d5fb48a97f941225a1957b2b0106225b7ec0a6544d8da39abe68d8bd82619b4a7c5bdae89c3783b256a8fa47820208f63aa86d2e857f070000".to_string();
+        let bytes = hex::decode(blocktemplate_blob).unwrap();
+        let mut block = deserialize::<monero::Block>(&bytes[..]).unwrap();
+        let block_header = BlockHeader {
+            version: 0,
+            height: 0,
+            prev_hash: FixedHash::zero(),
+            timestamp: EpochTime::now(),
+            output_mr: FixedHash::zero(),
+            output_mmr_size: 0,
+            kernel_mr: FixedHash::zero(),
+            kernel_mmr_size: 0,
+            input_mr: FixedHash::zero(),
+            total_kernel_offset: Default::default(),
+            total_script_offset: Default::default(),
+            nonce: 0,
+            pow: ProofOfWork::default(),
+            validator_node_mr: FixedHash::zero(),
+        };
+
+        // Let us manipulate the extra field to make it invalid
+        let mut extra_field_before_parse = ExtraField::try_parse(&block.miner_tx.prefix.extra).unwrap();
+        assert_eq!(
+            "ExtraField([TxPublicKey(06225b7ec0a6544d8da39abe68d8bd82619b4a7c5bdae89c3783b256a8fa4782), Nonce([246, \
+             58, 168, 109, 46, 133, 127, 7])])",
+            &format!("{:?}", extra_field_before_parse)
+        );
+        assert!(ExtraField::try_parse(&extra_field_before_parse.clone().into()).is_ok());
+
+        extra_field_before_parse.0.insert(0, SubField::Padding(230));
+        assert_eq!(
+            "ExtraField([Padding(230), TxPublicKey(06225b7ec0a6544d8da39abe68d8bd82619b4a7c5bdae89c3783b256a8fa4782), \
+             Nonce([246, 58, 168, 109, 46, 133, 127, 7])])",
+            &format!("{:?}", extra_field_before_parse)
+        );
+        assert!(ExtraField::try_parse(&extra_field_before_parse.clone().into()).is_err());
+
+        // Now insert the merge mining tag - this would also clean up the extra field and remove the invalid sub-fields
+        let hash = block_header.merge_mining_hash();
+        proxy_insert_merge_mining_tag_into_block(&mut block, hash).unwrap();
+        assert!(ExtraField::try_parse(&block.miner_tx.prefix.extra.clone()).is_ok());
+
+        // Verify that the merge mining tag is there
+        let extra_field_after_tag = ExtraField::try_parse(&block.miner_tx.prefix.extra.clone()).unwrap();
+        assert_eq!(
+            &format!(
+                "ExtraField([MergeMining(Some(0), 0x{}), \
+                 TxPublicKey(06225b7ec0a6544d8da39abe68d8bd82619b4a7c5bdae89c3783b256a8fa4782), Nonce([246, 58, 168, \
+                 109, 46, 133, 127, 7])])",
+                hex::encode(hash)
+            ),
+            &format!("{:?}", extra_field_after_tag)
+        );
+    }
+
     #[test]
     fn test_verify_header_no_coinbase() {
         let blocktemplate_blob = "0c0c8cd6a0fa057fe21d764e7abf004e975396a2160773b93712bf6118c3b4959ddd8ee0f76aad0000000002e1ea2701ffa5ea2701d5a299e2abb002028eb3066ced1b2cc82ea046f3716a48e9ae37144057d5fb48a97f941225a1957b2b0106225b7ec0a6544d8da39abe68d8bd82619b4a7c5bdae89c3783b256a8fa47820208f63aa86d2e857f070000".to_string();
@@ -656,7 +718,7 @@ mod test {
             validator_node_mr: FixedHash::zero(),
         };
         let hash = block_header.merge_mining_hash();
-        append_merge_mining_tag(&mut block, hash).unwrap();
+        proxy_insert_merge_mining_tag_into_block(&mut block, hash).unwrap();
         let count = 1 + (u16::try_from(block.tx_hashes.len()).unwrap());
         let mut hashes = Vec::with_capacity(count as usize);
         let mut proof = Vec::with_capacity(count as usize);
@@ -749,7 +811,7 @@ mod test {
             validator_node_mr: FixedHash::zero(),
         };
         let hash = block_header.merge_mining_hash();
-        append_merge_mining_tag(&mut block, hash).unwrap();
+        proxy_insert_merge_mining_tag_into_block(&mut block, hash).unwrap();
         let count = 1 + (u16::try_from(block.tx_hashes.len()).unwrap());
         let mut hashes = Vec::with_capacity(count as usize);
         let mut proof = Vec::with_capacity(count as usize);

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

@@ -24,12 +24,12 @@ pub use error::MergeMineError;
 
 mod helpers;
 pub use helpers::{
-    append_merge_mining_tag,
     construct_monero_data,
     create_blockhashing_blob_from_block,
     create_ordered_transaction_hashes_from_block,
     deserialize_monero_block_from_hex,
-    extract_tari_hash,
+    proxy_extract_tari_hash_from_block,
+    proxy_insert_merge_mining_tag_into_block,
     randomx_difficulty,
     serialize_monero_block_to_hex,
     verify_header,

base_layer/core/tests/tests/block_validation.rs

@@ -195,7 +195,7 @@ fn add_monero_data(tblock: &mut Block, seed_key: &str) {
     let bytes = hex::decode(blocktemplate_blob).unwrap();
     let mut mblock = monero_rx::deserialize::<MoneroBlock>(&bytes[..]).unwrap();
     let hash = tblock.header.merge_mining_hash();
-    monero_rx::append_merge_mining_tag(&mut mblock, hash).unwrap();
+    monero_rx::proxy_insert_merge_mining_tag_into_block(&mut mblock, hash).unwrap();
     let hashes = monero_rx::create_ordered_transaction_hashes_from_block(&mblock);
     let merkle_root = monero_rx::tree_hash(&hashes).unwrap();
     let coinbase_merkle_proof = monero_rx::create_merkle_proof(&hashes).unwrap();