diff --git a/applications/minotari_merge_mining_proxy/src/block_template_protocol.rs b/applications/minotari_merge_mining_proxy/src/block_template_protocol.rs index c11caaaa018..d6e3e0eac66 100644 --- a/applications/minotari_merge_mining_proxy/src/block_template_protocol.rs +++ b/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 diff --git a/applications/minotari_merge_mining_proxy/src/proxy.rs b/applications/minotari_merge_mining_proxy/src/proxy.rs index af8d5389083..ec88a7f39c0 100644 --- a/applications/minotari_merge_mining_proxy/src/proxy.rs +++ b/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()) })?; diff --git a/base_layer/core/src/proof_of_work/monero_rx/helpers.rs b/base_layer/core/src/proof_of_work/monero_rx/helpers.rs index 76f91fd494c..234a4baef4e 100644 --- a/base_layer/core/src/proof_of_work/monero_rx/helpers.rs +++ b/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 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 Result, 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, 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,8 +198,11 @@ 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>(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>( + 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", @@ -194,13 +210,8 @@ pub fn append_merge_mining_tag>(block: &mut monero::Block, hash: 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>(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::(&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); diff --git a/base_layer/core/src/proof_of_work/monero_rx/mod.rs b/base_layer/core/src/proof_of_work/monero_rx/mod.rs index 0f17ca0b83e..c33a73cef34 100644 --- a/base_layer/core/src/proof_of_work/monero_rx/mod.rs +++ b/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, diff --git a/base_layer/core/tests/tests/block_validation.rs b/base_layer/core/tests/tests/block_validation.rs index 7b702a20b0a..925f3ec895d 100644 --- a/base_layer/core/tests/tests/block_validation.rs +++ b/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::(&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();