Skip to content

Commit

Permalink
fix(core): remove implicit change in protocol for partial/full signat…
Browse files Browse the repository at this point in the history 
…ures (#5488)

Description
---
Removes check for an invalid signature, presumed to be a partial
signature, which results in partial signature aggregation being skipped
if a valid signature is passed in.

Motivation and Context
---
This check validates the receiver signature to determine if the
signature is sent is partial or full. If partial we aggregate the sender
and receiver partial signatures. A partial signature and an invalid
signature run the same code path (` if
received_output.verify_metadata_signature().is_err() {`) This code
branch is executed as part of every normal Tari transaction.

It is not possible for the (non-local) receiver to unilaterally produce
a correct signature and "bypass" aggregation as they do not possess the
required secrets. The primary reason for this check is to allow the
protocol to be shortcut when the sender and receiver are the same party
and therefore can produce a valid signature
(sign_as_sender_and_receiver). This PR implements this shortcut
explicitly by adding a separate call for this case
`add_presigned_recipient_info`.

How Has This Been Tested?
---
Existing transaction tests.

What process can a PR reviewer use to test or verify this change?
---
Send a normal and send-to-self transaction
<!-- 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 -->
  • Loading branch information
@sdbondi
sdbondi committed Jun 23, 2023
1 parent d128f85 commit fef701e
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 77 deletions.
168 changes: 98 additions & 70 deletions base_layer/core/src/transactions/transaction_protocol/sender.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use std::fmt;
use serde::{Deserialize, Serialize};
use tari_common_types::{
transaction::TxId,
types::{PrivateKey, PublicKey, Signature},
types::{ComAndPubSignature, PrivateKey, PublicKey, Signature},
};
use tari_crypto::{ristretto::pedersen::PedersenCommitment, tari_utilities::ByteArray};
pub use tari_key_manager::key_manager_service::KeyId;
Expand Down Expand Up @@ -93,6 +93,20 @@ pub(super) struct RawTransactionInfo {
pub text_message: String,
}

impl RawTransactionInfo {
pub fn add_recipient_signed_message(&mut self, msg: RecipientSignedMessage) {
let received_output = msg.output;
self.recipient_partial_kernel_excess = msg.public_spend_key;
self.recipient_partial_kernel_signature = msg.partial_signature;
self.recipient_partial_kernel_offset = msg.offset;
if self.metadata.kernel_features.is_burned() {
self.metadata.burn_commitment = Some(received_output.commitment.clone());
}

self.recipient_output = Some(received_output);
}
}

#[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize)]
pub struct SingleRoundSenderData {
/// The transaction id for the recipient
Expand Down Expand Up @@ -434,86 +448,100 @@ impl SenderTransactionProtocol {
Ok((public_nonce, public_excess))
}

/// Add the signed transaction from the recipient and move to the next state
/// Add partial signatures, add the the recipient info to sender state and move to the Finalizing state
pub async fn add_single_recipient_info<KM: TransactionKeyManagerInterface>(
&mut self,
rec: RecipientSignedMessage,
mut rec: RecipientSignedMessage,
key_manager: &KM,
) -> Result<(), TPE> {
match &mut self.state {
SenderState::CollectingSingleSignature(info) => {
match self.state {
SenderState::CollectingSingleSignature(ref info) => {
let mut info = info.clone();
// Add sender signature to recipient partial signature
rec.output.metadata_signature = self.add_sender_partial_signature(&rec, &info, key_manager).await?;
// Consolidate transaction info
let mut received_output = rec.output.clone();
if received_output.verify_metadata_signature().is_err() {
// we need to make sure we use our values here and not the received values.
let metadata_message = TransactionOutput::metadata_signature_message_from_parts(
&received_output.version,
&received_output.script, /* receiver chooses script here, can change fee per gram see issue: https://github.com/tari-project/tari/issues/5430 */
&info
.recipient_data
.as_ref()
.ok_or_else(|| {
TPE::IncompleteStateError("Missing data `recipient_output_features`".to_string())
})?
.recipient_output_features,
&info
.recipient_data
.as_ref()
.ok_or_else(|| TPE::IncompleteStateError("Missing data `recipient_covenant`".to_string()))?
.recipient_covenant,
&received_output.encrypted_data,
&info
.recipient_data
.as_ref()
.ok_or_else(|| {
TPE::IncompleteStateError("Missing data 'recipient_minimum_value_promise'".to_string())
})?
.recipient_minimum_value_promise,
);
let ephemeral_public_key_nonce = info
.recipient_data
.as_ref()
.ok_or_else(|| {
TPE::IncompleteStateError("Missing data `recipient_ephemeral_public_key_nonce`".to_string())
})?
.recipient_ephemeral_public_key_nonce
.clone();
let recipient_sender_offset_key_id = info
.recipient_data
.as_ref()
.ok_or_else(|| {
TPE::IncompleteStateError("Missing data `recipient_sender_offset_key_id`".to_string())
})?
.recipient_sender_offset_key_id
.clone();
let sender_metadata_signature = key_manager
.get_sender_partial_metadata_signature(
&ephemeral_public_key_nonce,
&recipient_sender_offset_key_id,
&received_output.commitment,
received_output.metadata_signature.ephemeral_commitment(),
&received_output.version,
&metadata_message,
)
.await?;
received_output.metadata_signature =
&received_output.metadata_signature + &sender_metadata_signature;
}
info.recipient_output = Some(received_output.clone());
info.recipient_partial_kernel_excess = rec.public_spend_key;
info.recipient_partial_kernel_signature = rec.partial_signature;
info.recipient_partial_kernel_offset = rec.offset;
if info.metadata.kernel_features.is_burned() {
info.metadata.burn_commitment = Some(received_output.commitment);
};

self.state = SenderState::Finalizing(info.clone());
info.add_recipient_signed_message(rec);
self.state = SenderState::Finalizing(info);
Ok(())
},
_ => Err(TPE::InvalidStateError),
}
}

/// Add the recipient info to sender state and move to the Finalizing state. This method does not add the sender
/// partial signature to the final signature. Use this if the sender and receiver are the same party and the
/// signature is already complete.
pub fn add_presigned_recipient_info(&mut self, rec: RecipientSignedMessage) -> Result<(), TPE> {
match self.state {
SenderState::CollectingSingleSignature(ref info) => {
let mut info = info.clone();
// Consolidate transaction info
info.add_recipient_signed_message(rec);

self.state = SenderState::Finalizing(info);
Ok(())
},
_ => Err(TPE::InvalidStateError),
}
}

async fn add_sender_partial_signature<KM: TransactionKeyManagerInterface>(
&self,
rec: &RecipientSignedMessage,
info: &RawTransactionInfo,
key_manager: &KM,
) -> Result<ComAndPubSignature, TPE> {
let received_output = &rec.output;
// we need to make sure we use our values here and not the received values.
let metadata_message = TransactionOutput::metadata_signature_message_from_parts(
&received_output.version,
&received_output.script, /* receiver chooses script here, can change fee per gram see issue: https://github.com/tari-project/tari/issues/5430 */
&info
.recipient_data
.as_ref()
.ok_or_else(|| TPE::IncompleteStateError("Missing data `recipient_output_features`".to_string()))?
.recipient_output_features,
&info
.recipient_data
.as_ref()
.ok_or_else(|| TPE::IncompleteStateError("Missing data `recipient_covenant`".to_string()))?
.recipient_covenant,
&received_output.encrypted_data,
&info
.recipient_data
.as_ref()
.ok_or_else(|| TPE::IncompleteStateError("Missing data 'recipient_minimum_value_promise'".to_string()))?
.recipient_minimum_value_promise,
);
let ephemeral_public_key_nonce = info
.recipient_data
.as_ref()
.ok_or_else(|| {
TPE::IncompleteStateError("Missing data `recipient_ephemeral_public_key_nonce`".to_string())
})?
.recipient_ephemeral_public_key_nonce
.clone();
let recipient_sender_offset_key_id = info
.recipient_data
.as_ref()
.ok_or_else(|| TPE::IncompleteStateError("Missing data `recipient_sender_offset_key_id`".to_string()))?
.recipient_sender_offset_key_id
.clone();
let sender_metadata_signature = key_manager
.get_sender_partial_metadata_signature(
&ephemeral_public_key_nonce,
&recipient_sender_offset_key_id,
&received_output.commitment,
received_output.metadata_signature.ephemeral_commitment(),
&received_output.version,
&metadata_message,
)
.await?;

let metadata_signature = &received_output.metadata_signature + &sender_metadata_signature;
Ok(metadata_signature)
}

/// Attempts to build the final transaction.
#[allow(clippy::too_many_lines)]
async fn build_transaction<KM: TransactionKeyManagerInterface>(
Expand Down
10 changes: 3 additions & 7 deletions base_layer/wallet/src/transaction_service/service.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1214,8 +1214,7 @@ where

// Start finalizing

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

// Finalize
Expand Down Expand Up @@ -1402,9 +1401,7 @@ where
let recipient_reply = rtp.get_signed_data()?.clone();

// Start finalizing

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

// Finalize
Expand Down Expand Up @@ -1661,8 +1658,7 @@ where
}

// Start finalizing
stp.add_single_recipient_info(recipient_reply, &self.resources.transaction_key_manager_service)
.await
stp.add_presigned_recipient_info(recipient_reply)
.map_err(|e| TransactionServiceProtocolError::new(tx_id, e.into()))?;

// Finalize
Expand Down

0 comments on commit fef701e

Please sign in to comment.