fix: spends fixes and security verifications

sn_client/src/audit/spend_dag.rs

@@ -12,8 +12,8 @@ use petgraph::graph::{DiGraph, NodeIndex};
 use petgraph::visit::EdgeRef;
 use serde::{Deserialize, Serialize};
 use sn_transfers::{
-    is_genesis_spend, CashNoteRedemption, DerivationIndex, Hash, NanoTokens, OutputPurpose,
-    SignedSpend, SpendAddress, UniquePubkey,
+    is_genesis_spend, CashNoteRedemption, DerivationIndex, Hash, NanoTokens, SignedSpend,
+    SpendAddress, UniquePubkey,
 };
 use std::{
     collections::{BTreeMap, BTreeSet},
@@ -409,15 +409,12 @@ impl SpendDag {
             Vec<(DerivationIndex, SpendAddress)>,
         > = BTreeMap::new();
         for s in spends {
-            for (unique_pk, (_amount, purpose)) in s.spend.descendants.iter() {
-                if let OutputPurpose::RoyaltyFee(derivation_index) = purpose {
-                    let parent_spend_addr =
-                        SpendAddress::from_unique_pubkey(&s.spend.unique_pubkey);
-                    royalties_by_unique_pk
-                        .entry(*unique_pk)
-                        .and_modify(|v| v.push((*derivation_index, parent_spend_addr)))
-                        .or_insert(vec![(*derivation_index, parent_spend_addr)]);
-                }
+            let parent_spend_addr = SpendAddress::from_unique_pubkey(&s.spend.unique_pubkey);
+            for (roy_pk, _, derivation_idx) in s.spend.network_royalties() {
+                royalties_by_unique_pk
+                    .entry(roy_pk)
+                    .and_modify(|v| v.push((derivation_idx, parent_spend_addr)))
+                    .or_insert(vec![(derivation_idx, parent_spend_addr)]);
             }
         }
 
@@ -728,9 +725,9 @@ impl SpendDag {
                 continue;
             }
 
-            // verify parent Tx
+            // verify parents
             for s in spends {
-                recorded_faults.extend(self.verify_parent_tx(s)?);
+                recorded_faults.extend(self.verify_spend_parents(s)?);
             }
         }
 
@@ -742,22 +739,22 @@ impl SpendDag {
     }
 
     /// Verifies a single spend and returns resulting errors and DAG poisoning spread
-    fn verify_parent_tx(&self, spend: &SignedSpend) -> Result<BTreeSet<SpendFault>, DagError> {
+    fn verify_spend_parents(&self, spend: &SignedSpend) -> Result<BTreeSet<SpendFault>, DagError> {
         let addr = spend.address();
         let mut recorded_faults = BTreeSet::new();
-        debug!("Verifying spend at: {addr:?}");
+        debug!("Verifying spend: {spend:?}");
 
         // skip if spend matches genesis
         if is_genesis_spend(spend) {
-            debug!("Skip transaction verification for Genesis at: {addr:?}");
+            debug!("Skip transaction verification for Genesis: {spend:?}");
             return Ok(recorded_faults);
         }
 
         // get the ancestors of this spend
         let (ancestor_spends, faults) = match self.get_direct_ancestors(spend) {
             Ok(a) => a,
             Err(missing_ancestor) => {
-                debug!("Failed to get ancestor spends of {addr:?} as ancestor at {missing_ancestor:?} is missing");
+                debug!("Failed to get ancestor spends of {spend:?} as ancestor at {missing_ancestor:?} is missing");
                 recorded_faults.insert(SpendFault::MissingAncestry {
                     addr,
                     ancestor: missing_ancestor,
@@ -778,11 +775,11 @@ impl SpendDag {
             let _ = parents_collector.insert(collector);
         }
 
-        // verify the tx
+        // verify the parents
         if let Err(e) = spend.verify_parent_spends(parents_collector.iter()) {
-            warn!("Parent Tx verfication failed for spend at: {addr:?}: {e}");
+            warn!("Parent verfication failed for spend at: {spend:?}: {e}");
             recorded_faults.insert(SpendFault::InvalidTransaction(addr, format!("{e}")));
-            let poison = format!("ancestor transaction was poisoned at: {addr:?}: {e}");
+            let poison = format!("ancestor transaction was poisoned at: {spend:?}: {e}");
             let descendants_faults = self.poison_all_descendants(spend, poison)?;
             recorded_faults.extend(descendants_faults);
         }

sn_networking/src/lib.rs

@@ -125,10 +125,10 @@ pub fn sort_peers_by_key<'a, T>(
     // bail early if that's not the case
     if CLOSE_GROUP_SIZE > peers.len() {
         warn!("Not enough peers in the k-bucket to satisfy the request");
-        // return Err(NetworkError::NotEnoughPeers {
-        //     found: peers.len(),
-        //     required: CLOSE_GROUP_SIZE,
-        // });
+        return Err(NetworkError::NotEnoughPeers {
+            found: peers.len(),
+            required: CLOSE_GROUP_SIZE,
+        });
     }
 
     // Create a vector of tuples where each tuple is a reference to a peer and its distance to the key.

sn_networking/src/transfers.rs

@@ -120,7 +120,7 @@ impl Network {
     ) -> Result<Vec<CashNote>> {
         // get all the parent spends
         trace!(
-            "Getting parent Tx for validation from {:?}",
+            "Getting parent spends for validation from {:?}",
             cashnote_redemptions.len()
         );
         let parent_addrs: BTreeSet<SpendAddress> = cashnote_redemptions
@@ -144,7 +144,6 @@ impl Network {
         let our_output_cash_notes: Vec<CashNote> = cashnote_redemptions
             .iter()
             .map(|cnr| {
-                let unique_pubkey = main_pubkey.new_unique_pubkey(&cnr.derivation_index);
                 let derivation_index = cnr.derivation_index;
                 // assuming parent spends all exist as they were collected just above
                 let parent_spends: BTreeSet<SignedSpend> = cnr
@@ -161,14 +160,18 @@ impl Network {
                     .collect();
 
                 CashNote {
-                    unique_pubkey,
                     parent_spends: parent_spends.clone(),
                     main_pubkey,
                     derivation_index,
                 }
             })
             .collect();
 
+        // verify our output cash notes
+        for cash_note in our_output_cash_notes.iter() {
+            cash_note.verify().map_err(|e| NetworkError::InvalidTransfer(format!("Invalid CashNoteRedemption: {e}")))?;
+        }
+
         Ok(our_output_cash_notes)
     }
 }

sn_node/tests/double_spend.rs

@@ -451,7 +451,7 @@ async fn parent_and_child_double_spends_should_lead_to_cashnote_being_invalid()
     let result = client.verify_cashnote(&cash_notes_for_y[0]).await;
     info!("Got result while verifying double spend from B -> Y: {result:?}");
     assert_matches!(result, Err(WalletError::CouldNotVerifyTransfer(str)) => {
-        assert!(str.starts_with("The spends in network were not the same as the ones in the CashNote. The parents of this CashNote are probably double spends."));
+        assert!(str.starts_with("Network Error Double spend(s) was detected"));
     });
 
     info!("Verifying the original cashnote of A -> B");
@@ -461,7 +461,7 @@ async fn parent_and_child_double_spends_should_lead_to_cashnote_being_invalid()
         assert!(str.starts_with("Network Error Double spend(s) was detected"));
     });
 
-    println!("Verifying the original cashnote of B -> C");
+    info!("Verifying the original cashnote of B -> C");
     let result = client.verify_cashnote(&cash_notes_for_c[0]).await;
     info!("Got result while verifying the original spend from B -> C: {result:?}");
     assert_matches!(result, Err(WalletError::CouldNotVerifyTransfer(str)) => {
@@ -598,7 +598,7 @@ async fn spamming_double_spends_should_not_shadow_live_branch() -> Result<()> {
 
     // Try to double spend A -> n different random keys
     for _ in 0..20 {
-        println!("Spamming double spends on A");
+        info!("Spamming double spends on A");
         let wallet_dir_y = TempDir::new()?;
         let wallet_y = get_wallet(wallet_dir_y.path());
         assert_eq!(wallet_y.balance(), NanoTokens::zero());

sn_transfers/benches/reissue.rs

@@ -57,7 +57,7 @@ fn bench_reissue_1_to_100(c: &mut Criterion) {
         let guard = pprof::ProfilerGuard::new(100).unwrap();
 
         b.iter(|| {
-            black_box(&signed_tx).verify(&starting_main_key).unwrap();
+            black_box(&signed_tx).verify().unwrap();
         });
 
         #[cfg(unix)]
@@ -135,7 +135,7 @@ fn bench_reissue_100_to_1(c: &mut Criterion) {
 
         b.iter(|| {
             black_box(&many_to_one_tx)
-                .verify(&starting_main_key)
+                .verify()
                 .unwrap();
         });
 

sn_transfers/src/cashnotes.rs

@@ -69,7 +69,6 @@ pub(crate) mod tests {
         );
 
         let cashnote = CashNote {
-            unique_pubkey: derived_key.unique_pubkey(),
             parent_spends,
             main_pubkey: main_key.main_pubkey(),
             derivation_index,
@@ -98,7 +97,6 @@ pub(crate) mod tests {
         );
 
         let cashnote = CashNote {
-            unique_pubkey: derived_key.unique_pubkey(),
             parent_spends,
             main_pubkey: main_key.main_pubkey(),
             derivation_index,
@@ -129,7 +127,6 @@ pub(crate) mod tests {
         );
 
         let cashnote = CashNote {
-            unique_pubkey: derived_key.unique_pubkey(),
             parent_spends,
             main_pubkey: main_key.main_pubkey(),
             derivation_index,
@@ -150,18 +147,16 @@ pub(crate) mod tests {
 
         let main_key = MainSecretKey::random_from_rng(&mut rng);
         let derivation_index = DerivationIndex::random(&mut rng);
-        let derived_key = main_key.derive_key(&derivation_index);
 
         let cashnote = CashNote {
-            unique_pubkey: derived_key.unique_pubkey(),
             parent_spends: Default::default(),
             main_pubkey: main_key.main_pubkey(),
             derivation_index,
         };
 
         assert!(matches!(
-            cashnote.verify(&main_key),
-            Err(TransferError::MissingTxInputs)
+            cashnote.verify(),
+            Err(TransferError::CashNoteMissingAncestors)
         ));
 
         Ok(())

sn_transfers/src/cashnotes/cashnote.rs

@@ -58,9 +58,6 @@ use tiny_keccak::{Hasher, Sha3};
 /// eg: `cashnote.derivation_index(&main_key)`
 #[derive(custom_debug::Debug, Clone, Eq, PartialEq, Serialize, Deserialize, Hash)]
 pub struct CashNote {
-    /// The unique public key of this CashNote. It is unique, and there can never
-    /// be another CashNote with the same public key. It used in SignedSpends.
-    pub unique_pubkey: UniquePubkey,
     /// The transaction's input's SignedSpends
     pub parent_spends: BTreeSet<SignedSpend>,
     /// This is the MainPubkey of the owner of this CashNote
@@ -74,7 +71,7 @@ pub struct CashNote {
 impl CashNote {
     /// Return the unique pubkey of this CashNote.
     pub fn unique_pubkey(&self) -> UniquePubkey {
-        self.unique_pubkey
+        self.main_pubkey().new_unique_pubkey(&self.derivation_index())
     }
 
     // Return MainPubkey from which UniquePubkey is derived.
@@ -135,21 +132,17 @@ impl CashNote {
         Hash::from(hash)
     }
 
-    /// Verifies that this CashNote is valid.
-    ///
-    /// A CashNote recipient should call this immediately upon receipt.
-    ///
-    /// important: this does not check if the CashNote has been spent.
-    /// For that, one must query the spentbook nodes.
-    ///
-    /// Note that the spentbook nodes cannot perform this check.  Only the CashNote
-    /// recipient (private key holder) can.
-    pub fn verify(&self, main_key: &MainSecretKey) -> Result<(), TransferError> {
+    /// Verifies that this CashNote is valid. This checks that the CashNote is structurally sound.
+    /// Important: this does not check if the CashNote has been spent, nor does it check if the parent spends are spent.
+    /// For that, one must query the Network.
+    pub fn verify(&self) -> Result<(), TransferError> {
+        // check if we have parents
         if self.parent_spends.is_empty() {
-            return Err(TransferError::MissingTxInputs);
+            return Err(TransferError::CashNoteMissingAncestors);
         }
 
-        let unique_pubkey = self.derived_key(main_key)?.unique_pubkey();
+        // check if the parents refer to us as a descendant
+        let unique_pubkey = self.unique_pubkey();
         if !self
             .parent_spends
             .iter()

sn_transfers/src/error.rs

@@ -39,8 +39,8 @@ pub enum TransferError {
     HexDeserializationFailed(String),
     #[error("Could not serialize CashNote to hex: {0}")]
     HexSerializationFailed(String),
-    #[error("Transaction must have at least one input.")]
-    MissingTxInputs,
+    #[error("CashNote must have at least one ancestor.")]
+    CashNoteMissingAncestors,
     #[error("The spends don't match the inputs of the Transaction.")]
     SpendsDoNotMatchInputs,
     #[error("Overflow occurred while adding values")]

sn_transfers/src/genesis.rs

@@ -209,7 +209,6 @@ pub fn create_first_cash_note_from_key(
     let parent_spends = BTreeSet::from_iter([SignedSpend::sign(pre_genesis_spend, &input_sk)]);
 
     let genesis_cash_note = CashNote {
-        unique_pubkey: output_pk,
         parent_spends,
         main_pubkey,
         derivation_index: GENESIS_OUTPUT_DERIVATION_INDEX,

sn_transfers/src/transfers/signed_transaction.rs

@@ -48,12 +48,12 @@ impl SignedTransaction {
     }
 
     /// Verify the `SignedTransaction`
-    pub fn verify(&self, main_key: &MainSecretKey) -> Result<()> {
+    pub fn verify(&self) -> Result<()> {
         for cn in self.output_cashnotes.iter() {
-            cn.verify(main_key)?;
+            cn.verify()?;
         }
         if let Some(ref cn) = self.change_cashnote {
-            cn.verify(main_key)?;
+            cn.verify()?;
         }
         for spend in self.spends.iter() {
             spend.verify()?;

sn_transfers/src/transfers/unsigned_transaction.rs

@@ -66,7 +66,6 @@ impl UnsignedTransaction {
             .iter()
             .map(|(amount, main_pk, derivation_index, purpose)| {
                 let cn = CashNote {
-                    unique_pubkey: main_pk.new_unique_pubkey(derivation_index),
                     parent_spends: BTreeSet::new(),
                     main_pubkey: *main_pk,
                     derivation_index: *derivation_index,
@@ -112,7 +111,6 @@ impl UnsignedTransaction {
 
                     // assign the change cash note
                     change_cn = Some(CashNote {
-                        unique_pubkey: change_key,
                         parent_spends: BTreeSet::new(),
                         main_pubkey: change_to,
                         derivation_index: change_derivation_index,

sn_transfers/src/wallet/hot_wallet.rs

@@ -349,7 +349,7 @@ impl HotWallet {
         let signed_tx = unsigned_tx
             .sign(&self.key)
             .map_err(|e| Error::CouldNotSignTransaction(e.to_string()))?;
-        if let Err(err) = signed_tx.verify(&self.key) {
+        if let Err(err) = signed_tx.verify() {
             return Err(Error::CouldNotSignTransaction(format!(
                 "Failed to verify signed transaction: {err:?}"
             )));