feat: added BlockNumber struct #1043

varun-doshi · 2024-12-31T17:21:31Z

Ref #1040

varun-doshi · 2024-12-31T17:22:29Z

This is currently in init state. I will implement the functions as advised next

PhilippGackstatter

Thanks for the contribution!

Looks good generally. I have some nits and mainly two comments.

I think we can make more use of BlockNumber in many places, see inline comments.
It would be great if we can get rid of the standalone functions so they are only accessed through BlockNumber::* functions:

miden-base/objects/src/block/header.rs

Lines 281 to 289 in c98b1b7

    
           /// Returns the block number of the epoch block for the given `epoch`. 
        
           pub const fn block_num_from_epoch(epoch: u16) -> u32 { 
        
               (epoch as u32) << BlockHeader::EPOCH_LENGTH_EXPONENT 
        
           } 
        
           /// Returns the epoch of the given block number. 
        
           pub const fn block_epoch_from_number(block_number: u32) -> u16 { 
        
               (block_number >> BlockHeader::EPOCH_LENGTH_EXPONENT) as u16 
        
           }

Not sure whether we should keep BlockHeader::block_epoch. It can be easily replaced by block_header.block_num().epoch(), but it's also convenient. Both options are fine, I think.

objects/src/block/header.rs

PhilippGackstatter

Thanks for the updates, it looks great!

I mentioned a couple more places where we can replace u32s with BlockNumber.

To get this PR in a mergable state, there are some other small tasks:

Add an entry in CHANGELOG.md pointing to this PR, for example.

