Merge pull request #2672 from dusk-network/mocello/fix_transfer_tree_…

…filter

transfer-contract: Fix tree-filter for transparent notes with 0 value

contracts/transfer/src/state.rs

@@ -645,15 +645,9 @@ impl TransferState {
                 let remainder_note =
                     tx.fee().gen_remainder_note(gas_spent, deposit);
 
-                let remainder_value = remainder_note
-                    .value(None)
-                    .expect("Should always succeed for a transparent note");
-
-                let refund_info = if remainder_value > 0 {
-                    Some(self.push_note_current_height(remainder_note))
-                } else {
-                    None
-                };
+                // if the refund-value is 0, we don't push the note onto the
+                // tree and the refund-note will be None
+                let refund_note = self.push_note_current_height(remainder_note);
 
                 rusk_abi::emit(
                     PHOENIX_TOPIC,
@@ -662,7 +656,7 @@ impl TransferState {
                         notes,
                         memo,
                         gas_spent,
-                        refund_info,
+                        refund_note,
                     },
                 );
             }
@@ -874,11 +868,11 @@ impl TransferState {
         self.roots.contains(root)
     }
 
-    pub fn push_note(&mut self, block_height: u64, note: Note) -> Note {
+    pub fn push_note(&mut self, block_height: u64, note: Note) -> Option<Note> {
         self.tree.push(NoteLeaf { block_height, note })
     }
 
-    fn push_note_current_height(&mut self, note: Note) -> Note {
+    fn push_note_current_height(&mut self, note: Note) -> Option<Note> {
         let block_height = rusk_abi::block_height();
         self.push_note(block_height, note)
     }

contracts/transfer/src/tree.rs

@@ -11,22 +11,42 @@ use execution_core::{
     BlsScalar,
 };
 
+/// The merkle tree that holds all phoenix-notes.
+///
+/// This tree is append only. When a note is spend its `nullifier` will be
+/// added to the nullifier-set in the transfer-contract's state.
+/// To get all unspend phoenix-notes one needs to get all owned notes and
+/// remove the ones who's nullifiers are already in the nullifier-set of the
+/// transfer-contract.
+/// To help with sync-time, we store the block-height at which a note has been
+/// added to the tree together with the note itself.
 pub struct Tree {
+    // Merkle tree of the note-hashes.
     tree: NotesTree,
-    // Since `dusk-merkle` does not include data blocks with the tree, we do it
-    // here.
+    // Since the merkle-tree only includes the note-hashes, we we store the
+    // actual notes (and their respective block-height) here.
+    // The index of a note in this vector corresponds to the position of its
+    // hash in the merkle-tree.
     leaves: Vec<NoteLeaf>,
 }
 
 impl Tree {
+    /// Create a new empty tree.
     pub const fn new() -> Self {
         Self {
             tree: NotesTree::new(),
             leaves: Vec::new(),
         }
     }
 
-    pub fn push(&mut self, mut leaf: NoteLeaf) -> Note {
+    /// Push one [`NoteLeaf`] onto the tree, filtering out notes that are
+    /// transparent with a value of 0.
+    pub fn push(&mut self, mut leaf: NoteLeaf) -> Option<Note> {
+        // skip transparent notes with a value of 0
+        if leaf.note.value(None).is_ok_and(|value| value == 0) {
+            return None;
+        }
+
         // update the position before computing the hash
         let pos = self.leaves.len() as u64;
         leaf.note.set_pos(pos);
@@ -38,61 +58,64 @@ impl Tree {
         self.tree.insert(pos, item);
         self.leaves.push(leaf.clone());
 
-        leaf.note
+        Some(leaf.note)
     }
 
+    /// Extend the tree with multiple [`NoteLeaf`] of the same block-height,
+    /// filtering out notes that are transparent with a value of 0.
     pub fn extend_notes<I: IntoIterator<Item = Note>>(
         &mut self,
         block_height: u64,
         notes: I,
     ) -> Vec<Note> {
-        let mut n = Vec::new();
+        let mut notes_vec = Vec::new();
 
         for note in notes {
-            // skip transparent notes with a value of 0
-            if !note.value(None).is_ok_and(|value| value == 0) {
-                let note = self.push(NoteLeaf { block_height, note });
-                n.push(note);
+            if let Some(note) = self.push(NoteLeaf { block_height, note }) {
+                notes_vec.push(note);
             }
         }
 
-        n
+        notes_vec
     }
 
+    /// Return the root of the merkle tree of notes.
     pub fn root(&self) -> BlsScalar {
         self.tree.root().hash
     }
 
     /// Return an iterator through the leaves in the tree, starting from a given
-    /// `height`.
-    pub fn leaves(&self, height: u64) -> impl Iterator<Item = &NoteLeaf> {
+    /// `block_height`.
+    pub fn leaves(&self, block_height: u64) -> impl Iterator<Item = &NoteLeaf> {
         // We can do this since we know the leaves are strictly increasing in
-        // block height. If this ever changes - such as in the case of a
+        // block-height. If this ever changes - such as in the case of a
         // sparsely populated tree - we should annotate the tree and use
         // `Tree::walk` instead.
         self.leaves
             .iter()
-            .skip_while(move |leaf| leaf.block_height < height)
+            .skip_while(move |leaf| leaf.block_height < block_height)
     }
 
     /// Return an iterator through the leaves in the tree, starting from a given
     /// `position`.
     pub fn leaves_pos(&self, pos: u64) -> impl Iterator<Item = &NoteLeaf> {
-        // We can do this since we know the leaves are strictly increasing in
-        // block height. If this ever changes - such as in the case of a
-        // sparsely populated tree - we should annotate the tree and use
-        // `Tree::walk` instead.
+        // We can do this since we know that, with increasing position, the
+        // leaves are strictly increasing in block-height. If this ever changes
+        // - such as in the case of a sparsely populated tree - we should
+        // annotate the tree and use `Tree::walk` instead.
         let pos = pos as usize;
         if self.leaves.len() < pos {
             return self.leaves[..0].iter();
         }
         self.leaves[pos..].iter()
     }
 
+    /// Return the merkle-opening for a note at a given position.
     pub fn opening(&self, pos: u64) -> Option<NoteOpening> {
         self.tree.opening(pos)
     }
 
+    /// Return the amount of leaves, i.e. notes, that are stored in the tree.
     pub fn leaves_len(&self) -> u64 {
         self.leaves.len() as u64
     }

execution-core/src/transfer.rs

@@ -503,7 +503,7 @@ pub struct PhoenixTransactionEvent {
     /// Gas spent by the transaction.
     pub gas_spent: u64,
     /// Optional gas-refund note if the refund is positive.
-    pub refund_info: Option<Note>,
+    pub refund_note: Option<Note>,
 }
 
 /// Event data emitted on a moonlight transaction's completion.