feat: add robustness to monero block extra field handling (#5826)

Description
---
Added robustness to editing and parsing the extra field in a Monero
block. Use cases for the extra field are:
- the extra field is changed to add a merge mining hash subfield
(_performed by the merge mining proxy_) - **NOT applicable, verification
is not compromised**
- the merge mining hash is extracted from the extra field (_performed by
the merge mining proxy_) - **applicable**
- a Monero block header is verified - **NOT applicable, verification is
not compromised**

As per Monero consensus rules, a parsing error with
`ExtraField::try_parse(raw_extra_field)` will not represent a failure to
de-serialize a block, as, upon error, it will just return the subfields
that could be read. This means that when a Monero block template is
requested by the merge mining proxy it can contain an "invalid" extra
field, mainly due to the presence of a variable length
`SubField::Padding(n)` with `n < 255` if not the last subfield in the
extra field.

If we append the merge mining tag subfield to a valid existing extra
field with a variable `SubField::Padding(n)` with `n < 255` as the last
subfield, the new extra field cannot be parsed successfully. To
circumvent this, we insert the merge mining subfield in front of the
subfield array instead of appending it at the rear.

In the event that we have an invalid extra field to start with, we can
now parse the extra field and clean it up in the same process before
inserting the merge mining subfield. When extracting the merge mining
hash we now also ignore an invalid extra field and return the merge
mining hash if present.

Having more than one merge mining tag subfield in a block is currently
not allowed in the Tari code base, so this is now checked prior to
adding the merge mining tag subfield.


Motivation and Context
---
See above

How Has This Been Tested?
---
- Pass existing tests
- Extended unit test `test_duplicate_append_mm_tag` to verify that
appending more than one merge mining tag will be blocked prior to
validating the Monero block header and that header validation will still
fail if an additional merge mining tag is present.
- Added unit test `fn test_extra_field_with_parsing_error()`

What process can a PR reviewer use to test or verify this change?
---
Code walkthrough
Familiarize the monero.rs code base

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->

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();