From 1bd2fded33153b5aa2b1442e3a70f1740f02db4a Mon Sep 17 00:00:00 2001 From: Aaron Feickert <66188213+AaronFeickert@users.noreply.github.com> Date: Mon, 22 Jul 2024 05:40:41 -0500 Subject: [PATCH] chore: remove unnecessary commitment hashing (#6416) Description --- Removes unnecessary hashing of commitments used in faucet-related signatures. Motivation and Context --- Signatures used for faucet functionality unnecessarily hash commitments before passing them as an input message. This may have been done on the assumption that the signing functions often refer to the message as a challenge, despite this not being the case. This PR removes the unnecessary hashing operations. It may be worthwhile to refactor the associated signing functions to avoid any possible confusion about the nature of messages and challenges (as suggested in #6418). How Has This Been Tested? --- Existing tests pass. What process can a PR reviewer use to test or verify this change? --- Check that the commitment data is being passed into the signing functions correctly, and that those functions do in face assume the input is an arbitrary message and not a challenge. --- .../src/automation/commands.rs | 12 ++---------- base_layer/core/src/blocks/faucets/mod.rs | 14 +++++--------- base_layer/core/src/common/one_sided.rs | 2 -- .../wallet/src/output_manager_service/service.rs | 12 ++---------- 4 files changed, 9 insertions(+), 31 deletions(-) diff --git a/applications/minotari_console_wallet/src/automation/commands.rs b/applications/minotari_console_wallet/src/automation/commands.rs index af6ba05732..83a6ceb329 100644 --- a/applications/minotari_console_wallet/src/automation/commands.rs +++ b/applications/minotari_console_wallet/src/automation/commands.rs @@ -31,9 +31,8 @@ use std::{ time::{Duration, Instant}, }; -use blake2::Blake2b; use chrono::{DateTime, Utc}; -use digest::{consts::U32, crypto_common::rand_core::OsRng, Digest}; +use digest::{crypto_common::rand_core::OsRng, Digest}; use futures::FutureExt; use log::*; use minotari_app_grpc::tls::certs::{generate_self_signed_certs, print_warning, write_cert_to_disk}; @@ -64,9 +63,7 @@ use tari_comms::{ }; use tari_comms_dht::{envelope::NodeDestination, DhtDiscoveryRequester}; use tari_core::{ - consensus::DomainSeparatedConsensusHasher, covenants::Covenant, - one_sided::FaucetHashDomain, transactions::{ key_manager::TransactionKeyManagerInterface, tari_amount::{uT, MicroMinotari, Minotari}, @@ -816,11 +813,6 @@ pub async fn command_runner( let session_info = read_session_info(args.input_file.clone())?; let commitment = Commitment::from_hex(&session_info.commitment_to_spend)?; - let commitment_hash: [u8; 32] = - DomainSeparatedConsensusHasher::>::new("com_hash") - .chain(&commitment) - .finalize() - .into(); let shared_secret = key_manager_service .get_diffie_hellman_shared_secret( &sender_offset_key.key_id, @@ -833,7 +825,7 @@ pub async fn command_runner( let shared_secret_public_key = PublicKey::from_canonical_bytes(shared_secret.as_bytes())?; let script_input_signature = key_manager_service - .sign_script_message(&wallet_spend_key.key_id, &commitment_hash) + .sign_script_message(&wallet_spend_key.key_id, commitment.as_bytes()) .await?; let out_dir = out_dir(&session_info.session_id)?; diff --git a/base_layer/core/src/blocks/faucets/mod.rs b/base_layer/core/src/blocks/faucets/mod.rs index 88dd787894..103a56e52d 100644 --- a/base_layer/core/src/blocks/faucets/mod.rs +++ b/base_layer/core/src/blocks/faucets/mod.rs @@ -24,8 +24,6 @@ mod test { use std::{convert::TryFrom, fs::File, io::Write}; - use blake2::Blake2b; - use digest::consts::U32; use rand::rngs::OsRng; use tari_common_types::{ tari_address::TariAddress, @@ -34,10 +32,10 @@ mod test { use tari_crypto::keys::{PublicKey as PkTrait, SecretKey as SkTrait}; use tari_key_manager::key_manager_service::KeyManagerInterface; use tari_script::{ExecutionStack, Opcode::CheckMultiSigVerifyAggregatePubKey, TariScript}; + use tari_utilities::ByteArray; use crate::{ - consensus::DomainSeparatedConsensusHasher, - one_sided::{public_key_to_output_encryption_key, FaucetHashDomain}, + one_sided::public_key_to_output_encryption_key, transactions::{ key_manager::{ create_memory_db_key_manager, @@ -91,10 +89,8 @@ mod test { .get_commitment(&commitment_mask.key_id, &amount.into()) .await .unwrap(); - let com_hash: [u8; 32] = DomainSeparatedConsensusHasher::>::new("com_hash") - .chain(&commitment) - .finalize() - .into(); + let mut commitment_bytes = [0u8; 32]; + commitment_bytes.clone_from_slice(commitment.as_bytes()); let sender_offset = key_manager .get_next_key(TransactionKeyManagerBranch::SenderOffset.get_branch_key()) @@ -104,7 +100,7 @@ mod test { signature_threshold, address_len, list_of_spend_keys.clone(), - Box::new(com_hash), + Box::new(commitment_bytes), )]); let output = WalletOutputBuilder::new(amount, commitment_mask.key_id) .with_features(OutputFeatures::new( diff --git a/base_layer/core/src/common/one_sided.rs b/base_layer/core/src/common/one_sided.rs index 17d59945d1..8bd8488a6c 100644 --- a/base_layer/core/src/common/one_sided.rs +++ b/base_layer/core/src/common/one_sided.rs @@ -44,8 +44,6 @@ hash_domain!( 1 ); -hash_domain!(FaucetHashDomain, "com.tari.base_layer.wallet.faucet", 0); - type WalletOutputEncryptionKeysDomainHasher = DomainSeparatedHasher, WalletOutputEncryptionKeysDomain>; type WalletOutputSpendingKeysDomainHasher = DomainSeparatedHasher, WalletOutputSpendingKeysDomain>; diff --git a/base_layer/wallet/src/output_manager_service/service.rs b/base_layer/wallet/src/output_manager_service/service.rs index 9421218452..b95f0fe44c 100644 --- a/base_layer/wallet/src/output_manager_service/service.rs +++ b/base_layer/wallet/src/output_manager_service/service.rs @@ -22,9 +22,7 @@ use std::{collections::HashMap, convert::TryInto, fmt, sync::Arc}; -use blake2::Blake2b; use diesel::result::{DatabaseErrorKind, Error as DieselError}; -use digest::consts::U32; use futures::{pin_mut, StreamExt}; use log::*; use rand::{rngs::OsRng, RngCore}; @@ -36,14 +34,13 @@ use tari_common_types::{ use tari_comms::types::CommsDHKE; use tari_core::{ borsh::SerializedSize, - consensus::{ConsensusConstants, DomainSeparatedConsensusHasher}, + consensus::ConsensusConstants, covenants::Covenant, one_sided::{ public_key_to_output_encryption_key, shared_secret_to_output_encryption_key, shared_secret_to_output_spending_key, stealth_address_script_spending_key, - FaucetHashDomain, }, proto::base_node::FetchMatchingUtxos, transactions::{ @@ -1237,17 +1234,12 @@ where if output.verify_mask(&self.resources.factories.range_proof, &spending_key, amount.as_u64())? { let mut script_signatures = Vec::new(); // lets add our own signature to the list - let script_challange: [u8; 32] = - DomainSeparatedConsensusHasher::>::new("com_hash") - .chain(output.commitment()) - .finalize() - .into(); let self_signature = self .resources .key_manager .sign_script_message( &self.resources.key_manager.get_spend_key().await?.key_id, - &script_challange, + output.commitment.as_bytes(), ) .await?; script_input_shares.insert(