StackOutputs: introduce pop_*() methods

[plafer]

This PR introduces methods to pop common types off StackOutputs.

The main benefits are:

1. We encode the logic that elements of a word are stored in reverse order in pop_digest()
2. Allows other crates to extend StackOutputs to pop their own types

For example, in miden-base, we should add:

pub trait StackOutputsPopHeader {
  fn pop_header(&mut self) -> Option<BlockHeader>;
}

impl StackOutputsPopHeader for StackOutputs {
  fn pop(&mut self) -> Option<BlockHeader> {
    let prev_hash = self.pop_felt();
    // ...
  }
}


// snippet
let block_header: BlockHeader = stack_outputs.pop_header()?;

[frisitano]

I agree with the general sentiment of making it easier to access data from StackOutputs but I'm not sure about allowing mutability of the StackOutputs object. I would favour introducing public accessors - something similar to what we have for the Process stack:

miden-vm/processor/src/stack/mod.rs

Lines 155 to 179 in c097af7

	/// Returns the value located at the specified position on the stack at the current clock cycle.
	pub fn get(&self, pos: usize) -> Felt {
	debug_assert!(pos < STACK_TOP_SIZE, "stack underflow");
	self.trace.get_stack_value_at(self.clk, pos)
	}
	/// Returns a word located at the specified word index on the stack.
	///
	/// Specifically, word 0 is defined by the first 4 elements of the stack, word 1 is defined
	/// by the next 4 elements etc. Since the top of the stack contains 4 word, the highest valid
	/// word index is 3.
	///
	/// The words are created in reverse order. For example, for word 0 the top element of the
	/// stack will be at the last position in the word.
	///
	/// Creating a word does not change the state of the stack.
	pub fn get_word(&self, word_idx: usize) -> Word {
	let offset = word_idx * WORD_SIZE;
	[
	self.get(offset + 3),
	self.get(offset + 2),
	self.get(offset + 1),
	self.get(offset),
	]
	}

[plafer]

@frisitano I added an alternative iterator-based implementation that doesn't require mutating StackOutputs. Use of the new API looks like:

let stack_outputs = StackOutputs::default();
let mut iter = stack_outputs.iter();

let root_1 = iter.next_digest()?;
let root_2 = iter.next_digest()?;

In miden-base, for example, we would also add:

pub trait StackOutputsIteratorHeader {
    fn next_header(&mut self) -> Option<BlockHeader>;
}

impl<I> StackOutputsIteratorHeader for I
where
  I: StackOutputsIteratorDigest
{
  fn next_header(&mut self) -> Option<BlockHeader> {
    // ...
    let account_root: Digest = iter.next_digest()?;
    // ...
    let block_num: Felt = iter.next()?;
    // ...
  }
}

Converted PR to draft since one of the two alternatives would need to be removed.

[frisitano]

I think I still favour the public accessor methods that I referenced above over the iterator method. The reason I think this would be preferable is because we can have some centrally defined constants that define how to interpret the StackOutputs - e.g. what we have here in miden-lib. If there is a change to these constants then we can just update their definition and all consumers should be updated automatically. If we use an iterator based impl then all consumers of the iterator will have to be updated independently.

Additionally I don't think it should be the responsibility of StackOutputs to interpret the type of the returned data (e.g. RpoDigest in this PR). StackOutputs should only return a Felt or a Word (native VM types) and then it is the responsibility of the caller to convert accordingly.

[plafer]

The main issue I'm trying to solve is here in the block builder's component of the block producer.

The problems I see:

1. The caller must know the order in which Felts are stored on the stack to form a Word (i.e. need to reverse). This ordering is a VM concern, as it is implicitly defined by MASM instructions such as mtree_set, and ideally should be hidden from the caller.
2. The current minimalist StackOutputs API incurs a lot of boilerplate on the caller

