From e0d345342760b184d11e972bfb556e72dff82de4 Mon Sep 17 00:00:00 2001 From: danda Date: Sat, 5 Mar 2022 17:10:10 -0800 Subject: [PATCH] fix: make prop_dbc_validation pass This test was intermittently failing in CI. I found and fixed a few problems with it, including some problematic logic in TransactionVerifier. * verification: improve logic for MissingSignatureForInput error. * verification: verify number of mint sigs equals number of inputs * verification: replace UnknownInput with MintSignatureInputMismatch * error: replace UnknownInput with MintSignatureInputMismatch * test: generated fuzzed_amount_secrets with correct blinding factor for cases when extra_output_amount is 0. * test: remove ugly commitments_match case when verification succeeds. * test: check for MintSignatureInputMismatch error instead of UnknownInput * test: check for AmountCommitmentsDoNotMatch error --- src/dbc.rs | 51 ++++++++++++++++++++++++++------------------- src/error.rs | 6 +++--- src/verification.rs | 6 +++--- 3 files changed, 35 insertions(+), 28 deletions(-) diff --git a/src/dbc.rs b/src/dbc.rs index 4e1ae96..66862e7 100644 --- a/src/dbc.rs +++ b/src/dbc.rs @@ -268,8 +268,6 @@ impl Dbc { pub(crate) mod tests { use super::*; - use std::iter::FromIterator; - use quickcheck_macros::quickcheck; use crate::tests::{NonZeroTinyInt, TinyInt}; @@ -442,7 +440,7 @@ pub(crate) mod tests { let owner_once = OwnerOnce::from_owner_base(Owner::from_random_secret_key(&mut rng), &mut rng8); - let (mut rr_builder, _dbc_builder, _material) = crate::TransactionBuilder::default() + let (mut rr_builder, dbc_builder, _material) = crate::TransactionBuilder::default() .add_inputs_dbc(inputs, &mut rng8)? .add_output( Output { @@ -471,8 +469,25 @@ pub(crate) mod tests { .combine_signatures(vec![mint_sig_share.threshold_crypto()]) .unwrap(); - let fuzzed_amt_secrets = - AmountSecrets::from_amount(amount + extra_output_amount.coerce::(), &mut rng8); + // We must obtain the RevealedCommitment for our output in order to + // know the correct blinding factor when creating fuzzed_amt_secrets. + let output = reissue_tx.outputs.get(0).unwrap(); + let pc_gens = bulletproofs::PedersenGens::default(); + let output_commitments: Vec<(crate::Commitment, RevealedCommitment)> = dbc_builder + .revealed_commitments + .iter() + .map(|r| (r.commit(&pc_gens).to_affine(), *r)) + .collect(); + let amount_secrets_list: Vec = output_commitments + .iter() + .filter(|(c, _)| *c == output.commitment()) + .map(|(_, r)| AmountSecrets::from(*r)) + .collect(); + + let fuzzed_amt_secrets = AmountSecrets::from(( + amount + extra_output_amount.coerce::(), + amount_secrets_list[0].blinding_factor(), + )); let dbc_amount = fuzzed_amt_secrets.amount(); let fuzzed_content = DbcContent::from(( @@ -559,8 +574,6 @@ pub(crate) mod tests { let key_manager = mint_node.key_manager(); let verification_res = dbc.verify(&owner_once.owner_base().secret_key()?, key_manager); - let commitments_match = matches!(verification_res, Err(Error::AmountCommitmentsDoNotMatch)); - let dbc_owner = dbc .owner_once(&owner_once.owner_base().secret_key()?)? .public_key_blst(); @@ -578,29 +591,19 @@ pub(crate) mod tests { assert_eq!(n_wrong_signer_sigs.coerce::(), 0); assert_eq!(n_wrong_msg_sigs.coerce::(), 0); - // note: reissue can succeed even though amount_secrets_cipher amount does not - // match tx commited amount. - assert!(dbc_amount == amount || !commitments_match); - assert!(extra_output_amount.coerce::() == 0 || !commitments_match); + assert_eq!(dbc_amount, amount); + assert_eq!(extra_output_amount.coerce::(), 0); } Err(Error::MissingSignatureForInput) => { assert!(n_valid_sigs.coerce::() < n_inputs.coerce::()); } + Err(Error::MintSignatureInputMismatch) => { + assert_ne!(dbc.mint_sigs.len(), n_inputs.coerce::()); + } Err(Error::SpentProofInputMismatch) => { // todo: fuzz spent proofs. assert!(dbc.spent_proofs.len() < dbc.transaction.mlsags.len()); } - Err(Error::UnknownInput) => { - assert!(n_extra_input_sigs.coerce::() > 0); - assert_ne!( - Vec::from_iter(dbc.mint_sigs.keys().cloned()), - dbc.transaction - .mlsags - .iter() - .map(|m| m.key_image.into()) - .collect::>() - ); - } Err(Error::DbcContentNotPresentInTransactionOutput) => { assert!(!dbc .transaction @@ -631,6 +634,10 @@ pub(crate) mod tests { .values() .any(|(pk, _)| key_manager.verify_known_key(pk).is_err())); } + Err(Error::AmountCommitmentsDoNotMatch) => { + assert_ne!(amount, dbc_amount); + assert_ne!(extra_output_amount, TinyInt(0)); + } res => panic!("Unexpected verification result {:?}", res), } diff --git a/src/error.rs b/src/error.rs index f07b5fd..4954998 100644 --- a/src/error.rs +++ b/src/error.rs @@ -24,9 +24,6 @@ pub enum Error { #[error("An error occured when signing {0}")] Signing(String), - #[error("This input has a signature, but it doesn't appear in the transaction")] - UnknownInput, - #[error("Failed signature check.")] FailedSignature, @@ -36,6 +33,9 @@ pub enum Error { #[error("At least one transaction input is missing a signature.")] MissingSignatureForInput, + #[error("The number of mint signatures does not match the number of transaction inputs.")] + MintSignatureInputMismatch, + #[error("Invalid SpentProof Signature for {0:?}")] InvalidSpentProofSignature(KeyImage), diff --git a/src/verification.rs b/src/verification.rs index 05603cc..57da94b 100644 --- a/src/verification.rs +++ b/src/verification.rs @@ -47,14 +47,14 @@ impl TransactionVerifier { ) -> Result<(), Error> { // Do quick checks first to reduce potential DOS vectors. - if mint_sigs.len() < transaction.mlsags.len() { - return Err(Error::MissingSignatureForInput); + if mint_sigs.len() != transaction.mlsags.len() { + return Err(Error::MintSignatureInputMismatch); } // Verify that we received a mint pk/sig for each tx input. for mlsag in transaction.mlsags.iter() { if mint_sigs.get(&mlsag.key_image.into()).is_none() { - return Err(Error::UnknownInput); + return Err(Error::MissingSignatureForInput); } }