feat: added wrapper for Mmr #668

varun-doshi · 2025-02-05T08:01:12Z

Ideally should be merged after #667 is merged so that we can directly send in the Iterator instead of collecting into a Vec

PhilippGackstatter

Thanks for working on this. I left some comments inline.

PhilippGackstatter · 2025-02-10T14:53:52Z

crates/store/src/state.rs

@@ -96,17 +96,114 @@ pub struct TransactionInputs {
    pub found_unauthenticated_notes: BTreeSet<NoteId>,
 }

+#[derive(Debug)]


We should also derive Clone.

PhilippGackstatter · 2025-02-10T14:54:04Z

crates/store/src/state.rs

+impl Blockchain {
+    pub fn get(&self) -> &Mmr {
+        &self.0
+    }


All public methods should be documented.

crates/store/src/state.rs

PhilippGackstatter · 2025-02-10T15:15:00Z

crates/store/src/state.rs

+    pub async fn get_chain_mmr(
+        &self,
+        blocks: &mut BTreeSet<BlockNumber>,
+        db: &Arc<Db>,
+    ) -> Result<(ChainMmr, Vec<miden_objects::block::BlockHeader>, BlockNumber), GetBatchInputsError>


We should pass in the block immutably, e.g. &BTreeSet<...>.
We also shouldn't pass in the database. This function should only deal with selectively copying part of the Mmr to the ChainMmr it creates.
The logic for selecting the block headers from the database should be done in get_batch_inputs by adding the returned batch reference block number to the headers we want to select.

Please correct me if im wrong but this line requires a mutable ref no?

blocks.remove(&latest_block_num);

It does if we want to do the logic as it is now, but I think we can compute the highest_block_num in get_batch_inputs, remove it from blocks and then pass the blocks and the highest block num, i.e.: get_chain_mmr(blocks: &BTreeSet<BlockNumber>, highest_block_num: BlockNumber). Does that make sense?

Understood
Will get it done

PhilippGackstatter · 2025-02-10T15:15:47Z

crates/store/src/state.rs

+        // TODO: Unnecessary conversion. We should change the select_block_headers function to take
+        // an impl Iterator instead to avoid this allocation.
+        let mut blocks: Vec<_> = blocks.clone().into_iter().collect();
+        // Fetch the reference block of the batch as part of this query, so we can avoid looking it
+        // up in a separate DB access.
+        blocks.push(batch_reference_block);


Can you merge in next and update this function with the changes from next?

PhilippGackstatter · 2025-02-10T15:16:10Z

crates/store/src/state.rs

+        let chain_mmr =
+            ChainMmr::new(partial_mmr, headers.clone()).expect("Unable to build ChainMmr");


When you move this back to get_batch_inputs, please also re-add the SAFETY comment and revert to the previous expect message.

crates/store/src/state.rs

PhilippGackstatter

Thanks for the updates! I left some comments.

PhilippGackstatter · 2025-02-14T13:13:42Z

crates/store/src/state.rs

+pub struct Blockchain(Mmr);
+
+impl Blockchain {
+    /// Returns the latest block number


Nit: Can you add . at the end of all doc comments?

PhilippGackstatter · 2025-02-14T13:17:33Z

crates/store/src/state.rs

+    /// Adds a new element to the MMR.
+    pub fn add(&mut self, el: RpoDigest) {
+        self.0.add(el);
+    }


Suggested change

/// Adds a new element to the MMR.

pub fn add(&mut self, el: RpoDigest) {

self.0.add(el);

}

/// Adds a block commitment to the MMR. The caller must ensure that this commitent is the one for the next block in the chain.

pub fn add(&mut self, block_commitment: RpoDigest) {

self.0.add(block_commitment);

}

PhilippGackstatter · 2025-02-14T13:19:41Z

crates/store/src/state.rs

    account_tree: SimpleSmt<ACCOUNT_TREE_DEPTH>,
 }

 impl InnerState {
    /// Returns the latest block number.
    fn latest_block_num(&self) -> BlockNumber {
-        let block_number: u32 = (self.chain_mmr.forest() - 1)
+        let block_number: u32 = (self.blockchain.0.forest() - 1)


The idea of adding methods like Blockchain::chain_tip is that we can call them here, so we can avoid accessing the inner Mmr directly. So all blockchain.0.xyz() methods should be replaced by calling methods directly, e.g. blockchain.xyz() (or with blockchain-specific names, so blockchain.chain_tip() instead of blockchain.0.forest().

Here we should be able to call chain_tip, right?

PhilippGackstatter · 2025-02-14T13:22:07Z

crates/store/src/state.rs

-        let inner = RwLock::new(InnerState { nullifier_tree, chain_mmr, account_tree });
+        let inner = RwLock::new(InnerState {
+            nullifier_tree,
+            blockchain: Blockchain(chain_mmr),


Can we create a proper constructor for this, so Blockchain::new(chain_mmr)?

PhilippGackstatter · 2025-02-14T13:24:43Z

crates/store/src/state.rs

-            .map_err(GetBatchInputsError::SelectBlockHeaderError)?;
+            .expect("partial mmr and block headers should be consistent");


I think we still want to error here instead of panic.

What I meant here was to revert this change and use .map_err(GetBatchInputsError::SelectBlockHeaderError)?;

PhilippGackstatter · 2025-02-14T13:36:07Z

crates/store/src/state.rs

+        //
+        // NOTE: Scoped block to automatically drop the mutex guard asap.
+        //
+        // We also avoid accessing the db in the block as this would delay
+        // dropping the guard.


This part of the comment is no longer relevant, so we can remove it.

PhilippGackstatter · 2025-02-14T13:36:56Z

crates/store/src/state.rs

+    }
+
+    /// Returns the latest block number and partial mmr
+    pub fn chain_mmr(


This actually only constructs a partial MMR now, so we should name it accordingly, e.g. partial_mmr_from_blocks or something.

PhilippGackstatter · 2025-02-14T13:38:11Z

crates/store/src/state.rs

+    pub fn chain_mmr(
+        &self,
+        blocks: &BTreeSet<BlockNumber>,
+        highest_block_number: &BlockNumber,


My bad, this should actually be called latest_block_number.

There seems to have been a misunderstanding.

I meant this:

pub fn partial_mmr_from_blocks( &self, blocks: &BTreeSet<BlockNumber>, latest_block_number: BlockNumber, ) -> Result<PartialMmr, GetBatchInputsError> { ... }

Sorry for being ambiguous.

We don't need to return the block number because it's always the same as the latest_block_number we pass in.

We also can take a BlockNumber instead of a &BlockNumber since the type is Copy.

PhilippGackstatter · 2025-02-14T13:39:24Z

crates/store/src/state.rs

-            (latest_block_num, partial_mmr)
-        };
+        let (batch_reference_block, partial_mmr) =
+            self.inner.blocking_read().blockchain.chain_mmr(&blocks, &highest_block_num)?;


We need to pass latest_block_num here instead, and we should probably acquire the RwLock just once and drop it as soon as we can, so everything together:

// Scoped block to automatically drop the read lock guard as soon as we're done. // We also avoid accessing the db in the block as this would delay dropping the guard. let (batch_reference_block, partial_mmr) = { let inner_state = self.inner.blocking_read(); let latest_block_num = inner_state.blockchain.chain_tip(); let highest_block_num = *blocks.last().expect("we should have checked for empty block references"); if highest_block_num > latest_block_num { return Err(GetBatchInputsError::TransactionBlockReferenceNewerThanLatestBlock { highest_block_num, latest_block_num, }); } // Remove the latest block from the to-be-tracked blocks as it will be the reference // block for the batch itself and thus added to the MMR within the batch kernel, so // there is no need to prove its inclusion. blocks.remove(&latest_block_num); inner_state.blockchain.chain_mmr(&blocks, &latest_block_num)? };

PhilippGackstatter

Thanks for the updates! I think we're close to finishing this. I left another round of comments.

PhilippGackstatter · 2025-02-17T09:47:39Z

crates/store/src/state.rs

+    pub fn chain_mmr(
+        &self,
+        blocks: &BTreeSet<BlockNumber>,
+        highest_block_number: &BlockNumber,


There seems to have been a misunderstanding.

I meant this:

pub fn partial_mmr_from_blocks( &self, blocks: &BTreeSet<BlockNumber>, latest_block_number: BlockNumber, ) -> Result<PartialMmr, GetBatchInputsError> { ... }

Sorry for being ambiguous.

We don't need to return the block number because it's always the same as the latest_block_number we pass in.

We also can take a BlockNumber instead of a &BlockNumber since the type is Copy.

PhilippGackstatter · 2025-02-17T09:50:56Z

crates/store/src/state.rs

-            .map_err(GetBatchInputsError::SelectBlockHeaderError)?;
+            .expect("partial mmr and block headers should be consistent");


What I meant here was to revert this change and use .map_err(GetBatchInputsError::SelectBlockHeaderError)?;

crates/store/src/state.rs

PhilippGackstatter

Thanks for addressing all the comments. Looks good to me!

@Mirko-von-Leipzig Ready for merging from my side.

CHANGELOG.md

crates/store/src/state.rs

Mirko-von-Leipzig · 2025-02-18T13:32:40Z

crates/store/src/state.rs

+        let latest = self.chain_tip();
+        latest + 1


I believe BlockNumber::child() exists.

Suggested change

let latest = self.chain_tip();

latest + 1

self.chain_tip().child()

Though I'm unsure if we should have both chain_tip and chain_length? I guarantee someone's going to confuse the two.

--edit-- in fact further down we have let chain_length = state.blockchain.chain_tip() as usize;

crates/store/src/state.rs

varun-doshi · 2025-02-18T14:02:32Z

@Mirko-von-Leipzig did not notice your commit there; apologies
I assume the only change was removing from the changelog?

Mirko-von-Leipzig · 2025-02-18T14:05:21Z

@Mirko-von-Leipzig did not notice your commit there; apologies I assume the only change was removing from the changelog?

That's my bad for inserting a commit without warning :) That was the only change, no harm done.

Mirko-von-Leipzig

Thanks for persevering @varun-doshi, I know its been a lengthy back-and-forth. Really appreciate it!

bobbinth · 2025-02-20T18:10:35Z

crates/store/src/state.rs

+    pub fn partial_mmr_from_blocks(
+        &self,
+        blocks: &BTreeSet<BlockNumber>,
+        latest_block_number: BlockNumber,
+    ) -> Result<PartialMmr, GetBatchInputsError> {


I know this is already merged, but I would have probably simplified this a bit:

pub fn partial_mmr_from_blocks( &self, blocks: &BTreeSet<BlockNumber>, ) -> Result<PartialMmr, GetBatchInputsError>

And the the latest_block_number could be just in the blocks (and we could get it via BTreeSet::last()). This would remove a potential inconsistency if latest_block_number would not actually be the highest value for blocks.

bobbinth · 2025-02-20T18:12:45Z

crates/store/src/state.rs

            blocks.remove(&latest_block_num);

-            // Using latest block as the target forest means we take the state of the MMR one before
-            // the latest block. This is because the latest block will be used as the reference
-            // block of the batch and will be added to the MMR by the batch kernel.
-            let target_forest = latest_block_num.as_usize();
-            let peaks = state
-                .chain_mmr
-                .peaks_at(target_forest)
-                .expect("target_forest should be smaller than forest of the chain mmr");
-            let mut partial_mmr = PartialMmr::from_peaks(peaks);
-
-            for block_num in blocks.iter().map(BlockNumber::as_usize) {
-                // SAFETY: We have ensured block nums are less than chain length.
-                let leaf = state
-                    .chain_mmr
-                    .get(block_num)
-                    .expect("block num less than chain length should exist in chain mmr");
-                let path = state
-                    .chain_mmr
-                    .open_at(block_num, target_forest)
-                    .expect("block num and target forest should be valid for this mmr")
-                    .merkle_path;
-                // SAFETY: We should be able to fill the partial MMR with data from the chain MMR
-                // without errors, otherwise it indicates the chain mmr is invalid.
-                partial_mmr
-                    .track(block_num, leaf, &path)
-                    .expect("filling partial mmr with data from mmr should succeed");
-            }
-
-            (latest_block_num, partial_mmr)
+            (
+                latest_block_num,
+                inner_state.blockchain.partial_mmr_from_blocks(&blocks, latest_block_num)?,
+            )


Related to the above comment, this could be:

let partial_mmr = inner_state.blockchain.partial_mmr_from_blocks(&blocks)?; blocks.remove(&latest_block_num); (latest_block_num, partial_mmr)

I can open a separate PR to get these changes in, if you'd like

That would be awesome! Thank you!

I did pretty much the simplification change from further above in #709 since I used that function there anyway and noticed we don't even need the Result on it, so no need for the PR I think, @varun-doshi, but thanks!

feat: added wrapper for Mmr

7eabe03

Mirko-von-Leipzig requested a review from PhilippGackstatter February 6, 2025 17:08

PhilippGackstatter reviewed Feb 10, 2025

View reviewed changes

igamigo reviewed Feb 10, 2025

View reviewed changes

crates/store/src/state.rs Outdated Show resolved Hide resolved

merge next into branch

f2a8da6

varun-doshi requested review from PhilippGackstatter and igamigo February 11, 2025 08:11

PhilippGackstatter reviewed Feb 14, 2025

View reviewed changes

fix: nits + suggestions

503e9c4

varun-doshi requested a review from PhilippGackstatter February 16, 2025 20:30

PhilippGackstatter reviewed Feb 17, 2025

View reviewed changes

varun-doshi requested a review from PhilippGackstatter February 18, 2025 09:51

PhilippGackstatter approved these changes Feb 18, 2025

View reviewed changes

Mirko-von-Leipzig reviewed Feb 18, 2025

View reviewed changes

Mirko-von-Leipzig added the no changelog label Feb 18, 2025

fix: wrong function names and nit

9c65a09

varun-doshi force-pushed the varun/mmr-wrapper branch from 6a59653 to 9c65a09 Compare February 18, 2025 14:00

varun-doshi requested a review from Mirko-von-Leipzig February 18, 2025 14:01

Mirko-von-Leipzig approved these changes Feb 18, 2025

View reviewed changes

Mirko-von-Leipzig merged commit d57c8dd into 0xPolygonMiden:next Feb 18, 2025
11 checks passed

Mirko-von-Leipzig mentioned this pull request Feb 19, 2025

fix(store): blocking in async fn get_batch_inputs #705

Merged

PhilippGackstatter mentioned this pull request Feb 20, 2025

[Task]: Wrap Mmr into a Blockchain struct #663

Closed

5 tasks

bobbinth reviewed Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added wrapper for Mmr #668

feat: added wrapper for Mmr #668

varun-doshi commented Feb 5, 2025

PhilippGackstatter left a comment

PhilippGackstatter Feb 10, 2025

PhilippGackstatter Feb 10, 2025

PhilippGackstatter Feb 10, 2025

varun-doshi Feb 11, 2025

PhilippGackstatter Feb 11, 2025

varun-doshi Feb 11, 2025

PhilippGackstatter Feb 10, 2025

PhilippGackstatter Feb 10, 2025

PhilippGackstatter left a comment

PhilippGackstatter Feb 14, 2025

PhilippGackstatter Feb 14, 2025

PhilippGackstatter Feb 14, 2025

PhilippGackstatter Feb 14, 2025

PhilippGackstatter Feb 14, 2025

PhilippGackstatter Feb 17, 2025

PhilippGackstatter Feb 14, 2025

PhilippGackstatter Feb 14, 2025

PhilippGackstatter Feb 14, 2025

PhilippGackstatter Feb 17, 2025

PhilippGackstatter Feb 14, 2025

PhilippGackstatter left a comment

PhilippGackstatter Feb 17, 2025

PhilippGackstatter Feb 17, 2025

PhilippGackstatter left a comment

Mirko-von-Leipzig Feb 18, 2025

varun-doshi commented Feb 18, 2025

Mirko-von-Leipzig commented Feb 18, 2025

Mirko-von-Leipzig left a comment

bobbinth Feb 20, 2025 •

edited

Loading

bobbinth Feb 20, 2025

varun-doshi Feb 20, 2025 •

edited

Loading

bobbinth Feb 20, 2025

PhilippGackstatter Feb 20, 2025 •

edited

Loading

		let chain_mmr =
		ChainMmr::new(partial_mmr, headers.clone()).expect("Unable to build ChainMmr");

		.map_err(GetBatchInputsError::SelectBlockHeaderError)?;
		.expect("partial mmr and block headers should be consistent");

	let latest = self.chain_tip();
	latest + 1
	self.chain_tip().child()

feat: added wrapper for Mmr #668

feat: added wrapper for Mmr #668

Conversation

varun-doshi commented Feb 5, 2025

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varun-doshi commented Feb 18, 2025

Mirko-von-Leipzig commented Feb 18, 2025

Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

bobbinth Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varun-doshi Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippGackstatter Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

bobbinth Feb 20, 2025 •

edited

Loading

varun-doshi Feb 20, 2025 •

edited

Loading

PhilippGackstatter Feb 20, 2025 •

edited

Loading