From 6db3a00edc992237058d6fe00804cd907dd4997b Mon Sep 17 00:00:00 2001 From: danda Date: Thu, 3 Mar 2022 15:05:53 -0800 Subject: [PATCH] fix: verify Tx in DbcBuilder::build() Previously DbcBuilder::build() was only doing some incomplete validations. This commit verifies the Tx which also requires that DbcBuilder::build() accept a KeyManager param, which has mintnode/spentbooknode trust implications/requirements that are now documented. * doc: add doc comment for TransactionVerifier::verify() and ::verify_without_sigs() * rename mint_verifier param to verifier * doc: add doc comment for Dbc::verify() * optimization: iter/collect() mint sigs once per tx, not per Dbc * add KeyManager arg to DbcBuilder::build() * call TransactionVerifier::verify() within DbcBuilder::build() * adapt tests, examples, benches to pass key_manager into ::build() --- benches/reissue.rs | 8 +++-- examples/mint-repl/mint-repl.rs | 4 +-- src/builder.rs | 55 +++++++++++++++++++++++---------- src/dbc.rs | 26 +++++++++------- src/mint.rs | 8 ++--- src/verification.rs | 41 +++++++++++++++++++----- 6 files changed, 98 insertions(+), 44 deletions(-) diff --git a/benches/reissue.rs b/benches/reissue.rs index d7f06d9..671651c 100644 --- a/benches/reissue.rs +++ b/benches/reissue.rs @@ -108,7 +108,7 @@ fn bench_reissue_100_to_1(c: &mut Criterion) { let reissue_share = mintnode.reissue(rr).unwrap(); dbc_builder = dbc_builder.add_reissue_share(reissue_share); - let dbcs = dbc_builder.build().unwrap(); + let dbcs = dbc_builder.build(mintnode.key_manager()).unwrap(); let output_owner_once = OwnerOnce::from_owner_base(Owner::from_random_secret_key(&mut rng), &mut rng8); @@ -187,7 +187,11 @@ fn generate_dbc_of_value( let reissue_share = mint_node.reissue(rr)?; dbc_builder = dbc_builder.add_reissue_share(reissue_share); - let (starting_dbc, ..) = dbc_builder.build()?.into_iter().next().unwrap(); + let (starting_dbc, ..) = dbc_builder + .build(mint_node.key_manager())? + .into_iter() + .next() + .unwrap(); Ok((mint_node, spentbook_node, starting_dbc)) } diff --git a/examples/mint-repl/mint-repl.rs b/examples/mint-repl/mint-repl.rs index 845200a..b3f7674 100644 --- a/examples/mint-repl/mint-repl.rs +++ b/examples/mint-repl/mint-repl.rs @@ -843,7 +843,7 @@ fn reissue_auto_cli(mintinfo: &mut MintInfo) -> Result<()> { for mint_node in mintinfo.mintnodes.iter() { dbc_builder = dbc_builder.add_reissue_share(mint_node.reissue(rr.clone())?); } - let outputs = dbc_builder.build()?; + let outputs = dbc_builder.build(mintinfo.mintnodes[0].key_manager())?; let output_dbcs: Vec = outputs.into_iter().map(|(dbc, ..)| dbc).collect(); for dbc in input_dbcs.iter() { @@ -895,7 +895,7 @@ fn reissue(mintinfo: &mut MintInfo, reissue_request: ReissueRequestRevealed) -> dbc_builder = dbc_builder.add_reissue_share(reissue_share); } - let output_dbcs = dbc_builder.build()?; + let output_dbcs = dbc_builder.build(mintinfo.mintnodes[0].key_manager())?; // for each output, construct Dbc and display for (dbc, _owner_once, _amount_secrets) in output_dbcs.iter() { diff --git a/src/builder.rs b/src/builder.rs index 25ab521..6fce0c2 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -15,9 +15,9 @@ use rand_core::RngCore; use std::collections::{BTreeMap, BTreeSet, HashSet}; use crate::{ - Amount, AmountSecrets, Commitment, Dbc, DbcContent, Error, Hash, KeyImage, NodeSignature, - OwnerOnce, PublicKeyBlst, PublicKeyBlstMappable, ReissueRequest, ReissueShare, Result, - SecretKeyBlst, SpentProof, SpentProofContent, SpentProofShare, + Amount, AmountSecrets, Commitment, Dbc, DbcContent, Error, Hash, KeyImage, KeyManager, + NodeSignature, OwnerOnce, PublicKeyBlst, PublicKeyBlstMappable, ReissueRequest, ReissueShare, + Result, SecretKeyBlst, SpentProof, SpentProofContent, SpentProofShare, TransactionVerifier, }; #[cfg(feature = "serde")] @@ -342,7 +342,16 @@ impl DbcBuilder { } /// Build the output DBCs - pub fn build(self) -> Result> { + /// + /// This function relies/assumes that the caller (wallet/client) obtains + /// the mint's and spentbook's public keys (held by KeyManager) in a + /// trustless/verified way. ie, the caller should not simply obtain keys + /// from a MintNode directly, but must somehow verify that the MintNode is + /// a valid authority. + pub fn build( + self, + mint_verifier: &K, + ) -> Result> { let mut mint_sig_shares: Vec = Default::default(); let mut pk_set: HashSet = Default::default(); @@ -388,6 +397,22 @@ impl DbcBuilder { // Combine signatures from all the mint nodes to obtain Mint's Signature. let mint_sig = mint_public_key_set.combine_signatures(mint_sig_shares_ref)?; + // note: all output Dbcs share the same mint_sigs + let mint_sigs = transaction + .mlsags + .iter() + .map(|mlsag| { + ( + mlsag.key_image.into(), + (mint_public_key_set.public_key(), mint_sig.clone()), + ) + }) + .collect(); + + // verify the Tx, along with mint sigs and spent proofs. + // note that we do this just once for entire Tx, not once per output Dbc. + TransactionVerifier::verify(mint_verifier, transaction, &mint_sigs, spent_proofs)?; + let pc_gens = PedersenGens::default(); let output_commitments: Vec<(Commitment, RevealedCommitment)> = self .revealed_commitments @@ -425,16 +450,7 @@ impl DbcBuilder { amount_secrets_list[0].clone(), )), transaction: transaction.clone(), - mint_sigs: transaction - .mlsags - .iter() - .map(|mlsag| { - ( - mlsag.key_image.into(), - (mint_public_key_set.public_key(), mint_sig.clone()), - ) - }) - .collect(), + mint_sigs: mint_sigs.clone(), spent_proofs: spent_proofs.clone(), }; (dbc, owner_once.clone(), amount_secrets_list[0].clone()) @@ -611,8 +627,15 @@ pub mod mock { mint_nodes.push(mint_node); } - let (genesis_dbc, _owner_once, amount_secrets) = - dbc_builder.build()?.into_iter().next().unwrap(); + // note: for our (mock) purposes, all mint nodes are verified to + // have the same public key. (in the same section) + let mint_node_arbitrary = &mint_nodes[0]; + + let (genesis_dbc, _owner_once, amount_secrets) = dbc_builder + .build(mint_node_arbitrary.key_manager())? + .into_iter() + .next() + .unwrap(); Ok(( mint_nodes, self.spentbook_nodes, diff --git a/src/dbc.rs b/src/dbc.rs index b11b1b3..4e1ae96 100644 --- a/src/dbc.rs +++ b/src/dbc.rs @@ -184,17 +184,19 @@ impl Dbc { hash } - // Check there exists a Transaction with the output containing this Dbc - // note: common verification logic is shared by MintNode::reissue() and Dbc::verify() - // - // note: for spent_proofs to verify, the mint_verifier must have/know the spentbook section's public key. - pub fn verify( - &self, - base_sk: &SecretKey, - mint_verifier: &K, - ) -> Result<(), Error> { + /// Verifies that this Dbc is valid. + /// + /// important: this does not check if the Dbc has been spent. + /// For that, one must query the SpentBook. + /// + /// note: common verification logic is shared by MintNode::reissue(), + /// DbcBuilder::build() and Dbc::verify() + /// + /// see TransactionVerifier::verify() for a description of + /// verifier requirements. + pub fn verify(&self, base_sk: &SecretKey, verifier: &K) -> Result<(), Error> { TransactionVerifier::verify( - mint_verifier, + verifier, &self.transaction, &self.mint_sigs, &self.spent_proofs, @@ -423,7 +425,7 @@ pub(crate) mod tests { let split_reissue_share = mint_node.reissue(reissue_request)?; dbc_builder = dbc_builder.add_reissue_share(split_reissue_share); - let output_dbcs = dbc_builder.build()?; + let output_dbcs = dbc_builder.build(mint_node.key_manager())?; // The outputs become inputs for next reissue. let inputs: Vec<(Dbc, SecretKey, Vec)> = output_dbcs @@ -675,7 +677,7 @@ pub(crate) mod tests { let reissue_share = mint_node.reissue(rr)?; dbc_builder = dbc_builder.add_reissue_share(reissue_share); - let mut iter = dbc_builder.build()?.into_iter(); + let mut iter = dbc_builder.build(mint_node.key_manager())?.into_iter(); let (starting_dbc, ..) = iter.next().unwrap(); let (change_dbc, ..) = iter.next().unwrap(); diff --git a/src/mint.rs b/src/mint.rs index 907b653..bee6e14 100644 --- a/src/mint.rs +++ b/src/mint.rs @@ -262,7 +262,7 @@ mod tests { // Aggregate ReissueShare to build output DBCs dbc_builder = dbc_builder.add_reissue_share(reissue_share); - let output_dbcs = dbc_builder.build()?; + let output_dbcs = dbc_builder.build(mint_node.key_manager())?; for (dbc, owner_once, amount_secrets) in output_dbcs.iter() { let dbc_amount = amount_secrets.amount(); @@ -387,7 +387,7 @@ mod tests { // Aggregate ReissueShare to build output DBCs dbc_builder = dbc_builder.add_reissue_share(reissue_share); - let output_dbcs = dbc_builder.build()?; + let output_dbcs = dbc_builder.build(mint_node.key_manager())?; // The outputs become inputs for next reissue. let inputs_dbcs: Vec<(Dbc, SecretKey, Vec)> = output_dbcs @@ -534,7 +534,7 @@ mod tests { // Aggregate ReissueShare to build output DBCs dbc_builder = dbc_builder.add_reissue_share(rs); - let output_dbcs = dbc_builder.build()?; + let output_dbcs = dbc_builder.build(mint_node.key_manager())?; for (dbc, owner_once, _amount_secrets) in output_dbcs.iter() { let dbc_confirm_result = dbc.verify( @@ -703,7 +703,7 @@ mod tests { // Aggregate ReissueShare to build output DBCs dbc_builder = dbc_builder.add_reissue_share(reissue_share); - let output_dbcs = dbc_builder.build()?; + let output_dbcs = dbc_builder.build(mint_node.key_manager())?; let (b_dbc, ..) = &output_dbcs[0]; // ---------- diff --git a/src/verification.rs b/src/verification.rs index fe35df9..f5a6104 100644 --- a/src/verification.rs +++ b/src/verification.rs @@ -27,9 +27,20 @@ use std::collections::{BTreeMap, BTreeSet}; pub struct TransactionVerifier {} impl TransactionVerifier { - // note: for spent_proofs to verify, the mint_verifier must have/know the spentbook section's public key. + /// Verifies a transaction including mint signatures and spent proofs. + /// + /// This function relies/assumes that the caller (wallet/client) obtains + /// the mint's and spentbook's public keys (held by KeyManager) in a + /// trustless/verified way. ie, the caller should not simply obtain keys + /// from a MintNode directly, but must somehow verify that the MintNode is + /// a valid authority. + /// + /// note: for spent_proofs to verify, the verifier must have/know the + /// public key of each spentbook section that recorded a tx input as spent. + /// note: for mint_sigs to verify, the verifier must have/know the + /// public key of the mint section that performed the reissue and signed tx. pub fn verify( - mint_verifier: &K, + verifier: &K, transaction: &RingCtTransaction, mint_sigs: &BTreeMap, spent_proofs: &BTreeSet, @@ -42,6 +53,10 @@ impl TransactionVerifier { let tx_hash = Hash::from(transaction.hash()); + // todo: optimization. avoid duplicate validations + // when multiple inputs share the same mint_key+mint_sig + // (because they were outputs of a single tx) + // Verify that each input has a corresponding valid mint signature. for (key_image, (mint_key, mint_sig)) in mint_sigs.iter() { if !transaction @@ -52,25 +67,35 @@ impl TransactionVerifier { return Err(Error::UnknownInput); } - mint_verifier + verifier .verify(&tx_hash, mint_key, mint_sig) .map_err(|e| Error::Signing(e.to_string()))?; } - Self::verify_without_sigs_internal(mint_verifier, transaction, tx_hash, spent_proofs) + Self::verify_without_sigs_internal(verifier, transaction, tx_hash, spent_proofs) } + /// Verifies a transaction including including spent proofs and excluding mint signatures. + /// + /// This function relies/assumes that the caller (wallet/client) obtains + /// spentbook's public keys (held by KeyManager) in a + /// trustless/verified way. ie, the caller should not simply obtain keys + /// from a SpentBookNode directly, but must somehow verify that the + /// SpentBookNode is a valid authority. + /// + /// note: for spent_proofs to verify, the verifier must have/know the + /// public key of each spentbook section that recorded a tx input as spent. pub fn verify_without_sigs( - mint_verifier: &K, + verifier: &K, transaction: &RingCtTransaction, spent_proofs: &BTreeSet, ) -> Result<(), Error> { let tx_hash = Hash::from(transaction.hash()); - Self::verify_without_sigs_internal(mint_verifier, transaction, tx_hash, spent_proofs) + Self::verify_without_sigs_internal(verifier, transaction, tx_hash, spent_proofs) } fn verify_without_sigs_internal( - mint_verifier: &K, + verifier: &K, transaction: &RingCtTransaction, transaction_hash: Hash, spent_proofs: &BTreeSet, @@ -102,7 +127,7 @@ impl TransactionVerifier { { return Err(Error::SpentProofInputMismatch); } - spent_proof.verify(transaction_hash, mint_verifier)?; + spent_proof.verify(transaction_hash, verifier)?; } // We must get the spent_proofs into the same order as mlsags