refactor: consolidate common validation logic

* move common transaction validation logic into new TransactionValidator
* change SimpleKeyManager::new() to impl From
* fix: add genesis public commitments to SpentBookMock with ::set_genesis()
* cleanup and add/edit comments
* cleanup: remove unused Error variants
* cleanup: remove redundant vars from GenesisDbcShare

src/builder.rs

@@ -137,7 +137,7 @@ impl TransactionBuilder {
     }
 }
 
-/// Builds a ReissueRequest from a ReissueTransaction and
+/// Builds a ReissueRequest from a RingCtTransaction and
 /// any number of (input) DBC spent proof shares.
 #[derive(Debug)]
 pub struct ReissueRequestBuilder {
@@ -231,7 +231,7 @@ pub struct DbcBuilder {
 }
 
 impl DbcBuilder {
-    /// Create a new DbcBuilder from a ReissueTransaction
+    /// Create a new DbcBuilder
     pub fn new(
         revealed_commitments: Vec<RevealedCommitment>,
         output_owners: OutputOwnerMap,

src/dbc.rs

@@ -8,15 +8,17 @@
 
 use crate::{dbc_content::OwnerPublicKey, DbcContent, Error, KeyManager, Result};
 
-use crate::{AmountSecrets, BlsHelper, Hash, SpentProof};
+use crate::{AmountSecrets, BlsHelper, SpentProof, TransactionValidator};
 use blst_ringct::ringct::{OutputProof, RingCtTransaction};
 use blst_ringct::RevealedCommitment;
 use blstrs::group::Curve;
 use blsttc::{PublicKey, SecretKey, Signature};
 use std::collections::{BTreeMap, BTreeSet};
 use tiny_keccak::{Hasher, Sha3};
 
-// note: typedef should be moved into blst_ringct crate
+// todo: move this someplace better, maybe lib.rs.
+// Alternatively, we could perhaps wrap G1Affine to add Ord so it can
+// be used in a BTreeMap directly
 pub type KeyImage = [u8; 48]; // G1 compressed
 
 // #[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize, Serialize)]
@@ -70,46 +72,16 @@ impl Dbc {
         base_sk: &SecretKey,
         mint_verifier: &K,
     ) -> Result<(), Error> {
-        let tx_hash = Hash::from(self.transaction.hash());
-
-        // Verify that each input has a corresponding valid mint signature.
-        for (key_image, (mint_key, mint_sig)) in self.transaction_sigs.iter() {
-            if !self
-                .transaction
-                .mlsags
-                .iter()
-                .any(|m| m.key_image.to_compressed() == *key_image)
-            {
-                return Err(Error::UnknownInput);
-            }
-
-            mint_verifier
-                .verify(&tx_hash, mint_key, mint_sig)
-                .map_err(|e| Error::Signing(e.to_string()))?;
-        }
-
-        // Verify that each input has a corresponding valid spent proof.
-        for spent_proof in self.spent_proofs.iter() {
-            if !self
-                .transaction
-                .mlsags
-                .iter()
-                .any(|m| m.key_image.to_compressed() == spent_proof.key_image)
-            {
-                return Err(Error::UnknownInput);
-            }
-            spent_proof.validate(tx_hash, mint_verifier)?;
-        }
+        TransactionValidator::validate(
+            mint_verifier,
+            &self.transaction,
+            &self.transaction_sigs,
+            &self.spent_proofs,
+        )?;
 
         let owner = self.owner(base_sk)?;
 
-        if self.transaction.mlsags.is_empty() {
-            Err(Error::TransactionMustHaveAnInput)
-        } else if self.transaction_sigs.len() < self.transaction.mlsags.len() {
-            Err(Error::MissingSignatureForInput)
-        } else if self.spent_proofs.len() != self.transaction.mlsags.len() {
-            Err(Error::MissingSpentProof)
-        } else if !self
+        if !self
             .transaction
             .outputs
             .iter()
@@ -123,6 +95,25 @@ impl Dbc {
 
     /// Checks if the provided AmountSecrets matches the amount commitment.
     /// note that both the amount and blinding_factor must be correct.
+    ///
+    /// Note that the mint cannot perform this check.  Only the Dbc
+    /// recipient can.
+    ///
+    /// A Dbc recipient should call this immediately upon receipt.
+    /// If the commitments do not match, then the Dbc cannot be spent
+    /// using the AmountSecrets provided.
+    ///
+    /// To clarify, the Dbc is still spendable, however the correct
+    /// AmountSecrets need to be obtained from the sender somehow.
+    ///
+    /// As an example, if the Dbc recipient is a merchant, they typically
+    /// would not provide goods to the purchaser if this check fails.
+    /// However the purchaser may still be able to remedy the situation by
+    /// providing the correct AmountSecrets to the merchant.
+    ///
+    /// If the merchant were to send the goods without first performing
+    /// this check, then they could be stuck with an unspendable Dbc
+    /// and no recourse.
     pub fn confirm_provided_amount_matches_commitment(
         &self,
         base_sk: &SecretKey,
@@ -268,7 +259,7 @@ mod tests {
         };
 
         let id = crate::bls_dkg_id(&mut rng);
-        let mint_key_manager = SimpleKeyManager::new(SimpleSigner::from(id));
+        let mint_key_manager = SimpleKeyManager::from(SimpleSigner::from(id));
 
         assert!(matches!(
             dbc.confirm_valid(&derived_owner.base_secret_key()?, &mint_key_manager,),
@@ -293,16 +284,18 @@ mod tests {
 
         let amount = 100;
 
-        let mut spentbook = SpentBookMock::from(SimpleKeyManager::new(SimpleSigner::from(
+        let mut spentbook = SpentBookMock::from(SimpleKeyManager::from(SimpleSigner::from(
             crate::bls_dkg_id(&mut rng),
         )));
 
-        let (mint_node, genesis) = MintNode::new(SimpleKeyManager::new(SimpleSigner::from(
+        let (mint_node, genesis) = MintNode::new(SimpleKeyManager::from(SimpleSigner::from(
             crate::bls_dkg_id(&mut rng),
         )))
         .trust_spentbook_public_key(spentbook.key_manager.public_key_set()?.public_key())?
         .issue_genesis_dbc(amount, &mut rng8)?;
 
+        spentbook.set_genesis(&genesis.ringct_material);
+
         let _genesis_spent_proof_share =
             spentbook.log_spent(genesis.input_key_image, genesis.transaction.clone())?;
 
@@ -427,7 +420,7 @@ mod tests {
         for _ in 0..n_wrong_signer_sigs.coerce() {
             if let Some(input) = repeating_inputs.next() {
                 let id = crate::bls_dkg_id(&mut rng);
-                let key_manager = SimpleKeyManager::new(SimpleSigner::from(id));
+                let key_manager = SimpleKeyManager::from(SimpleSigner::from(id));
                 let trans_sig_share = key_manager
                     .sign(&Hash::from(reissue_share.transaction.hash()))
                     .unwrap();
@@ -514,7 +507,7 @@ mod tests {
             Err(Error::MissingSignatureForInput) => {
                 assert!(n_valid_sigs.coerce::<u8>() < n_inputs.coerce::<u8>());
             }
-            Err(Error::MissingSpentProof) => {
+            Err(Error::SpentProofInputMismatch) => {
                 // todo: fuzz spent proofs.
                 assert!(dbc.spent_proofs.len() < dbc.transaction.mlsags.len());
             }

src/error.rs

@@ -5,7 +5,6 @@
 // under the GPL Licence is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 // KIND, either express or implied. Please review the Licences for the specific language governing
 // permissions and limitations relating to use of the SAFE Network Software.
-use std::io;
 use thiserror::Error;
 
 use crate::KeyImage;
@@ -20,22 +19,25 @@ pub type Result<T, E = Error> = std::result::Result<T, E>;
 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,
+
     #[error("Unrecognised authority.")]
     UnrecognisedAuthority,
+
     #[error("At least one transaction input is missing a signature.")]
     MissingSignatureForInput,
-    #[error("At least one input is missing a spent proof")]
-    MissingSpentProof,
+
     #[error("Invalid SpentProof Signature for {0:?}")]
     InvalidSpentProofSignature(KeyImage),
-    #[error("Mint request doesn't balance out sum(input) == sum(output)")]
-    DbcReissueRequestDoesNotBalance,
+
     #[error("The DBC transaction must have at least one input")]
     TransactionMustHaveAnInput,
+
     #[error("Dbc Content is not a member of transaction outputs")]
     DbcContentNotPresentInTransactionOutput,
 
@@ -51,9 +53,6 @@ pub enum Error {
     #[error("The number of SpentProof does not match the number of input MlsagSignature")]
     SpentProofInputMismatch,
 
-    #[error("The SpentProof key-image is not found amongst transaction inputs")]
-    SpentProofKeyImageMismatch,
-
     #[error("The PublicKeySet differs between ReissueRequest entries")]
     ReissueRequestPublicKeySetMismatch,
 
@@ -75,23 +74,12 @@ pub enum Error {
     #[error("No reissue shares")]
     NoReissueShares,
 
-    // #[error("RangeProof error: {0}")]
-    // RangeProof(#[from] bulletproofs::ProofError),
-    #[error("Derived owner key does not match")]
-    DerivedOwnerKeyDoesNotMatch,
-
-    #[error("Decryption error: {0}")]
-    DecryptionBySharesFailed(#[from] blsttc::error::Error),
-
     #[error("Decryption failed")]
     DecryptionBySecretKeyFailed,
 
     #[error("Invalid AmountSecret bytes")]
     AmountSecretsBytesInvalid,
 
-    #[error("Invalid Amount Commitment")]
-    AmountCommitmentInvalid,
-
     #[error("Amount Commitments do not match")]
     AmountCommitmentsDoNotMatch,
 
@@ -101,17 +89,13 @@ pub enum Error {
     #[error("Public key not found")]
     PublicKeyNotFound,
 
+    #[error("Bls error: {0}")]
+    Blsttc(#[from] blsttc::error::Error),
+
     /// blst_ringct error.
     #[error("ringct error: {0}")]
     RingCt(#[from] blst_ringct::Error),
 
-    /// I/O error.
-    #[error("I/O error: {0}")]
-    Io(#[from] io::Error),
-    /// JSON serialisation error.
-    #[error("JSON serialisation error: {0}")]
-    JsonSerialisation(#[from] serde_json::Error),
-
     #[error("Infallible.  Can never fail")]
     Infallible(#[from] std::convert::Infallible),
 }

src/key_manager.rs

@@ -95,8 +95,8 @@ pub struct SimpleKeyManager {
     cache: Keys,
 }
 
-impl SimpleKeyManager {
-    pub fn new(signer: SimpleSigner) -> Self {
+impl From<SimpleSigner> for SimpleKeyManager {
+    fn from(signer: SimpleSigner) -> Self {
         let public_key_set = signer.public_key_set();
         let mut cache = Keys::default();
         cache.add_known_key(public_key_set.public_key());

src/lib.rs

@@ -19,6 +19,7 @@ mod error;
 mod key_manager;
 mod mint;
 mod spent_proof;
+mod validation;
 
 pub use crate::{
     amount_secrets::AmountSecrets,
@@ -33,6 +34,7 @@ pub use crate::{
     },
     mint::{GenesisDbcShare, MintNode, MintNodeSignatures, ReissueRequest, ReissueShare},
     spent_proof::{SpentProof, SpentProofShare},
+    validation::TransactionValidator,
 };
 
 #[derive(Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
@@ -174,7 +176,8 @@ mod tests {
     use core::num::NonZeroU8;
     use quickcheck::{Arbitrary, Gen};
 
-    use blst_ringct::ringct::{OutputProof, RingCtTransaction};
+    use blst_ringct::ringct::{OutputProof, RingCtMaterial, RingCtTransaction};
+    use blstrs::group::Curve;
     use std::collections::BTreeMap;
 
     #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
@@ -275,33 +278,41 @@ mod tests {
         assert_eq!(sha3_256(data), *expected);
     }
 
-    /// This is a toy SpentBook used in our mint-repl, a proper implementation
-    /// will be distributed, and include signatures and be auditable.
+    /// This is a mock SpentBook used for our test cases. A proper implementation
+    /// will be distributed, persistent, and auditable.
+    ///
+    /// This impl has a serious inefficiency when looking up OutputProofs by
+    /// PublicKey.  A scan of all spent Tx is required.  This is not a problem
+    /// for small tests.
+    ///
+    /// A real (performant) impl would need to add an additional index/map from
+    /// PublicKey to Tx.  Or alternatively from PublicKey to KeyImage.  This requirement
+    /// may add complexity to a distributed implementation.
     #[derive(Debug, Clone)]
     pub struct SpentBookMock {
         pub key_manager: SimpleKeyManager,
         pub transactions: BTreeMap<KeyImage, RingCtTransaction>,
-        pub genesis_input_key_image: Option<KeyImage>,
+        pub genesis: Option<(KeyImage, G1Affine)>, // genesis input (keyimage, public_commitment)
     }
 
     impl From<SimpleKeyManager> for SpentBookMock {
         fn from(key_manager: SimpleKeyManager) -> Self {
             Self {
                 key_manager,
                 transactions: Default::default(),
-                genesis_input_key_image: None,
+                genesis: None,
             }
         }
     }
 
-    impl From<(SimpleKeyManager, KeyImage)> for SpentBookMock {
-        fn from(params: (SimpleKeyManager, KeyImage)) -> Self {
-            let (key_manager, key_image) = params;
+    impl From<(SimpleKeyManager, KeyImage, G1Affine)> for SpentBookMock {
+        fn from(params: (SimpleKeyManager, KeyImage, G1Affine)) -> Self {
+            let (key_manager, key_image, public_commitment) = params;
 
             Self {
                 key_manager,
                 transactions: Default::default(),
-                genesis_input_key_image: Some(key_image),
+                genesis: Some((key_image, public_commitment)),
             }
         }
     }
@@ -328,14 +339,14 @@ mod tests {
 
             // If this is the very first tx logged and genesis key_image was not
             // provided, then it becomes the genesis tx.
-            let genesis_input_key_image = match self.genesis_input_key_image {
-                Some(k) => k,
-                None => key_image,
+            let (genesis_key_image, genesis_public_commitment) = match self.genesis {
+                Some((k, pc)) => (k, pc),
+                None => panic!("Genesis key_image and public commitments unavailable"),
             };
 
-            // public_commitments should only be empty for the genesis transaction.
-            let public_commitments: Vec<G1Affine> = if key_image == genesis_input_key_image {
-                vec![]
+            // public_commitments are not available in spentbook for genesis transaction.
+            let public_commitments: Vec<G1Affine> = if key_image == genesis_key_image {
+                vec![genesis_public_commitment]
             } else {
                 // Todo: make this cleaner and more efficient.
                 //       spentbook needs to also be indexed by OutputProof PublicKey.
@@ -380,10 +391,6 @@ mod tests {
                 .entry(key_image)
                 .or_insert_with(|| tx.clone());
             if existing_tx.hash() == tx.hash() {
-                if self.genesis_input_key_image.is_none() {
-                    self.genesis_input_key_image = Some(genesis_input_key_image);
-                }
-
                 Ok(SpentProofShare {
                     key_image,
                     spentbook_pks,
@@ -394,5 +401,16 @@ mod tests {
                 panic!("Attempt to Double Spend")
             }
         }
+
+        pub fn set_genesis(&mut self, material: &RingCtMaterial) {
+            let key_image = material.inputs[0].true_input.key_image().to_compressed();
+            let public_commitment = material.inputs[0]
+                .true_input
+                .revealed_commitment
+                .commit(&Default::default())
+                .to_affine();
+
+            self.genesis = Some((key_image, public_commitment));
+        }
     }
 }