fix: avoid possible panic in issue_genesis_dbc

Various cleanup:

* add some missing doc comments
* return error instead of panicing in issue_genesis_dbc
* use (3) random_decoys() in tests.
* remove commented code from builder.rs
* make panic in SpentBookMock::iter() explicit

src/builder.rs

@@ -458,9 +458,6 @@ impl DbcBuilder {
             })
             .collect();
 
-        // sort outputs by name.  todo: is sorting necessary?
-        // output_dbcs.sort_by_key(Dbc::owner);
-
         Ok(output_dbcs)
     }
 }

src/dbc.rs

@@ -150,13 +150,18 @@ impl Dbc {
         self.key_image(&self.owner_base().secret_key()?)
     }
 
+    /// returns a TrueInput that represents this Dbc for use as
+    /// a transaction input.
     pub fn as_true_input(&self, base_sk: &SecretKey) -> Result<TrueInput> {
         Ok(TrueInput {
             secret_key: self.owner_once(base_sk)?.secret_key_blst()?,
             revealed_commitment: self.amount_secrets(base_sk)?.into(),
         })
     }
 
+    /// returns a TrueInput that represents this Dbc for use as
+    /// a transaction input.
+    /// will return an error if the SecretKey is not available.  (not bearer)
     pub fn as_true_input_bearer(&self) -> Result<TrueInput> {
         self.as_true_input(&self.owner_base().secret_key()?)
     }
