Skip to content

Commit

Permalink
feat: remove extra validation (#4981)
Browse files Browse the repository at this point in the history
Description
---
This removes extra validation from transaction protocols. 

Motivation and Context
---
The transaction protocols do a lot of extra unnecessary validation. 
For example:
The code will run add_output(output), which will validate the range_proof and signatures. 
The next line will run tx.build(), which will again validate the range_proof and signatures. 
While these tests are not expensive, they are not free either. 

The second problem is that when we do aggregate transactions such as with branch: `feature-m-of-n` these validations fail as the signatures are partial until right before we build the final transaction

How Has This Been Tested?
---
Passes all unit tests

Fixes: #4813
  • Loading branch information
SWvheerden authored Dec 1, 2022
1 parent bcc47e1 commit 3f1ebf6
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 94 deletions.
63 changes: 7 additions & 56 deletions base_layer/core/src/transactions/transaction_protocol/sender.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,10 @@
use std::fmt;

use derivative::Derivative;
use rand::rngs::OsRng;
use serde::{Deserialize, Serialize};
use tari_common_types::{
transaction::TxId,
types::{
BlindingFactor,
ComAndPubSignature,
CommitmentFactory,
HashOutput,
PrivateKey,
PublicKey,
RangeProofService,
Signature,
},
types::{BlindingFactor, ComAndPubSignature, HashOutput, PrivateKey, PublicKey, Signature},
};
use tari_crypto::{
keys::PublicKey as PublicKeyTrait,
Expand Down Expand Up @@ -440,14 +430,9 @@ impl SenderTransactionProtocol {
}

/// Add the signed transaction from the recipient and move to the next state
pub fn add_single_recipient_info(
&mut self,
rec: RecipientSignedMessage,
prover: &RangeProofService,
) -> Result<(), TPE> {
pub fn add_single_recipient_info(&mut self, rec: RecipientSignedMessage) -> Result<(), TPE> {
match &mut self.state {
SenderState::CollectingSingleSignature(info) => {
rec.output.verify_range_proof(prover)?;
// Consolidate transaction info
info.outputs.push(rec.output.clone());

Expand All @@ -470,7 +455,6 @@ impl SenderTransactionProtocol {
private_commitment_nonce,
recipient_sender_offset_private_key,
&info.outputs[index].clone(),
&CommitmentFactory::default(),
)?;
}

Expand All @@ -490,10 +474,8 @@ impl SenderTransactionProtocol {
private_commitment_nonce: &PrivateKey,
sender_offset_private_key: &PrivateKey,
output: &TransactionOutput,
commitment_factory: &CommitmentFactory,
) -> Result<ComAndPubSignature, TPE> {
// Create sender signature
let ephemeral_public_nonce = PublicKey::from_secret_key(private_commitment_nonce);
let sender_signature = TransactionOutput::create_sender_partial_metadata_signature(
output.version,
&output.commitment,
Expand All @@ -508,33 +490,8 @@ impl SenderTransactionProtocol {
)?;
// Create aggregated metadata signature
let aggregated_metadata_signature = &sender_signature + &output.metadata_signature;
// lets verify everything is fine
let e = TransactionOutput::build_metadata_signature_challenge(
output.version,
&output.script,
&output.features,
&output.sender_offset_public_key,
output.metadata_signature.ephemeral_commitment(),
&ephemeral_public_nonce,
&output.commitment,
&output.covenant,
&output.encrypted_value,
output.minimum_value_promise,
);
if aggregated_metadata_signature.verify_challenge(
&output.commitment,
&output.sender_offset_public_key,
&e,
commitment_factory,
&mut OsRng,
) {
Ok(aggregated_metadata_signature)
} else {
Err(TPE::InvalidSignatureError(format!(
"Transaction output metadata signature not valid for {}",
output
)))
}

Ok(aggregated_metadata_signature)
}

/// Attempts to build the final transaction.
Expand Down Expand Up @@ -1052,9 +1009,7 @@ mod test {
// Receiver gets message, deserializes it etc, and creates his response
let mut bob_info =
SingleReceiverTransactionProtocol::create(&msg, b.nonce, b.spend_key, &factories, None).unwrap(); // Alice gets message back, deserializes it, etc
alice
.add_single_recipient_info(bob_info.clone(), &factories.range_proof)
.unwrap();
alice.add_single_recipient_info(bob_info.clone()).unwrap();
// Transaction should be complete
assert!(alice.is_finalizing());
match alice.finalize(&factories, None, u64::MAX) {
Expand Down Expand Up @@ -1135,9 +1090,7 @@ mod test {
bob_info.output.commitment.as_public_key().to_hex()
);
// Alice gets message back, deserializes it, etc
alice
.add_single_recipient_info(bob_info, &factories.range_proof)
.unwrap();
alice.add_single_recipient_info(bob_info).unwrap();
// Transaction should be complete
assert!(alice.is_finalizing());
match alice.finalize(&factories, None, u64::MAX) {
Expand Down Expand Up @@ -1336,9 +1289,7 @@ mod test {
.unwrap();

// Alice gets message back, deserializes it, etc
alice
.add_single_recipient_info(bob_info, &factories.range_proof)
.unwrap();
alice.add_single_recipient_info(bob_info).unwrap();
// Transaction should be complete
assert!(alice.is_finalizing());
match alice.finalize(&factories, None, u64::MAX) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use log::*;
use rand::rngs::OsRng;
use tari_common_types::{
transaction::TxId,
types::{BlindingFactor, Commitment, CommitmentFactory, HashOutput, PrivateKey, PublicKey},
types::{BlindingFactor, Commitment, HashOutput, PrivateKey, PublicKey},
};
use tari_crypto::{
commitment::HomomorphicCommitmentFactory,
Expand Down Expand Up @@ -223,32 +223,6 @@ impl SenderTransactionInitializer {
output: UnblindedOutput,
sender_offset_private_key: PrivateKey,
) -> Result<&mut Self, BuildError> {
let commitment_factory = CommitmentFactory::default();
let commitment = commitment_factory.commit(&output.spending_key, &PrivateKey::from(output.value));
let e = TransactionOutput::build_metadata_signature_challenge(
output.version,
&output.script,
&output.features,
&output.sender_offset_public_key,
output.metadata_signature.ephemeral_commitment(),
output.metadata_signature.ephemeral_pubkey(),
&commitment,
&output.covenant,
&output.encrypted_value,
output.minimum_value_promise,
);
if !output.metadata_signature.verify_challenge(
&commitment,
&output.sender_offset_public_key,
&e,
&commitment_factory,
&mut OsRng,
) {
return self.clone().build_err(&*format!(
"Metadata signature not valid, cannot add output: {:?}",
output
))?;
}
self.excess_blinding_factor = &self.excess_blinding_factor + &output.spending_key;
self.sender_custom_outputs.push(output);
self.sender_offset_private_keys.push(sender_offset_private_key);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,7 @@ where

outbound_tx
.sender_protocol
.add_single_recipient_info(recipient_reply, &self.resources.factories.range_proof)
.add_single_recipient_info(recipient_reply)
.map_err(|e| TransactionServiceProtocolError::new(self.id, TransactionServiceError::from(e)))?;

outbound_tx
Expand Down
6 changes: 3 additions & 3 deletions base_layer/wallet/src/transaction_service/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1141,7 +1141,7 @@ where

// Start finalizing

stp.add_single_recipient_info(recipient_reply, &self.resources.factories.range_proof)
stp.add_single_recipient_info(recipient_reply)
.map_err(|e| TransactionServiceProtocolError::new(tx_id, e.into()))?;

// Finalize
Expand Down Expand Up @@ -1278,7 +1278,7 @@ where

// Start finalizing

stp.add_single_recipient_info(recipient_reply, &self.resources.factories.range_proof)
stp.add_single_recipient_info(recipient_reply)
.map_err(|e| TransactionServiceProtocolError::new(tx_id, e.into()))?;

// Finalize
Expand Down Expand Up @@ -1430,7 +1430,7 @@ where

// Start finalizing

stp.add_single_recipient_info(recipient_reply, &self.resources.factories.range_proof)
stp.add_single_recipient_info(recipient_reply)
.map_err(|e| TransactionServiceProtocolError::new(tx_id, e.into()))?;

// Finalize
Expand Down
10 changes: 3 additions & 7 deletions base_layer/wallet/tests/transaction_service_tests/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1505,8 +1505,7 @@ async fn finalize_tx_with_incorrect_pubkey() {
.try_into()
.unwrap();

stp.add_single_recipient_info(recipient_reply.clone(), &factories.range_proof)
.unwrap();
stp.add_single_recipient_info(recipient_reply.clone()).unwrap();
stp.finalize(&factories, None, u64::MAX).unwrap();
let tx = stp.get_transaction().unwrap();

Expand Down Expand Up @@ -1620,8 +1619,7 @@ async fn finalize_tx_with_missing_output() {
.try_into()
.unwrap();

stp.add_single_recipient_info(recipient_reply.clone(), &factories.range_proof)
.unwrap();
stp.add_single_recipient_info(recipient_reply.clone()).unwrap();
stp.finalize(&factories, None, u64::MAX).unwrap();

let finalized_transaction_message = proto::TransactionFinalizedMessage {
Expand Down Expand Up @@ -2963,9 +2961,7 @@ async fn test_restarting_transaction_protocols() {

let alice_reply = receiver_protocol.get_signed_data().unwrap().clone();

bob_stp
.add_single_recipient_info(alice_reply.clone(), &factories.range_proof)
.unwrap();
bob_stp.add_single_recipient_info(alice_reply.clone()).unwrap();

match bob_stp.finalize(&factories, None, u64::MAX) {
Ok(_) => (),
Expand Down

0 comments on commit 3f1ebf6

Please sign in to comment.