Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add metadata signature check #5411

Merged
merged 2 commits into from
May 26, 2023
Merged

feat: add metadata signature check #5411

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00b7ae5
Add metadata signature check
hansieodendaal May 25, 2023
eb3ae1f
review comments
hansieodendaal May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions base_layer/core/src/transactions/transaction_components/transaction_output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,19 @@ impl TransactionOutput {
&self.encrypted_data,
self.minimum_value_promise,
);
// Let's first verify that the metadata signature is valid
hansieodendaal marked this conversation as resolved.
Show resolved Hide resolved
if !self.metadata_signature.verify_challenge(
&self.commitment,
&self.sender_offset_public_key,
&e_bytes,
&CommitmentFactory::default(),
&mut OsRng,
) {
return Err(RangeProofError::InvalidRangeProof(
"Metadata signature not valid!".to_string(),
));
}
// Now we can perform the balance proof
let e = PrivateKey::from_bytes(&e_bytes).unwrap();
let value_as_private_key = PrivateKey::from(self.minimum_value_promise.as_u64());
let commit_nonce_a = PrivateKey::default(); // This is the deterministic nonce `r_a` of zero
Expand Down Expand Up @@ -674,6 +687,8 @@ pub fn batch_verify_range_proofs(

#[cfg(test)]
mod test {
use tari_crypto::errors::RangeProofError;

use super::{batch_verify_range_proofs, TransactionOutput};
use crate::transactions::{
tari_amount::MicroTari,
Expand Down Expand Up @@ -825,6 +840,32 @@ mod test {
}
}

#[test]
fn revealed_value_proofs_only_succeed_with_valid_metadata_signatures() {
let factories = CryptoFactories::default();
let test_params = TestParams::new();
let mut output = create_output(
&test_params,
&factories,
MicroTari(20),
MicroTari(20),
RangeProofType::RevealedValue,
)
.unwrap();
assert!(output.verify_metadata_signature().is_ok());
assert!(output.revealed_value_range_proof_check().is_ok());

output.features.maturity += 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this overflow and fail the test? I know we've seen this elsewhere in tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the test, it does not matter if it overflows, as long as it is changed, so that the features committed to in the metadata signature is different.

Copy link
Collaborator

@AaronFeickert AaronFeickert May 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I had been concerned that an overflow could panic and intermittently fail the test, depending on how the value was originally set. Looks like it's originally set to zero, so this can't happen. I'd do output.features.maturity = 1 instead anyway; seems more elegant!

assert!(output.verify_metadata_signature().is_err());
match output.revealed_value_range_proof_check() {
Ok(_) => panic!("Should not have passed check"),
Err(e) => assert_eq!(
e,
RangeProofError::InvalidRangeProof("Metadata signature not valid!".to_string())
),
}
}

#[test]
fn it_does_not_batch_verify_incorrect_minimum_values() {
let factories = CryptoFactories::default();
Expand Down