This PR solves these problems by making the StackOutputs more like a higher-level "Outputs" object, where the low-level detail that the VM writes its outputs as Felts on a stack are abstracted away slightly. In other words, as a user of the VM, I think of my VM program as taking n hashes and m numbers, and outputting x hashes and y numbers; not inputting 4*n + m Felts and outputting 4*x + y Felts.

Additionally I don't think it should be the responsibility of StackOutputs to interpret the type of the returned data (e.g. RpoDigest in this PR). StackOutputs should only return a Felt or a Word (native VM types) and then it is the responsibility of the caller to convert accordingly.

That's fair; I agree that I should have made the method next_word() instead of next_digest(). However, note that the current StackOutputs only outputs Felts at the moment.

If there is a change to these constants then we can just update their definition and all consumers should be updated automatically.

This is a good point.

If I understand your concerns correctly, I believe the following API would solve both our problems:

impl StackOutputs {
  pub get_felt_at(&self, idx: usize) -> Option<Felt> { ... }
  pub get_word_at(&self, idx: usize) -> Option<Word> { ... }
}

// caller
let tx_script_root = stack_outputs.get_word_at(TX_SCRIPT_ROOT_WORD_IDX)?;
let created_notes_commitment = stack_outputs.get_word_at(CREATED_NOTES_COMMITMENT_WORD_IDX)?;
let final_account_hash = stack_outputs.get_word_at(FINAL_ACCOUNT_HASH_WORD_IDX)?;

An adjustment I'd make to these constants is I would have them be indices in "Felt space" instead of "Word space". That is, their values are currently 0, 1 and 2; I would make them 0, 4, and 8 instead. This would make word indices consistent with felt indices. It would also allow for words to be stored at non-word boundaries (e.g. at index 1).

What do you think?

[frisitano]

The main issue I'm trying to solve is here in the block builder's component of the block producer.

Of relevance to your work here I would point you to the TryFromVmResult impl's here. Maybe they will be of use to you?

The problems I see:

1. The caller must know the order in which Felts are stored on the stack to form a Word (i.e. need to reverse). This ordering is a VM concern, as it is implicitly defined by MASM instructions such as mtree_set, and ideally should be hidden from the caller.
2. The current minimalist StackOutputs API incurs a lot of boilerplate on the caller

I agree with the sentiment here.

If I understand your concerns correctly, I believe the following API would solve both our problems:

impl StackOutputs {
  pub get_felt_at(&self, idx: usize) -> Option<Felt> { ... }
  pub get_word_at(&self, idx: usize) -> Option<Word> { ... }
}

// caller
let tx_script_root = stack_outputs.get_word_at(TX_SCRIPT_ROOT_WORD_IDX)?;
let created_notes_commitment = stack_outputs.get_word_at(CREATED_NOTES_COMMITMENT_WORD_IDX)?;
let final_account_hash = stack_outputs.get_word_at(FINAL_ACCOUNT_HASH_WORD_IDX)?;

Yes this API is what I had in mind - somewhat similar to the Process stack I referenced above (which also does the reverse ordering for Words).

An adjustment I'd make to these constants is I would have them be indices in "Felt space" instead of "Word space". That is, their values are currently 0, 1 and 2; I would make them 0, 4, and 8 instead. This would make word indices consistent with felt indices. It would also allow for words to be stored at non-word boundaries (e.g. at index 1).

What do you think?

I think this is a good idea, it makes the API more flexible! I wonder why this wasn't done for the Process stack. Maybe we should create an issue to discuss changing the Process stack get word api / operations to also use Felt space?

[plafer]

Of relevance to your work here I would point you to the TryFromVmResult impl's here. Maybe they will be of use to you?

Oh yes, definitely. This would replace the StackOutputsIteratorHeader idea here.

I think this is a good idea, it makes the API more flexible! I wonder why this wasn't done for the Process stack. Maybe we should create an issue to discuss changing the Process stack get word api / operations to also use Felt space?

Opened #1153

I will implement an API along the lines of what was described here instead of what's currently in this PR.

[plafer]

Closing in favor of #1155