Add block_num as a public input to the transaction kernel

[sergerad]

Closes #1119

Adds block number as a public input to the transaction kernel and validate it is consistent with the commitment block number.

[sergerad]

This needs to be tested somehow. LMK best way to do this

[PhilippGackstatter]

To test the memory layout generally, you can extend the global_input_memory_assertions function in crates/miden-tx/src/tests/kernel_tests/test_prologue.rs.

This procedure specifically will be tested implicitly by testing procedures that call it, so no extra test needed.

[sergerad]

Unsure if the intention was to store this felt in the same memory slot as the account id but that is my interpretation of the instructions.

[PhilippGackstatter]

Good question!

I think I would align the memory layout with the stack layout, so:

• The BLOCK_HASH word (4 elements) still goes into BLK_HASH_PTR from 400..=403 (i.e. nothing to do here).
• The block_num (1 element) goes into BLK_NUM_PTR at 404.
• We don't do anything with the 0 input so we leave offset 405 unoccupied.
• The account_id_suffix (1 element) goes into ACCT_ID_PTR at 406.
• The account_id_prefix (1 element) goes into ACCT_ID_PTR + 1 at 407.
  • On second thoughts, it might be best to introduce ACCT_ID_PREFIX_PTR and ACCT_ID_SUFFIX_PTR for these.

So I would add a set_blk_num procedure in memory.masm to set just the block number which is called from the prologue.
set_global_acct_id and get_global_acct_id must be modified to not use mem_storew/mem_loadw but rather two mem_store/mem_load operations on the individual felts.

Maybe @bobbinth has a different opinion, but that's how I would go about it.

[bobbinth]

There is another way to do, which could potentially require fewer changes. Specifically:

1. We modify process_global_inputs to return block_num. That is, process_global_inputs won't write it to memory but won't consume it either.
2. Then, block_num becomes an input into process_block_data procedure (i.e., the inputs will be Operand stack: [block_num]).
3. Then, at the end of process_block_data we read the block_num that was written to memory and make sure it is the same as the one on the stack.

If we do this, no other MASM code needs to change, I think.

[sergerad]

@bobbinth is the latest commit along the lines you were thinking?

So process_global_inputs looks like this:

#! Inputs:  [BLOCK_HASH, INITIAL_ACCOUNT_HASH, INPUT_NOTES_COMMITMENT, account_id_prefix, account_id_suffix, block_num]
#! Outputs: [block_num]

The test_transaction_prologue passes with those changes so should be functional.

P.S. I'm out of context w.r.t the block_num being written to elsewhere and why passing it in as a global input and comparing that against the one in memory is worthwhile.

[bobbinth]

Yes, that's how I was thinking about it - thank you!

The overall context is: we want to include block_num as a field in the ProvenTransaction struct (having it there simplifies things during batch building). But if we just add this to the struct, it opens up a possibility of internal inconsistency (e.g., block_num specified in the field may different from the block number associated with the block_hash). So, we want to make sure that the ZKP attached to the proven transaction also verifies that block_num and block_hash are consistent with each other.

So, after this change, once we verify the ZKP, we can be sure that block_num is in fact the number of the block with the specified block_hash.

[PhilippGackstatter]

Thanks for taking this on!

I left some comments inline hopefully answering your questions.

We should also add a check in prologue::process_block_data to ensure the global block number from the public inputs matches the one from the advice provider. This could happen after the squeeze_digest operation, for example.
This should just be a comparison between memory::get_blk_num and memory::get_global_blk_num and adding a new error for the assert statement.

[PhilippGackstatter]

Good question!

I think I would align the memory layout with the stack layout, so:

• The BLOCK_HASH word (4 elements) still goes into BLK_HASH_PTR from 400..=403 (i.e. nothing to do here).
• The block_num (1 element) goes into BLK_NUM_PTR at 404.
• We don't do anything with the 0 input so we leave offset 405 unoccupied.
• The account_id_suffix (1 element) goes into ACCT_ID_PTR at 406.
• The account_id_prefix (1 element) goes into ACCT_ID_PTR + 1 at 407.
  • On second thoughts, it might be best to introduce ACCT_ID_PREFIX_PTR and ACCT_ID_SUFFIX_PTR for these.

So I would add a set_blk_num procedure in memory.masm to set just the block number which is called from the prologue.
set_global_acct_id and get_global_acct_id must be modified to not use mem_storew/mem_loadw but rather two mem_store/mem_load operations on the individual felts.

Maybe @bobbinth has a different opinion, but that's how I would go about it.

[PhilippGackstatter]

To test the memory layout generally, you can extend the global_input_memory_assertions function in crates/miden-tx/src/tests/kernel_tests/test_prologue.rs.

This procedure specifically will be tested implicitly by testing procedures that call it, so no extra test needed.

[PhilippGackstatter]

We should ideally call this get_global_blk_num (global to indicate it's the one from the public inputs and blk for consistency, although I would personally also like us to use "block", but that's another discussion).

[PhilippGackstatter]

Did not get around to a full review, but left some small comments/questions.

[bobbinth]

Not from this PR, but I noticed that some procedures in the memory module can be optimized. For example:

export.get_blk_version
    padw push.BLOCK_METADATA_PTR mem_loadw drop drop swap drop
end

Can be written as:

export.get_blk_version
    push.BLOCK_METADATA_PTR add.1 mem_load
end

We can do this now because after we migrated to element-addressable memory, we can read individual elements from memory rather than having to read full words and then extract the element we are interested in. cc @Fumuran

[Fumuran]

Yep, I think a lot of memory procedures could be optimized this way. I created a corresponding issue #1132.

[bobbinth]

All looks good! Thank you!