@@ -298,6 +303,8 @@ mod tests {
     use rand_core::RngCore;
     use rand_core::SeedableRng as SeedableRngCore;
 
+    const STD_NUM_DECOYS: usize = 3;
+
     fn divide(amount: Amount, n_ways: u8) -> impl Iterator<Item = Amount> {
         (0..n_ways).into_iter().map(move |i| {
             let equal_parts = amount / (n_ways as Amount);
@@ -318,7 +325,7 @@ mod tests {
     ) -> Result<(ReissueRequest, Vec<RevealedCommitment>, OutputOwnerMap)> {
         let amount = amount_secrets.amount();
 
-        let decoy_inputs = vec![]; // for now.
+        let decoy_inputs = spentbook.random_decoys(STD_NUM_DECOYS, rng8);
 
         let (reissue_tx, revealed_commitments, _material, output_owners) =
             crate::TransactionBuilder::default()
@@ -443,7 +450,7 @@ mod tests {
                 (
                     dbc,
                     owner_once.owner_base().secret_key().unwrap(),
-                    vec![], // todo: decoy_inputs,
+                    spentbook.random_decoys(STD_NUM_DECOYS, &mut rng8),
                 )
             })
             .collect();

src/dbc_content.rs

@@ -18,6 +18,7 @@ use crate::{Error, Hash, Result};
 // note: Amount should move into blst_ringct crate.
 // (or else blst_ringct::RevealedCommitment should be made generic over Amount type)
 
+/// Represents a Dbc's value.
 pub type Amount = u64;
 
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]

src/mint.rs

@@ -91,15 +91,16 @@ impl<K: KeyManager> MintNode<K> {
         amount: Amount,
         mut rng: impl RngCore + rand_core::CryptoRng,
     ) -> Result<(Self, GenesisDbcShare)> {
+        // note: for now, we bypass KeyManager and create the keys deterministically,
+        // as the same keys must be generated by all MintNodes.
+
         // Make a secret key for the input to Genesis Tx.
         let input_poly = Poly::zero();
         let input_secret_key_set = SecretKeySet::from(input_poly);
         let input_secret_key =
             SecretKeyBlst::from_bytes_be(&input_secret_key_set.secret_key().to_bytes()).unwrap();
 
         // Make a secret key for the output of Genesis Tx. (The Genesis Dbc)
-        // temporary: we bypass KeyManager and create a deterministic
-        // secret key, used by all MintNodes.
         let poly = Poly::one();
         let secret_key_set = SecretKeySet::from(poly);
         let owner_once =
@@ -130,9 +131,7 @@ impl<K: KeyManager> MintNode<K> {
         };
 
         // Here we sign as the input DBC owner.
-        let (transaction, revealed_commitments) = ringct_material
-            .sign(&mut rng)
-            .expect("Failed to sign transaction");
+        let (transaction, revealed_commitments) = ringct_material.sign(&mut rng)?;
 
         let amount_secrets = AmountSecrets::from(revealed_commitments[0]);
         let dbc_content = DbcContent::from((
@@ -467,7 +466,6 @@ mod tests {
         };
 
         // normally spentbook verifies the tx, but here we skip it in order check reissue results.
-        // todo: refactor so that we use same result checking code here as for the reissue.
         let spent_proof_share = match spentbook
             .log_spent_and_skip_tx_verification(genesis_key_image, reissue_tx.clone())
         {

src/owner.rs

@@ -18,6 +18,25 @@ use rand::Rng;
 
 pub type DerivationIndex = [u8; 32];
 
+/// Represents a Dbc owner.
+///
+/// If the type is SecretKey, the Dbc is considered
+/// to be bearer (anyone in possession can spend).
+///
+/// If the type is PublicKey, the Dbc is considered
+/// to be owned (by whoever holds the SecretKey).
+///
+/// Dbc's have both a base Owner and a one-time-use Owner.
+///
+/// The base Owner public key is given out to other people
+/// in order to receive payments from them. It is never
+/// seen by the Mint or SpentBook.
+///
+/// The one-time-use Owner public key is not normally given
+/// to 3rd parties. It is used by Mint and SpentBook exactly
+/// once, when spending the associated Dbc.
+///
+/// See OwnerOnce which relates the two.
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[derive(Clone)]
 pub enum Owner {

src/spentbook.rs

@@ -79,10 +79,16 @@ impl From<(SimpleKeyManager, KeyImage, Commitment)> for SpentBookNodeMock {
 
 impl SpentBookNodeMock {
     pub fn iter(&self) -> impl Iterator<Item = (&KeyImage, &RingCtTransaction)> + '_ {
-        // fixme: unwrap.  maybe try_iter() instead?
-        self.key_images
-            .iter()
-            .map(move |(k, h)| (k, self.transactions.get(h).unwrap()))
+        self.key_images.iter().map(move |(k, h)| {
+            (
+                k,
+                match self.transactions.get(h) {
+                    Some(tx) => tx,
+                    // todo: something better.
+                    None => panic!("Spentbook is in an inconsistent state"),
+                },
+            )
+        })
     }
 
     pub fn is_spent(&self, key_image: &KeyImage) -> bool {
@@ -141,9 +147,8 @@ impl SpentBookNodeMock {
                             .public_keys()
                             .iter()
                             .flat_map(|pk| {
-                                // fixme: we (ab)use KeyImage G1Affine wrapper to give us Ord trait.
-                                let pki: KeyImage = (*pk).into();
-                                self.outputs.get(&pki)
+                                let pkbm: PublicKeyBlstMappable = (*pk).into();
+                                self.outputs.get(&pkbm)
                             })
                             .collect();
 
@@ -191,10 +196,9 @@ impl SpentBookNodeMock {
             let existing_tx = self.transactions.entry(tx_hash).or_insert_with(|| tx);
 
             // Add public_key:output_proof to public_key index.
-            // fixme: we (ab)use KeyImage G1Affine wrapper to give us Ord trait.
             for output in existing_tx.outputs.iter() {
-                let pki: KeyImage = (*output.public_key()).into();
-                self.outputs.entry(pki).or_insert_with(|| output.clone());
+                let pkbm: PublicKeyBlstMappable = (*output.public_key()).into();
+                self.outputs.entry(pkbm).or_insert_with(|| output.clone());
             }
 
             Ok(SpentProofShare {
@@ -204,7 +208,7 @@ impl SpentBookNodeMock {
                 public_commitments,
             })
         } else {
-            // fixme: return an error.
+            // fixme: return an error.  can wait until we refactor into a Mock feature flag.
             panic!("Attempt to Double Spend")
         }
     }