Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 19, 2025
+2 −6
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions crates/store/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,11 +197,7 @@ struct InnerState {
impl InnerState {
/// Returns the latest block number.
fn latest_block_num(&self) -> BlockNumber {
let block_number: u32 = (self.blockchain.chain_tip().as_usize() - 1)
.try_into()
.expect("chain_mmr always has, at least, the genesis block");

block_number.into()
self.blockchain.chain_tip()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is also a bugfix.

These issues creep in due to the forest / chain_tip offset by one which I don't actually fully understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone could explain to me why forest is offset, and for example why https://github.com/0xPolygonMiden/miden-node/pull/705/files#diff-339991026f9c6728dbde8578e8d896f0c44e4234a63e25113190fd1701d343b5R803-R811 isn't simply the latest peaks? Its extremely confusing that things are offset.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PhilippGackstatter, @bobbinth some insight maybe? 😓

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the chain MMR also confused me for a long time and I still find it confusing, with the peaks_at and all.

Basically, if we're currently building block 5, then it references the previous block 4. And the state of the chain MMR at block 4, is all blocks up to, but excluding that block, so the blocks in range 0..=3. This is so because block 5 can't add itself to the chain MMR, because it can't compute its own block hash yet, because for that, the chain MMR root is required, so there's a circular dependency. So the chain MMR therefore "lags" behind by one additional block relative to the other data structures like account or nullifier tree.
So while building block 5 we're only adding block 4 to the chain MMR, and block 6 will add block 5, etc.

To take your example for block inputs: If we fetch block inputs for block 5, then we need the peaks at the latest block (block 4), where forest is also 4 (i.e. the chain is of length 4 and the latest block is 3). That's why we're using forest = latest_block_num.

Does that help?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That helps with half of the puzzle. I don't understand what forest actually means as an index. A forest in the colloquial sense is a collection of trees. Is forest: usize the i'th tree? I don't know what it means 😓

Quoting from the various bits of documentation in the inner Mmr:

    /// Returns the peaks of the MMR at the state specified by `forest`.
    ///
    /// # Errors
    /// Returns an error if the specified `forest` value is not valid for this MMR.
    pub fn peaks_at(&self, forest: usize) -> Result<MmrPeaks, MmrError> {

doesn't really help, okay let's checkout the definition:

pub struct Mmr {
    /// Refer to the `forest` method documentation for details of the semantics of this value.
    pub(super) forest: usize,

okay another layer, let's check out the method:

    /// Returns the MMR forest representation.
    ///
    /// The forest value has the following interpretations:
    /// - its value is the number of elements in the forest
    /// - bit count corresponds to the number of trees in the forest
    /// - each true bit position determines the depth of a tree in the forest
    pub const fn forest(&self) -> usize {

Does number of elements mean the number of leaves? If so, how does this now relate back to peaks_at(forest: usize) -- since that's multiple peaks aka multiple leaves?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we actually need to create a newtype for forest and explain how it works there better (there is an issue for this in miden-crypto). But overall:

  • The value of forest is the number of leaves in the MMR. In this case, it would be the number of blocks. So, for example, if a chain has 1 block, the value of the forest is 1, if the chain has 5 blocks, the value of the forest is 5.
  • Given the above, forest - 1 gives us the number of the last block in the chain (e.g., if there are 5 blocks in the chain, 4 is the number of the latest block).
  • The reason why this is called forest is because the bit representation of the value gives us all the information about the underlying trees in the MMR. So, for example, if the forest value is 5, the binary representation is 101. The number of 1s is the number of trees in the forest (so, in this case 2 trees), and the position of 1s specifies the size of the trees - i.e., there is 1 tree of depth 2 and 1 tree of depth 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think partially its also confusing because we're mixing metaphors; we have mountains and peaks, along with trees, forests and leaves.

Is it more along the lines of: give me the peaks of the mountain range as it was back when it contained N elements?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - good point! Some of these are equivalent (at least in my brain :) ):

  • peak == root
  • mountain = tree
  • range = forest

So, MMR with 2 peaks contains 2 trees :)

Is it more along the lines of: give me the peaks of the mountain range as it was back when it contained N elements?

Yes, that's exactly it. Assuming our chain has 5 blocks:

  • peaks_at(5) gives us the current peaks (i.e., the 2 peaks representing trees of depth 0 and 2).
  • peaks_at(4) gives us the peaks when the chain had 4 blocks (this would be a single peak for a tree of depth 2).
  • peaks_at(3) gives us the peaks when the chain had 3 blocks (this would be two peaks for trees of depth 0 and 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - good point! Some of these are equivalent (at least in my brain :) ):

peak == root
mountain = tree
range = forest
So, MMR with 2 peaks contains 2 trees :)

That's also my model; but also why I found it confusing to ask to ask the forest (aka MMR aka range) for the peaks at forest.

Alright, so we're getting the peaks to prove the next block. However the chain MMR lags behind by one (for circular reasons), and therefore we're off by one. Though somehow we already have an additional element in there (or otherwise we could just ask for latest).

Copy link

@PhilippGackstatter PhilippGackstatter Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though somehow we already have an additional element in there (or otherwise we could just ask for latest).

We could ask for the latest in the block inputs here, but I think its done that way for explicitness reasons and I would probably keep it that way. Just to express that we fetch the latest block header from the DB and then get the chain MMR for exactly that block. If we called peaks() we'd get the same, because we're holding on to the RwLockReadGuard, so nothing can change under our feet, but that's a bit too subtle here to change it I think.

A better comment explaining it would help though. I'll probably make changes to this function when updating the node to use the LocalBlockProver, then I can take a stab.

}
}

Expand Down Expand Up @@ -638,7 +634,7 @@ impl State {
// 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 inner_state = self.inner.read().await;

let latest_block_num = inner_state.blockchain.chain_tip();

Expand Down
Loading