fix(store): blocking in async fn get_batch_inputs

[Mirko-von-Leipzig]

Bug was introduced in #668.

We are missing store tests.

Fixes #703.

[PhilippGackstatter]

Looks good, thanks for the fix!

[Mirko-von-Leipzig]

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.

[Mirko-von-Leipzig]

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.

[Mirko-von-Leipzig]

@PhilippGackstatter, @bobbinth some insight maybe? 😓

[PhilippGackstatter]

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?

[Mirko-von-Leipzig]

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?

[bobbinth]

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.

[Mirko-von-Leipzig]

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?

[bobbinth]

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).

[Mirko-von-Leipzig]

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).

[PhilippGackstatter]

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.