feat!: update commitment signature #4943
Conversation
WIP WIP: size typo wip Auto stash before checking out "origin/development" wip wip
SWvheerden
added
the
W-consensus_breaking
| @@ -1032,13 +1032,13 @@ 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","script","input_data","script_private_key","sender_offset_public_key","public_nonce","signature_u","signature_v""## | |||
| r##""index","value","spending_key","commitment","flags","maturity","script","input_data","script_private_key","sender_offset_public_key","emperical_commitment","emperical_nonce","signature_u_x","signature_u_a","signature_u_y""## | |||
There was a problem hiding this comment.
Misspelling... should be ephemeral_commitment and ephemeral_nonce.
| @@ -170,7 +172,8 @@ impl TransactionInput { | |||
| match version { | |||
| TransactionInputVersion::V0 | TransactionInputVersion::V1 => { | |||
| DomainSeparatedConsensusHasher::<TransactionHashDomain>::new("script_challenge") | |||
There was a problem hiding this comment.
May want to use the reverse-domain standard for this label, in order to be more consistent with other uses of the hashing API elsewhere in the codebase.
| /// Convenience function that calculates the challenge for the metadata commitment signature | ||
| pub fn build_metadata_signature_challenge( | ||
| version: TransactionOutputVersion, | ||
| script: &TariScript, | ||
| features: &OutputFeatures, | ||
| sender_offset_public_key: &PublicKey, | ||
| public_commitment_nonce: &Commitment, | ||
| ephemeral_commitment: &Commitment, | ||
| ephemeral_pubkey: &PublicKey, | ||
| commitment: &Commitment, | ||
| covenant: &Covenant, | ||
| encrypted_value: &EncryptedValue, | ||
| minimum_value_promise: MicroTari, | ||
| ) -> [u8; 32] { | ||
| let common = DomainSeparatedConsensusHasher::<TransactionHashDomain>::new("metadata_signature") |
There was a problem hiding this comment.
May want to use the reverse-domain standard for this label, in order to be more consistent with other uses of the hashing API elsewhere in the codebase.
| let aggregated_metadata_signature = ComSignature::new(r_pub_aggregated, u_aggregated, v.clone()); | ||
|
|
||
| let sender_offset_public_key = PublicKey::from_secret_key(sender_offset_private_key); | ||
| let aggregated_metadata_signature = &sender_signature + &output.metadata_signature; |
There was a problem hiding this comment.
It's possible that the recipient maliciously provided its own values for ephemeral_pubkey or u_y before returning it to the sender. Provided the output's sender offset public key cannot be overwritten by the recipient in a way that forces the sender to include it in the challenge, this will be caught by the verification check below, but could also be short-circuited by asserting that the recipient's partial metadata signature has ephemeral_pubkey == u_y == 0.
There was a problem hiding this comment.
It may also be a good idea to include this check anyway for defense in depth.
There was a problem hiding this comment.
When doing m-of-n this will need to be bypassed. I prefer a one fit for all check.
But yeah if the recipient changes ephemeral_pubkey or u_y the check below will fail and the signature will not pass.
The recipient can at all times replace sender offset public key and create a new signature at will, but this will break the script_offset of the transaction.
|
An even safer approach to partial metadata signatures would be not to add them at all (which is where you could get recipient shenanigans), but instead to build a fresh metadata signature from the appropriate constituent parts provided by the sender and recipient. This would eliminate the possibility of unexpected values included in partial signatures. |
|
One thing to keep in mind with these signatures, is that the main thing that stop any recipient shenanigans is |
Description
Replaces PR #4733
This updates CommitmentSignature to CommitmentAndPublicKeySignature for transaction authorization.
Includes a new Gen block
Motivation and Context
See issue: #4734
How Has This Been Tested?
Unit tests
BREAKING CHANGE: Commitment signature proof has changed