Add BlockNumber struct (feat: added BlockNumber struct #1043).
Run make format, make clippy and make doc locally to ensure everything is formatted correctly, that no lints complain and that docs can be built without errors.
You can also run cargo test -p miden-objects --features testing to run tests locally and make sure CI won't complain.
There is a small merge conflict that needs to be resolved.

Scroll to the bottom of this PR to see which checks in CI are failing.

Thanks again for your contributions!

objects/src/block/header.rs

objects/src/transaction/chain_mmr.rs

objects/src/accounts/account_id_anchor.rs

objects/src/notes/location.rs

…truct

PhilippGackstatter

Looks good to me! Thank you again for the contribution!

I pushed some minor commits, mostly to resolve the merge conflicts.

@bobbinth Do you want to take a look or should we merge it?

miden-lib/src/transaction/inputs.rs

objects/src/block/header.rs

PhilippGackstatter · 2025-01-14T09:03:30Z

@varun-doshi I only now notice that your commits are not signed. This repo requires that commits are signed, otherwise we cannot merge the PR. Can you sign them (this might be helpful: https://stackoverflow.com/questions/41882919/is-there-a-way-to-gpg-sign-all-previous-commits)? If you don't want to do that, I can also rebase the PR to include your commits in mine and sign them, though I think the PR would not count as your contribution then. Let me know how you want to proceed.

Mirko-von-Leipzig · 2025-01-14T10:49:48Z

miden-tx/src/testing/mock_chain/mod.rs

+                    note_block_num.as_u32(),
+                    self.blocks.get(note_block_num.as_usize()).unwrap().header(),


Should the maps not use BlockNumber as a key instead?

Mirko-von-Leipzig · 2025-01-14T10:51:01Z

miden-tx/src/testing/mock_chain/mod.rs

@@ -632,7 +632,7 @@ impl MockChain {
    /// This will also make all the objects currently pending available for use.
    /// If `block_num` is `Some(number)`, blocks will be generated up to `number`.
    pub fn seal_block(&mut self, block_num: Option<u32>) -> Block {
-        let next_block_num = self.blocks.last().map_or(0, |b| b.header().block_num() + 1);
+        let next_block_num = self.blocks.last().map_or(0, |b| b.header().block_num().as_u32() + 1);


I'd recommend adding a BlockNumber::next or similar method to increment by one.

Mirko-von-Leipzig · 2025-01-14T10:55:10Z

objects/src/block/header.rs

+#[derive(Debug, Eq, PartialEq, Copy, Clone, PartialOrd, Ord, Hash)]
+pub struct BlockNumber(u32);


Should we not derive Default, and possible add some utility functions like next/previous or child/parent?

I'd also appreciate addition and checked subtraction implementations.

Using next or previous results in this warning from clippy:

method `next` can be confused for the standard trait method `std::iter::Iterator::next`

hence going with parent and child

Mirko-von-Leipzig · 2025-01-14T11:01:33Z

objects/src/transaction/inputs.rs

@@ -44,9 +44,9 @@ impl TransactionInputs {

        // check the block_chain and block_header are consistent
        let block_num = block_header.block_num();
-        if block_chain.chain_length() != block_header.block_num() as usize {
+        if block_chain.chain_length() != block_header.block_num().as_usize() {


Should chain length not be a block number?

varun-doshi · 2025-01-14T12:10:55Z

@varun-doshi I only now notice that your commits are not signed. This repo requires that commits are signed, otherwise we cannot merge the PR. Can you sign them (this might be helpful: https://stackoverflow.com/questions/41882919/is-there-a-way-to-gpg-sign-all-previous-commits)? If you don't want to do that, I can also rebase the PR to include your commits in mine and sign them, though I think the PR would not count as your contribution then. Let me know how you want to proceed.

@PhilippGackstatter I'm rebasing and signing my commits, but looks like for some reason after the rebase, your signatures for the last 3 commits are gone(git log --show-signature). Is that okay? You can perhaps sign them again?

PhilippGackstatter · 2025-01-14T12:31:48Z

@PhilippGackstatter I'm rebasing and signing my commits, but looks like for some reason after the rebase, your signatures for the last 3 commits are gone(git log --show-signature). Is that okay? You can perhaps sign them again?

Thanks. I think you may have rebased onto some older branch as there are now a lot of unrelated changes in your PR and conflicts that shouldn't be there. Did you rebase onto 0xPolygonMiden/miden-base/next, i.e. the next branch form this repo and not your own repo?

@bobbinth told me he can override the commit sign requirement, so I think it's easiest to just do that. I can force push the old state of the branch again if that's fine with you.

varun-doshi · 2025-01-14T12:32:52Z

@PhilippGackstatter I'm rebasing and signing my commits, but looks like for some reason after the rebase, your signatures for the last 3 commits are gone(git log --show-signature). Is that okay? You can perhaps sign them again?

Thanks. I think you may have rebased onto some older branch as there are now a lot of unrelated changes in your PR and conflicts that shouldn't be there. Did you rebase onto 0xPolygonMiden/miden-base/next, i.e. the next branch form this repo and not your own repo?

@bobbinth told me he can override the commit sign requirement, so I think it's easiest to just do that. I can force push the old state of the branch again if that's fine with you.

I see
Yes thats fine with me
Apologies for the confusion

PhilippGackstatter · 2025-01-14T12:35:28Z

@varun-doshi No worries at all, we appreciate the contribution 😃!

varun-doshi · 2025-01-14T12:48:10Z

@PhilippGackstatter should i go ahead and make the changes suggested by @Mirko-von-Leipzig ?(in case you've not already started on those)

PhilippGackstatter · 2025-01-14T12:49:29Z

@PhilippGackstatter should i go ahead and make the changes suggested by @Mirko-von-Leipzig ?(in case you've not already started on those)

Sure, feel free to address those, thanks.

objects/src/block/header.rs

objects/src/errors.rs

objects/src/transaction/chain_mmr.rs

PhilippGackstatter

Looks good to me!
Just BlockNumber::parent should be changed (see inline comment).

PhilippGackstatter · 2025-01-16T06:51:09Z

objects/src/block/block_number.rs

+    pub fn parent(&self) -> u32 {
+        self.checked_sub(1).expect("Cannot go below Genesis block!").0
+    }


This should return Option<BlockNumber>. If you call BlockNumber::from(0_u32).parent() this would panic which is not what we want. The caller of parent() needs to deal with the possibility of calling this on block number 0.

bobbinth

Looks great! Thank you for working on this. I left a couple of optional comments inline.

objects/src/transaction/inputs.rs

objects/src/block/block_number.rs

Mirko-von-Leipzig · 2025-01-16T08:09:49Z

objects/src/block/block_number.rs

    /// Returns the previous block number
-    pub fn parent(&self) -> u32 {
-        self.checked_sub(1).expect("Cannot go below Genesis block!").0
+    pub fn parent(&self) -> Option<u32> {


These should probably return Option<BlockNumber>, same with fn child.

Should child actually return Option<BlockNumber>? It seems fine to return BlockNumber since a u32 overflow would in practice only occur decades from now.
For parent I think it makes sense because in practice one might actually call parent() on the genesis block.

Oh right I realise what I said was ambiguous. I agree, I meant that they should both return BlockNumber and not u32 values.

fn parent(&self) -> Option<BlockNumber> fn child(&self) -> BlockNumber

btw you should probably also take by value since these are trivially Copy types so no need for references.

bobbinth

All looks good! Thank you again for working on this!

bobbinth requested a review from PhilippGackstatter January 6, 2025 08:14

PhilippGackstatter reviewed Jan 7, 2025

View reviewed changes

varun-doshi force-pushed the varun/blocknumber-struct branch from 0b1e3ab to 0358d5d Compare January 8, 2025 17:35

varun-doshi requested a review from PhilippGackstatter January 8, 2025 17:36

PhilippGackstatter reviewed Jan 9, 2025

View reviewed changes

varun-doshi added 4 commits January 9, 2025 22:44

added BlockNumber type

dd82316

resolved comments

13a4d3d

resolved comments

8fa6ac3

Merge remote-tracking branch 'upstream/next' into varun/blocknumber-s…

e43148c

…truct

varun-doshi force-pushed the varun/blocknumber-struct branch from 0358d5d to e43148c Compare January 10, 2025 18:57

varun-doshi requested a review from PhilippGackstatter January 11, 2025 05:50

PhilippGackstatter added 4 commits January 13, 2025 10:20

chore: Use BlockNumber in ChainMmrError

b3bfc33

chore: Convert some more u32s to BlockNumber

a46a5de

Merge remote-tracking branch 'origin/next' into varun/blocknumber-struct

fc48333

Merge remote-tracking branch 'origin/next' into varun/blocknumber-struct

f99d14a

PhilippGackstatter approved these changes Jan 13, 2025

View reviewed changes

bobbinth reviewed Jan 13, 2025

View reviewed changes

miden-lib/src/transaction/inputs.rs Outdated Show resolved Hide resolved

bobbinth reviewed Jan 13, 2025

View reviewed changes

objects/src/block/header.rs Outdated Show resolved Hide resolved

feat: Implement Felt, u64 and usize conversion methods

3fcd0fc

PhilippGackstatter mentioned this pull request Jan 14, 2025

refactor: standardize errors 0xPolygonMiden/miden-node#611

Merged

Mirko-von-Leipzig reviewed Jan 14, 2025

View reviewed changes

varun-doshi force-pushed the varun/blocknumber-struct branch from 3fcd0fc to 5c1c71d Compare January 14, 2025 12:25

PhilippGackstatter force-pushed the varun/blocknumber-struct branch from 5c1c71d to 3fcd0fc Compare January 14, 2025 12:34

bobbinth reviewed Jan 14, 2025

View reviewed changes

objects/src/block/header.rs Outdated Show resolved Hide resolved