fix: coinbase extra info is not checked (#4995)

This was initially a PR to change the field name of `metadata` in output features to `coinbase_extra`.

This is to reduce confusion, since at least one other field is labelled metadata and looking for this in the code is very confusing with this ambiguity.

However, in making the change, two tests were added:
* Check that *transactions* with coinbase_extra set are rejected as invalid. This turned out not to be the case, because output features are _only checked on orphan block validations_. This is a bug, because invalid transactions (from a stateless perspective) must be rejected as transactions, and not after they have been placed in a block.

* Check that the size of the coinbase_extra field is enforced. This does not appear to be done correctly, since adding a test in the general blockchain end-to-end test did not throw a block with an oversized extra field out.

This then uncovered a very odd definition in the tx weight calculation, which has been marked for clarification.

At this point, this PR is more of an issue, with breaking tests and comments. I'm looking for some clarification on those before making further changes.

applications/tari_app_grpc/proto/wallet.proto

@@ -236,6 +236,7 @@ message GetCoinbaseRequest {
     uint64 reward = 1;
     uint64 fee = 2;
     uint64 height = 3;
+    bytes extra = 4;
 }
 
 message GetCoinbaseResponse {
@@ -334,4 +335,4 @@ message RegisterValidatorNodeResponse {
     uint64 transaction_id = 1;
     bool is_success = 2;
     string failure_message = 3;
-}
+}

applications/tari_app_grpc/src/conversions/output_features.rs

@@ -64,7 +64,7 @@ impl From<OutputFeatures> for grpc::OutputFeatures {
             version: features.version as u32,
             output_type: u32::from(features.output_type.as_byte()),
             maturity: features.maturity,
-            metadata: features.metadata,
+            metadata: features.coinbase_extra,
             sidechain_feature: features.sidechain_feature.map(Into::into),
         }
     }

applications/tari_console_wallet/src/automation/commands.rs

