Signer bug fix (#347)

* trivial leaf vec * changelog * test * add files Signed-off-by: Georgi Zlatarev <georgi.zlatarev@manta.network> * it works * everything ready * test * cleaned up * refactor * docs * final touches * changelog * ignore flag on test * better explanation * final touch * removed useless prints * comments addressed and test de-seeded * added overflow check * remove unnecessary comments * fixed cargo_toml * nullifiermaptype * nullifier map * bug fixed * clone for storagestate * len method --------- Signed-off-by: Georgi Zlatarev <georgi.zlatarev@manta.network> Co-authored-by: Georgi Zlatarev <georgi.zlatarev@manta.network>
Manta-Network · May 19, 2023 · 82606a8 · 82606a8
1 parent 019a4aa
commit 82606a8
Show file tree

Hide file tree

Showing 6 changed files with 272 additions and 41 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,6 +14,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Removed
 
 ### Fixed
+-[\#347](https://github.com/Manta-Network/manta-rs/pull/347) Signer bug fix
 
 ### Security
 

diff --git a/manta-accounting/src/transfer/utxo/protocol.rs b/manta-accounting/src/transfer/utxo/protocol.rs
@@ -2405,9 +2405,7 @@ where
     Eq(bound = "UtxoCommitmentRandomness<C>: Eq"),
     Hash(bound = "UtxoCommitmentRandomness<C>: Hash"),
     Ord(bound = "UtxoCommitmentRandomness<C>: Ord"),
-    PartialEq(
-        bound = "UtxoCommitmentRandomness<C>: core::cmp::PartialEq<UtxoCommitmentRandomness<C>>"
-    ),
+    PartialEq(bound = "UtxoCommitmentRandomness<C>: cmp::PartialEq<UtxoCommitmentRandomness<C>>"),
     PartialOrd(bound = "UtxoCommitmentRandomness<C>: PartialOrd")
 )]
 pub struct Identifier<C>
@@ -2734,8 +2732,7 @@ where
     Debug(bound = "Nullifier<C>: Debug, OutgoingNote<C>: Debug"),
     Default(bound = "Nullifier<C>: Default, OutgoingNote<C>: Default"),
     Eq(bound = "Nullifier<C>: cmp::Eq, OutgoingNote<C>: cmp::Eq"),
-    Hash(bound = "Nullifier<C>: Hash, OutgoingNote<C>: Hash"),
-    PartialEq(bound = "Nullifier<C>: cmp::PartialEq, OutgoingNote<C>: cmp::PartialEq")
+    Hash(bound = "Nullifier<C>: Hash, OutgoingNote<C>: Hash")
 )]
 pub struct FullNullifier<C>
 where
@@ -2782,6 +2779,16 @@ where
     }
 }
 
+impl<C> cmp::PartialEq for FullNullifier<C>
+where
+    C: Configuration<Bool = bool>,
+{
+    #[inline]
+    fn eq(&self, other: &Self) -> bool {
+        !self.is_independent(other)
+    }
+}
+
 impl<C> PartialEq<Self> for FullNullifier<C>
 where
     C: Configuration<Bool = bool>,

diff --git a/manta-accounting/src/wallet/signer/functions.rs b/manta-accounting/src/wallet/signer/functions.rs
@@ -38,9 +38,10 @@ use crate::{
         UtxoAccumulatorItem, UtxoAccumulatorModel, UtxoAccumulatorWitness,
     },
     wallet::signer::{
-        AccountTable, BalanceUpdate, Checkpoint, Configuration, InitialSyncRequest, SignError,
-        SignResponse, SignWithTransactionDataResponse, SignWithTransactionDataResult,
-        SignerParameters, SyncData, SyncError, SyncRequest, SyncResponse,
+        nullifier_map::NullifierMap, AccountTable, BalanceUpdate, Checkpoint, Configuration,
+        InitialSyncRequest, SignError, SignResponse, SignWithTransactionDataResponse,
+        SignWithTransactionDataResult, SignerParameters, SyncData, SyncError, SyncRequest,
+        SyncResponse,
     },
 };
 use alloc::{vec, vec::Vec};
