From e4203ee14b51159833b5d7115274b8c5e3b36517 Mon Sep 17 00:00:00 2001 From: grumbach Date: Tue, 23 Jul 2024 19:12:58 +0200 Subject: [PATCH] fix: spends fixes and security verifications --- sn_client/src/audit/spend_dag.rs | 37 +++++++++---------- sn_networking/src/lib.rs | 8 ++-- sn_networking/src/transfers.rs | 9 +++-- sn_node/tests/double_spend.rs | 6 +-- sn_transfers/benches/reissue.rs | 4 +- sn_transfers/src/cashnotes.rs | 9 +---- sn_transfers/src/cashnotes/cashnote.rs | 25 +++++-------- sn_transfers/src/error.rs | 4 +- sn_transfers/src/genesis.rs | 1 - .../src/transfers/signed_transaction.rs | 6 +-- .../src/transfers/unsigned_transaction.rs | 2 - sn_transfers/src/wallet/hot_wallet.rs | 2 +- 12 files changed, 49 insertions(+), 64 deletions(-) diff --git a/sn_client/src/audit/spend_dag.rs b/sn_client/src/audit/spend_dag.rs index 388bd9dbfc..69a3c327b2 100644 --- a/sn_client/src/audit/spend_dag.rs +++ b/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,14 +739,14 @@ impl SpendDag { } /// Verifies a single spend and returns resulting errors and DAG poisoning spread - fn verify_parent_tx(&self, spend: &SignedSpend) -> Result, DagError> { + fn verify_spend_parents(&self, spend: &SignedSpend) -> Result, 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); } @@ -757,7 +754,7 @@ impl SpendDag { 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); } diff --git a/sn_networking/src/lib.rs b/sn_networking/src/lib.rs index 90333b311a..58778d509e 100644 --- a/sn_networking/src/lib.rs +++ b/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. diff --git a/sn_networking/src/transfers.rs b/sn_networking/src/transfers.rs index ce538594a0..960e37c711 100644 --- a/sn_networking/src/transfers.rs +++ b/sn_networking/src/transfers.rs @@ -120,7 +120,7 @@ impl Network { ) -> Result> { // 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 = cashnote_redemptions @@ -144,7 +144,6 @@ impl Network { let our_output_cash_notes: Vec = 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 = cnr @@ -161,7 +160,6 @@ impl Network { .collect(); CashNote { - unique_pubkey, parent_spends: parent_spends.clone(), main_pubkey, derivation_index, @@ -169,6 +167,11 @@ impl Network { }) .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) } } diff --git a/sn_node/tests/double_spend.rs b/sn_node/tests/double_spend.rs index c9adffd8fc..a78f551f31 100644 --- a/sn_node/tests/double_spend.rs +++ b/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()); diff --git a/sn_transfers/benches/reissue.rs b/sn_transfers/benches/reissue.rs index cf54d2b4e3..165b46face 100644 --- a/sn_transfers/benches/reissue.rs +++ b/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(); }); diff --git a/sn_transfers/src/cashnotes.rs b/sn_transfers/src/cashnotes.rs index 90c3a45f14..1e3878581f 100644 --- a/sn_transfers/src/cashnotes.rs +++ b/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(()) diff --git a/sn_transfers/src/cashnotes/cashnote.rs b/sn_transfers/src/cashnotes/cashnote.rs index 944ac2ffda..dd6bc1aff2 100644 --- a/sn_transfers/src/cashnotes/cashnote.rs +++ b/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, /// 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() diff --git a/sn_transfers/src/error.rs b/sn_transfers/src/error.rs index 4dab28112e..8e17a20934 100644 --- a/sn_transfers/src/error.rs +++ b/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")] diff --git a/sn_transfers/src/genesis.rs b/sn_transfers/src/genesis.rs index 59af3e1751..15b221df4b 100644 --- a/sn_transfers/src/genesis.rs +++ b/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, diff --git a/sn_transfers/src/transfers/signed_transaction.rs b/sn_transfers/src/transfers/signed_transaction.rs index 5e07eb92c1..7efb337369 100644 --- a/sn_transfers/src/transfers/signed_transaction.rs +++ b/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()?; diff --git a/sn_transfers/src/transfers/unsigned_transaction.rs b/sn_transfers/src/transfers/unsigned_transaction.rs index bb9e9164e5..63e00c86ce 100644 --- a/sn_transfers/src/transfers/unsigned_transaction.rs +++ b/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, diff --git a/sn_transfers/src/wallet/hot_wallet.rs b/sn_transfers/src/wallet/hot_wallet.rs index 4829dbf87a..80147b0ca4 100644 --- a/sn_transfers/src/wallet/hot_wallet.rs +++ b/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:?}" )));