feat: add robustness to monero block extra field handling

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::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::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
@@ -77,8 +94,8 @@ pub fn verify_header(header: &BlockHeader) -> Result<MoneroPowData, MergeMineErr
     let expected_merge_mining_hash = header.merge_mining_hash();
     let extra_field = ExtraField::try_parse(&monero_data.coinbase_tx.prefix.extra)
         .map_err(|_| MergeMineError::DeserializeError("Invalid 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;
@@ -110,15 +127,33 @@ 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> {
-    let extra_field = ExtraField::try_parse(&monero.miner_tx.prefix.extra)
-        .map_err(|_| MergeMineError::DeserializeError("Invalid extra field".to_string()))?;
-    for item in &extra_field.0 {
-        if let SubField::MergeMining(_depth, merge_mining_hash) = item {
-            return Ok(Some(*merge_mining_hash));
-        }
+pub fn 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);
+
+    // Only one merge mining tag is allowed
+    let merge_mining_hashes: Vec<monero::Hash> = extra_field
+        .0
+        .iter()
+        .filter_map(|item| {
+            if let SubField::MergeMining(_depth, merge_mining_hash) = item {
+                Some(*merge_mining_hash)
+            } else {
+                None
+            }
+        })
+        .collect();
+    if merge_mining_hashes.len() > 1 {
+        return Err(MergeMineError::ValidationError(
+            "More than one merge mining tag found in coinbase".to_string(),
+        ));
+    }
+
+    if let Some(merge_mining_hash) = merge_mining_hashes.into_iter().next() {
+        Ok(Some(merge_mining_hash))
+    } else {
+        Ok(None)
     }
-    Ok(None)
 }
 
 /// Deserializes the given hex-encoded string into a Monero block
@@ -174,20 +209,38 @@ 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 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()
         )));
     }
-    let hash = monero::Hash::from_slice(hash.as_ref());
-    let mm_tag = SubField::MergeMining(Some(VarInt(0)), hash);
+    // When we insert the merge mining tag, we need to make sure that the extra field is valid.
     let mut extra_field = ExtraField::try_parse(&block.miner_tx.prefix.extra)
         .map_err(|_| MergeMineError::DeserializeError("Invalid extra field".to_string()))?;
-    extra_field.0.push(mm_tag);
+
+    // Adding more than one merge mining tag is not allowed
+    for item in &extra_field.0 {
+        if let SubField::MergeMining(Some(_), _) = item {
+            return Err(MergeMineError::ValidationError(
+                "More than one merge mining tag in coinbase not allowed".to_string(),
+            ));
+        }
+    }
+
+    // If `SubField::Padding(n)` with `n < 255` is the last sub field in the extra field, then appending a new field
+    // will always fail deserialization (`ExtraField::try_parse`) - the new field cannot be parsed in that sequence.
+    // 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());
+    extra_field.0.insert(0, SubField::MergeMining(Some(VarInt(0)), hash));
+
     block.miner_tx.prefix.extra = extra_field.into();
     Ok(())
 }
@@ -337,7 +390,7 @@ mod test {
             validator_node_mr: FixedHash::zero(),
         };
         let hash = block_header.merge_mining_hash();
-        append_merge_mining_tag(&mut block, hash).unwrap();
+        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();
@@ -393,7 +446,7 @@ mod test {
             validator_node_mr: FixedHash::zero(),
         };
         let hash = block_header.merge_mining_hash();
-        append_merge_mining_tag(&mut block, hash).unwrap();
+        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());
@@ -496,7 +549,7 @@ mod test {
             validator_node_mr: FixedHash::zero(),
         };
         let hash = Hash::null();
-        append_merge_mining_tag(&mut block, hash).unwrap();
+        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);
@@ -551,12 +604,28 @@ mod test {
             validator_node_mr: FixedHash::zero(),
         };
         let hash = block_header.merge_mining_hash();
-        append_merge_mining_tag(&mut block, hash).unwrap();
+        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();
-        append_merge_mining_tag(&mut block, hash2).unwrap();
+        assert!(extract_tari_hash_from_block(&block).is_ok());
+
+        // Try via the API - this will fail because more than one merge mining tag is not allowed
+        assert!(insert_merge_mining_tag_into_block(&mut block, hash2).is_err());
+
+        // 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());
+        extra_field.0.insert(0, SubField::MergeMining(Some(VarInt(0)), hash));
+        block.miner_tx.prefix.extra = extra_field.into();
+
+        // Trying to extract the Tari hash will fail because there are more than one merge mining tag
+        let err = extract_tari_hash_from_block(&block).unwrap_err();
+        unpack_enum!(MergeMineError::ValidationError(details) = err);
+        assert!(details.contains("More than one merge mining tag found in coinbase"));
+
         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());
@@ -582,11 +651,70 @@ mod test {
             pow_data: serialized,
         };
         block_header.pow = pow;
+
+        // Header verification will fail because there are more than one merge mining tag
         let err = verify_header(&block_header).unwrap_err();
         unpack_enum!(MergeMineError::ValidationError(details) = err);
         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();
+        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();
@@ -610,7 +738,7 @@ mod test {
             validator_node_mr: FixedHash::zero(),
         };
         let hash = block_header.merge_mining_hash();
-        append_merge_mining_tag(&mut block, hash).unwrap();
+        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);
@@ -703,7 +831,7 @@ mod test {
             validator_node_mr: FixedHash::zero(),
         };
         let hash = block_header.merge_mining_hash();
-        append_merge_mining_tag(&mut block, hash).unwrap();
+        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,
+    extract_tari_hash_from_block,
+    insert_merge_mining_tag_into_block,
     randomx_difficulty,
     serialize_monero_block_to_hex,
     verify_header,

base_layer/core/tests/tests/block_validation.rs

@@ -200,7 +200,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::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();