@@ -51,9 +52,7 @@ use manta_crypto::{
     rand::Rand,
 };
 use manta_util::{
-    array_map,
-    cmp::Independence,
-    into_array_unchecked,
+    array_map, into_array_unchecked,
     iter::IteratorExt,
     num::{CheckedAdd, CheckedSub},
     persistence::Rollback,
@@ -150,7 +149,7 @@ fn insert_next_item<C>(
     assets: &mut C::AssetMap,
     parameters: &Parameters<C>,
     identified_asset: IdentifiedAsset<C>,
-    nullifiers: &mut Vec<Nullifier<C>>,
+    nullifiers: &mut C::NullifierMap,
     deposit: &mut Vec<Asset<C>>,
     rng: &mut C::Rng,
 ) where
@@ -163,20 +162,15 @@ fn insert_next_item<C>(
         asset.clone(),
         rng,
     );
-    if let Some(index) = nullifiers
-        .iter()
-        .position(move |n| n.is_related(&nullifier))
-    {
-        nullifiers.remove(index);
+    if nullifiers.contains_item(&nullifier) {
+        utxo_accumulator.insert_nonprovable(&item_hash::<C>(parameters, &utxo));
     } else {
         utxo_accumulator.insert(&item_hash::<C>(parameters, &utxo));
         if !asset.is_zero() {
             deposit.push(asset.clone());
         }
         assets.insert(identifier, asset);
-        return;
     }
-    utxo_accumulator.insert_nonprovable(&item_hash::<C>(parameters, &utxo));
 }
 
 /// Checks if `asset` matches with `nullifier`, removing it from the `utxo_accumulator` and
@@ -189,7 +183,7 @@ fn is_asset_unspent<C>(
     parameters: &Parameters<C>,
     identifier: Identifier<C>,
     asset: Asset<C>,
-    nullifiers: &mut Vec<Nullifier<C>>,
+    nullifiers: &mut C::NullifierMap,
     withdraw: &mut Vec<Asset<C>>,
     rng: &mut C::Rng,
 ) -> bool
@@ -198,11 +192,7 @@ where
 {
     let (_, utxo, nullifier) =
         parameters.derive_spend(authorization_context, identifier, asset.clone(), rng);
-    if let Some(index) = nullifiers
-        .iter()
-        .position(move |n| n.is_related(&nullifier))
-    {
-        nullifiers.remove(index);
+    if nullifiers.contains_item(&nullifier) {
         utxo_accumulator.remove_proof(&item_hash::<C>(parameters, &utxo));
         if !asset.is_zero() {
             withdraw.push(asset);
@@ -219,11 +209,12 @@ where
 fn sync_with<C, I>(
     authorization_context: &mut AuthorizationContext<C>,
     assets: &mut C::AssetMap,
+    nullifiers: &mut C::NullifierMap,
     checkpoint: &mut C::Checkpoint,
     utxo_accumulator: &mut C::UtxoAccumulator,
     parameters: &Parameters<C>,
     inserts: I,
-    mut nullifiers: Vec<Nullifier<C>>,
+    nullifier_data: Vec<Nullifier<C>>,
     is_partial: bool,
     rng: &mut C::Rng,
 ) -> Result<SyncResponse<C, C::Checkpoint>, SyncError<C::Checkpoint>>
@@ -232,7 +223,8 @@ where
     I: Iterator<Item = (Utxo<C>, Note<C>)>,
     C::AssetValue: CheckedAdd<Output = C::AssetValue> + CheckedSub<Output = C::AssetValue>,
 {
-    let nullifier_count = nullifiers.len();
+    let nullifier_count = nullifier_data.len();
+    nullifiers.extend(nullifier_data);
     let mut deposit = Vec::new();
     let mut withdraw = Vec::new();
     let decryption_key = parameters.derive_decryption_key(authorization_context);
@@ -250,7 +242,7 @@ where
                 assets,
                 parameters,
                 transfer::utxo::IdentifiedAsset::new(identifier, asset),
-                &mut nullifiers,
+                nullifiers,
                 &mut deposit,
                 rng,
             );
@@ -269,7 +261,7 @@ where
                 parameters,
                 identifier.clone(),
                 asset.clone(),
-                &mut nullifiers,
+                nullifiers,
                 &mut withdraw,
                 rng,
             )
@@ -852,11 +844,13 @@ where
 }
 
 /// Updates `assets`, `checkpoint` and `utxo_accumulator`, returning the new asset distribution.
+#[allow(clippy::too_many_arguments)] // This function must take 8 arguments
 #[inline]
 pub fn sync<C>(
     parameters: &SignerParameters<C>,
     authorization_context: &mut AuthorizationContext<C>,
     assets: &mut C::AssetMap,
+    nullifiers: &mut C::NullifierMap,
     checkpoint: &mut C::Checkpoint,
     utxo_accumulator: &mut C::UtxoAccumulator,
     request: SyncRequest<C, C::Checkpoint>,
@@ -876,6 +870,7 @@ where
     let response = sync_with::<C, _>(
         authorization_context,
         assets,
+        nullifiers,
         checkpoint,
         utxo_accumulator,
         &parameters.parameters,