@@ -1035,7 +1035,7 @@ fn write_utxos_to_csv_file(utxos: Vec<UnblindedOutput>, file_path: PathBuf) -> R
     let mut csv_file = LineWriter::new(file);
     writeln!(
         csv_file,
-        r##""index","value","spending_key","commitment","flags","maturity","metadata","script","covenant","input_data","script_private_key","sender_offset_public_key","ephemeral_commitment","ephemeral_nonce","signature_u_x","signature_u_a","signature_u_y","script_lock_height","encrypted_value","minimum_value_promise""##
+        r##""index","value","spending_key","commitment","flags","maturity","coinbase_extra","script","covenant","input_data","script_private_key","sender_offset_public_key","ephemeral_commitment","ephemeral_nonce","signature_u_x","signature_u_a","signature_u_y","script_lock_height","encrypted_value","minimum_value_promise""##
     )
     .map_err(|e| CommandError::CSVFile(e.to_string()))?;
     for (i, utxo) in utxos.iter().enumerate() {
@@ -1051,7 +1051,8 @@ fn write_utxos_to_csv_file(utxos: Vec<UnblindedOutput>, file_path: PathBuf) -> R
                 .to_hex(),
             utxo.features.output_type,
             utxo.features.maturity,
-            utxo.features.metadata.to_hex(),
+            String::from_utf8(utxo.features.coinbase_extra.clone())
+                .unwrap_or_else(|_| utxo.features.coinbase_extra.to_hex()),
             utxo.script.to_hex(),
             utxo.covenant.to_bytes().to_hex(),
             utxo.input_data.to_hex(),

applications/tari_console_wallet/src/grpc/wallet_grpc_server.rs

@@ -305,7 +305,7 @@ impl wallet_server::Wallet for WalletGrpcServer {
         let mut tx_service = self.get_transaction_service();
 
         let coinbase = tx_service
-            .generate_coinbase_transaction(request.reward.into(), request.fee.into(), request.height)
+            .generate_coinbase_transaction(request.reward.into(), request.fee.into(), request.height, request.extra)
             .await
             .map_err(|err| Status::unknown(err.to_string()))?;
 

applications/tari_merge_mining_proxy/src/block_template_protocol.rs

@@ -22,7 +22,7 @@
 
 //! Methods for seting up a new block.
 
-use std::cmp;
+use std::{cmp, sync::Arc};
 
 use log::*;
 use tari_base_node_grpc_client::{grpc, BaseNodeGrpcClient};
@@ -32,13 +32,15 @@ use tari_wallet_grpc_client::WalletGrpcClient;
 use crate::{
     block_template_data::{BlockTemplateData, BlockTemplateDataBuilder},
     common::merge_mining,
+    config::MergeMiningProxyConfig,
     error::MmProxyError,
 };
 
 const LOG_TARGET: &str = "tari_mm_proxy::proxy::block_template_protocol";
 
 /// Structure holding grpc connections.
 pub struct BlockTemplateProtocol<'a> {
+    config: Arc<MergeMiningProxyConfig>,
     base_node_client: &'a mut BaseNodeGrpcClient<tonic::transport::Channel>,
     wallet_client: &'a mut WalletGrpcClient<tonic::transport::Channel>,
 }
@@ -47,10 +49,12 @@ impl<'a> BlockTemplateProtocol<'a> {
     pub fn new(
         base_node_client: &'a mut BaseNodeGrpcClient<tonic::transport::Channel>,
         wallet_client: &'a mut WalletGrpcClient<tonic::transport::Channel>,
+        config: Arc<MergeMiningProxyConfig>,
     ) -> Self {
         Self {
             base_node_client,
             wallet_client,
+            config,
         }
     }
 }
@@ -180,13 +184,15 @@ impl BlockTemplateProtocol<'_> {
         let tari_height = template.height();
         let block_reward = miner_data.reward;
         let total_fees = miner_data.total_fees;
+        let extra = self.config.coinbase_extra.as_bytes().to_vec();
 
         let coinbase_response = self
             .wallet_client
             .get_coinbase(grpc::GetCoinbaseRequest {
                 reward: block_reward,
                 fee: total_fees,
                 height: tari_height,
+                extra,
             })
             .await
             .map_err(|status| MmProxyError::GrpcRequestError {

applications/tari_merge_mining_proxy/src/config.rs

@@ -60,6 +60,10 @@ pub struct MergeMiningProxyConfig {
     pub check_tari_difficulty_before_submit: bool,
     /// The maximum amount of VMs that RandomX will be use
     pub max_randomx_vms: usize,
+    /// The extra data to store in the coinbase, usually some data about the mining pool.
+    /// Note that this data is publicly readable, but it is suggested you populate it so that
+    /// pool dominance can be seen before any one party has more than 51%.
+    pub coinbase_extra: String,
 }
 
 impl Default for MergeMiningProxyConfig {
@@ -78,6 +82,7 @@ impl Default for MergeMiningProxyConfig {
             wait_for_initial_sync_at_startup: true,
             check_tari_difficulty_before_submit: true,
             max_randomx_vms: 5,
+            coinbase_extra: "tari_merge_mining_proxy".to_string(),
         }
     }
 }

applications/tari_merge_mining_proxy/src/proxy.rs

@@ -83,7 +83,7 @@ impl MergeMiningProxyService {
         debug!(target: LOG_TARGET, "Config: {:?}", config);
         Self {
             inner: InnerService {
-                config,
+                config: Arc::new(config),
                 block_templates,
                 http_client,
                 base_node_client,
@@ -154,7 +154,7 @@ impl Service<Request<Body>> for MergeMiningProxyService {
 
 #[derive(Debug, Clone)]
 struct InnerService {
-    config: MergeMiningProxyConfig,
+    config: Arc<MergeMiningProxyConfig>,
     block_templates: BlockTemplateRepository,
     http_client: reqwest::Client,
     base_node_client: BaseNodeGrpcClient<tonic::transport::Channel>,
@@ -423,7 +423,8 @@ impl InnerService {
             }
         }
 
-        let new_block_protocol = BlockTemplateProtocol::new(&mut grpc_client, &mut grpc_wallet_client);
+        let new_block_protocol =
+            BlockTemplateProtocol::new(&mut grpc_client, &mut grpc_wallet_client, self.config.clone());
 
         let seed_hash = FixedByteArray::from_hex(&monerod_resp["result"]["seed_hash"].to_string().replace('\"', ""))
             .map_err(|err| MmProxyError::InvalidMonerodResponse(format!("seed hash hex is invalid: {}", err)))?;

applications/tari_miner/src/config.rs

@@ -69,6 +69,10 @@ pub struct MinerConfig {
     pub mining_wallet_address: String,
     /// Stratum Mode configuration - mining worker name
     pub mining_worker_name: String,
+    /// The extra data to store in the coinbase, usually some data about the mining pool.
+    /// Note that this data is publicly readable, but it is suggested you populate it so that
+    /// pool dominance can be seen before any one party has more than 51%.
+    pub coinbase_extra: String,
 }
 
 #[derive(Serialize, Deserialize, Debug, Default)]
@@ -96,6 +100,7 @@ impl Default for MinerConfig {
             mining_pool_address: String::new(),
             mining_wallet_address: String::new(),
             mining_worker_name: String::new(),
+            coinbase_extra: "tari_miner".to_string(),
         }
     }
 }

applications/tari_miner/src/main.rs

@@ -248,7 +248,7 @@ async fn mining_cycle(
     }
 
     debug!(target: LOG_TARGET, "Getting coinbase");
-    let request = coinbase_request(&template)?;
+    let request = coinbase_request(&template, config.coinbase_extra.as_bytes().to_vec())?;
     let coinbase = wallet_conn.get_coinbase(request).await?.into_inner();
     let (output, kernel) = extract_outputs_and_kernels(coinbase)?;
     let body = block_template

applications/tari_miner/src/utils.rs

@@ -31,7 +31,10 @@ use tari_app_grpc::tari_rpc::{
 use crate::errors::{err_empty, MinerError};
 
 /// Convert NewBlockTemplateResponse to GetCoinbaseRequest
-pub fn coinbase_request(template_response: &NewBlockTemplateResponse) -> Result<GetCoinbaseRequest, MinerError> {
+pub fn coinbase_request(
+    template_response: &NewBlockTemplateResponse,
+    extra: Vec<u8>,
+) -> Result<GetCoinbaseRequest, MinerError> {
     let template = template_response
         .new_block_template
         .as_ref()
@@ -47,7 +50,12 @@ pub fn coinbase_request(template_response: &NewBlockTemplateResponse) -> Result<
         .as_ref()
         .ok_or_else(|| err_empty("template.header"))?
         .height;
-    Ok(GetCoinbaseRequest { reward, fee, height })
+    Ok(GetCoinbaseRequest {
+        reward,
+        fee,
+        height,
+        extra,
+    })
 }
 
 pub fn extract_outputs_and_kernels(

base_layer/core/src/blocks/block.rs

@@ -126,7 +126,7 @@ impl Block {
     /// 1. coinbase metadata length does not exceed its limit
     pub fn check_output_features(&self, consensus_constants: &ConsensusConstants) -> Result<(), BlockValidationError> {
         self.body
-            .check_output_features(consensus_constants.coinbase_output_features_metadata_max_length())?;
+            .check_output_features(consensus_constants.coinbase_output_features_extra_max_length())?;
 
         Ok(())
     }

base_layer/core/src/blocks/genesis_block.rs

@@ -296,7 +296,7 @@ fn get_esmeralda_genesis_block_raw() -> Block {
     );
     let coinbase = TransactionOutput::new(
         TransactionOutputVersion::get_current_version(),
-        OutputFeatures::create_coinbase(6),
+        OutputFeatures::create_coinbase(6, None),
         Commitment::from_hex("88f274d57be82db8fc4d3a2a246e31333df49c3b5cc042dcde5b3a9cae704963").unwrap(),
         BulletRangeProof::from_hex("01a225006336a27b35ed120cc25865278d44ee0b95e5fddaed9ab9253d32c8383cb4bb0458a1fca5e43308a1a6c8353dee1fb203c08bd35fb761d8b5d3edcb222cb8ba2bb19da42156a6cbb6e52c4ff9bafebe8651b0f747f0492c9814f5b8b534708eb83e4ece000135a3d9d724c57030a08172b439b85f0787f032f42accc433bc25c48449790dd94c447e2493034e834be5d76b3a2c571cd84a6dca0ed80359f6c8cd8c86a81e365f6b61fc9f37b70a96f046ebf05b3162e1ce31c9d39fee6f88a7804e98edb2c5bb985c43c54052b06b8276a9e7a56a75a5252f6bf53c5c69584e4c7348b00493fef802aae382533ca37dfa6081bb58a975ca21fa178d7b6b9e8abaa923270306c2e21a5c4ea6e8a9ee601be86f767e93ee0d30333c15b2799e5fba2c86a6b3a4a1d23801b4a9bbfcd2d888465b208a29e7f206986065471a2c86b69d889fd0c189e0217b668587270beb80cae15d5e5d0aa3558838506b29baabc9bd7b3aa51bde16ff9f9c7685c4921e1f4779e73c354a9286d0c78fa7306eceb6a089f8e27d9f250ecd87b55379e0f20ed5f26ee70b1d593aff3c30cd2ede759fba64dc7f307bee9a91c4085ee49ec40315d90d0d82998c084c5c177654ae09cb1b9b3bc302c6e4516dcc5eb965086a8edab1f019c09d2c194a5b148968eef94a5303e6aa0a6711d3ad186da96f4e9a502a6ed3e2545fcb6002e0f06604aa0aaf0681b272d5072fc2143ad4f2330e3883b68bf9f60fa0754b6e9992940646e422dac53da12ef0db5c684366f2a3ccbe34903c3e6a9660cfa227ee0e190a").unwrap(),
         // A default script can never be spent, intentionally

base_layer/core/src/consensus/consensus_constants.rs

@@ -97,7 +97,7 @@ pub struct ConsensusConstants {
     /// An allowlist of output types
     permitted_output_types: &'static [OutputType],
     /// Coinbase outputs are allowed to have metadata, but it has the following length limit
-    coinbase_output_features_metadata_max_length: usize,
+    coinbase_output_features_extra_max_length: u32,
     /// Epoch duration in blocks
     vn_epoch_length: u64,
     /// The number of Epochs that a validator node registration is valid
@@ -217,8 +217,8 @@ impl ConsensusConstants {
         self.transaction_weight.calculate(1, 0, 1, metadata_size)
     }
 
-    pub fn coinbase_output_features_metadata_max_length(&self) -> usize {
-        self.coinbase_output_features_metadata_max_length
+    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.
@@ -375,7 +375,7 @@ impl ConsensusConstants {
             vn_registration_min_deposit_amount: MicroTari(0),
             vn_registration_lock_height: 0,
             vn_registration_shuffle_interval: VnEpoch(100),
-            coinbase_output_features_metadata_max_length: 64,
+            coinbase_output_features_extra_max_length: 64,
         }]
     }
 
@@ -421,7 +421,7 @@ impl ConsensusConstants {
             vn_registration_min_deposit_amount: MicroTari(0),
             vn_registration_lock_height: 0,
             vn_registration_shuffle_interval: VnEpoch(100),
-            coinbase_output_features_metadata_max_length: 64,
+            coinbase_output_features_extra_max_length: 64,
         }]
     }
 
@@ -471,7 +471,7 @@ impl ConsensusConstants {
             vn_registration_min_deposit_amount: MicroTari(0),
             vn_registration_lock_height: 0,
             vn_registration_shuffle_interval: VnEpoch(100),
-            coinbase_output_features_metadata_max_length: 64,
+            coinbase_output_features_extra_max_length: 64,
         }]
     }
 
@@ -527,7 +527,7 @@ impl ConsensusConstants {
                 vn_registration_min_deposit_amount: MicroTari(0),
                 vn_registration_lock_height: 0,
                 vn_registration_shuffle_interval: VnEpoch(100),
-                coinbase_output_features_metadata_max_length: 64,
+                coinbase_output_features_extra_max_length: 64,
             },
             ConsensusConstants {
                 effective_from_height: 23000,
@@ -556,7 +556,7 @@ impl ConsensusConstants {
                 vn_registration_min_deposit_amount: MicroTari(0),
                 vn_registration_lock_height: 0,
                 vn_registration_shuffle_interval: VnEpoch(100),
-                coinbase_output_features_metadata_max_length: 64,
+                coinbase_output_features_extra_max_length: 64,
             },
         ]
     }
@@ -609,7 +609,7 @@ impl ConsensusConstants {
             vn_registration_min_deposit_amount: MicroTari(0),
             vn_registration_lock_height: 0,
             vn_registration_shuffle_interval: VnEpoch(100),
-            coinbase_output_features_metadata_max_length: 64,
+            coinbase_output_features_extra_max_length: 64,
         };
 
         vec![consensus_constants_1]
@@ -658,7 +658,7 @@ impl ConsensusConstants {
             vn_registration_min_deposit_amount: MicroTari(0),
             vn_registration_lock_height: 0,
             vn_registration_shuffle_interval: VnEpoch(100),
-            coinbase_output_features_metadata_max_length: 64,
+            coinbase_output_features_extra_max_length: 64,
         }]
     }
 

base_layer/core/src/covenants/byte_codes.rs

@@ -88,7 +88,7 @@ pub const FIELD_COVENANT: u8 = 0x03;
 pub const FIELD_FEATURES: u8 = 0x04;
 pub const FIELD_FEATURES_OUTPUT_TYPE: u8 = 0x05;
 pub const FIELD_FEATURES_MATURITY: u8 = 0x06;
-pub const FIELD_FEATURES_METADATA: u8 = 0x07;
+pub const FIELD_FEATURES_COINBASE_EXTRA: u8 = 0x07;
 pub const FIELD_FEATURES_SIDE_CHAIN_FEATURES: u8 = 0x08;
 
 #[cfg(test)]

base_layer/core/src/covenants/decoder.rs

@@ -185,7 +185,7 @@ mod test {
         unpack_enum!(CovenantArg::OutputFields(fields) = token.as_arg().unwrap());
         assert_eq!(fields.fields(), &[
             OutputField::Commitment,
-            OutputField::FeaturesMetadata
+            OutputField::FeaturesCoinbaseExtra
         ]);
 
         let token = decoder.next().unwrap().unwrap();