Restrict the number of stack inputs and outputs to 16 #1456

Fumuran · 2024-08-16T20:19:31Z

This PR changes the input and output stack depth to be constant of 16. As a result of this change StackOutputs now doesn't have the OverflowArray, since we don't allow the stack to have more than 16 elements.

TODO:

Create a new error which returns if the program have a non-empty OverflowTable at the end of the execution.
Update docs and comments.
Update CHANGELOG.

Fumuran · 2024-08-16T20:20:12Z

core/src/stack/mod.rs

 /// The number of stack registers which can be accessed by the VM directly. This is also the
 /// minimum stack depth enforced by the VM.
 pub const STACK_TOP_SIZE: usize = 16;
+
+/// Maximum number of elements allowed for the input and output stack.
+pub const STACK_DEPTH: usize = 16;


Probably we should leave only one constant.

Agreed; I would maybe have just STACK_MAX_DEPTH? And then document that

it's the maximum number of non-zero elements allowed on the stack at the beginning and end of the execution

Anything more than that goes in the overflow table

I think MAX_STACK_DEPTH may be a bit confusing because we also have the concept of stack depth defined as 16 + number of elements in the overflow table (e.g., accessible via sdepth instruction).

We could use MIN_STACK_DEPTH (as stack depth never drops below 16) - but I'd probably prefer to keep STACK_TOP_SIZE as I think MIN_STACK_DEPTH would be a bit confusing as well.

I agree that MAX_STACK_DEPTH isn't ideal, but I also don't understand the naming behind "stack top size". In my mind, the "stack top" is the element on the top of the stack (e.g. in C++). Then "stack top size" is the size of that element.

Maybe VISIBLE_STACK_DEPTH, where "visible" refers to the fact that it's directly in the trace? If not, I find MIN_STACK_DEPTH to be less confusing than STACK_TOP_SIZE

I tried to use "stack top" as a stand-alone concept that refers to the top 16 elements of the stack - but probably that's a bit confusing. Basically, would be good to have something like that because it is tied to a few things:

Only the top 16 elements can be initialize at the start of execution and remain populated at the end of execution.

Only the top 16 elements can be accessed directly via instructions.

Only the top 16 elements remain visible to the callee when we switch context via call or syscall instructions.

The depth of the stack never drops below 16 elements.

And saying "the top 16 elements of the stack" is a bit of a mouthful.

Given the options though - I probably would prefer MIN_STACK_DEPTH to VISIBLE_STACK_DEPTH.

But in that case I already done this: I left only one constant (MIN_STACK_DEPTH) and created a docstring with a description based on the Bobbin's comment. Sorry, probably I'm missing something.

We still have the constant STACK_TOP_SIZE here, right? My point is:

Rename STACK_TOP_SIZE to MIN_STACK_DEPTH

Add this docstring to it

Remove this constant

Ah, I see, there are two STACK_TOP_SIZE constants there! Yep, now I get it, I'll rework it as you said.

Edit: probably we shouldn't remove the MIN_STACK_DEPTH constant from the core, it is widely used across the repo, and sometimes we can't import the one from air without importing the air crate itself (for example, we can't import it to the core), so probably it will be cleaner to leave both of MIN_STACK_DEPTH constants.

Edit 2: or, if we really want to leave only one constant, probably it will be better to remove one from the air and just import there the constant from the core, it will be a lot better in terms of dependency conciseness.

Ah yes good point! I didn't even notice it was defined a few times (STACK_TOP_SIZE in core, in air and MIN_STACK_DEPTH).

As you said I would define it just once in core here (although renamed to MIN_STACK_DEPTH) and use that in all crates.

One thing to note, we still use the term "stack top" pretty consistently throughout the codebase to refer to the top 16 elements of the stack (e.g., here and here). I wonder to what extent we can replace these with StackInputs and StackOutputs structs. Though, updating the debug instruction should probably be in a separate PR

plafer

Looks good! We're missing a few things but all are minor

core/src/stack/inputs.rs

core/src/stack/outputs.rs

processor/src/stack/overflow.rs

processor/src/stack/mod.rs

prover/src/lib.rs

plafer · 2024-08-26T20:15:21Z

test-utils/Cargo.toml

@@ -32,6 +32,7 @@ processor = { package = "miden-processor", path = "../processor", version = "0.1
    "testing",
 ] }
 prover = { package = "miden-prover", path = "../prover", version = "0.10", default-features = false }
+stdlib = { package = "miden-stdlib", path = "../stdlib", version = "0.10", default-features = false }


It seems like test-utils depends on stdlib, and stdlib depends on test-utils. What am I missing? Why isn't this an error?

Why do we need the stdlib dependency here? I would prover not to introduce it (if we can avoid it).

I added the stdlib dependency to be able to use sys::truncate_stack in the build_op_test! macro. Without it we get the OutputStackOverflow error each time we push something to the stack. Potentially we can rework all tests which use build_op_test! so that we will need to drop excess values from the stack, but I thought that using trucnate_stack will be more clean.

I agree that adding the stdlib dependency is undesirable, but I'm not sure how can we solve this problem without it.

One way to solve this is to just manually copy the implementation of sys::truncate_stack into test_utils crate somewhere. Then, test program can be defined like:

{TRUNCATE_STACK_PROC} begin {} exec.truncate_stack end

If I get it right, the problem is that usage of the constant inside the macro require this constant to be available in the modules where this macro is used. So in order to create a program using the source code of the truncate_stack procedure I need to manually paste in inside the macro (which will be ugly, but will work)

plafer · 2024-08-26T20:25:59Z

core/src/stack/mod.rs

 /// The number of stack registers which can be accessed by the VM directly. This is also the
 /// minimum stack depth enforced by the VM.
 pub const STACK_TOP_SIZE: usize = 16;
+
+/// Maximum number of elements allowed for the input and output stack.
+pub const STACK_DEPTH: usize = 16;


Agreed; I would maybe have just STACK_MAX_DEPTH? And then document that

it's the maximum number of non-zero elements allowed on the stack at the beginning and end of the execution

Anything more than that goes in the overflow table

processor/src/stack/overflow.rs

plafer · 2024-08-26T20:32:19Z

air/src/constraints/stack/mod.rs

-/// Gets the initial value of the overflow table auxiliary column from the provided sets of initial
-/// values and random elements.
-fn get_overflow_table_init<E>(alphas: &[E], init_values: &[Felt]) -> E
+pub fn get_aux_assertions_first_step<E>(result: &mut Vec<Assertion<E>>)


We can also remove processor::stack::aux_trace::AuxTraceBuilder::init_responses() (the default implementation in AuxColumnBuilder is now sufficient).

I also think this means we can get rid of both fields in that AuxTraceBuilder.

nit: we can actually fully remove the definition, since it has an implementation in the trait directly

Ah, I see, we can just use the default implementation, indeed.

bobbinth

Looks good! Thank you! I also left some comments inline.

miden/tests/integration/main.rs

miden/tests/integration/operations/field_ops.rs

core/src/stack/inputs.rs

bobbinth · 2024-08-27T00:08:14Z

core/src/stack/mod.rs

 /// The number of stack registers which can be accessed by the VM directly. This is also the
 /// minimum stack depth enforced by the VM.
 pub const STACK_TOP_SIZE: usize = 16;
+
+/// Maximum number of elements allowed for the input and output stack.
+pub const STACK_DEPTH: usize = 16;


I think MAX_STACK_DEPTH may be a bit confusing because we also have the concept of stack depth defined as 16 + number of elements in the overflow table (e.g., accessible via sdepth instruction).

We could use MIN_STACK_DEPTH (as stack depth never drops below 16) - but I'd probably prefer to keep STACK_TOP_SIZE as I think MIN_STACK_DEPTH would be a bit confusing as well.

core/src/stack/outputs.rs

bobbinth · 2024-08-27T00:22:49Z

test-utils/Cargo.toml

@@ -32,6 +32,7 @@ processor = { package = "miden-processor", path = "../processor", version = "0.1
    "testing",
 ] }
 prover = { package = "miden-prover", path = "../prover", version = "0.10", default-features = false }
+stdlib = { package = "miden-stdlib", path = "../stdlib", version = "0.10", default-features = false }


Why do we need the stdlib dependency here? I would prover not to introduce it (if we can avoid it).

bobbinth

Thank you! Looks good! I left a few more comments inline.

I think the main thing is that we can probably treat StackInputs and StackOutputs as very thin wrappers around [Felt; MIN_STACK_DEPTH] and so, can probably simplify them a bit more by implementing Deref into [Felt; MIN_STACK_DEPTH] for them.

Another thing to consider is that for many functions which return [Felt; MIN_STACK_DEPTH] we can probably return StackOutputs struct instead.

core/src/stack/mod.rs

core/src/stack/inputs.rs

core/src/stack/outputs.rs

air/src/constraints/stack/mod.rs

bobbinth · 2024-09-07T08:55:57Z

test-utils/src/lib.rs

 pub fn stack_top_to_ints(values: &[u64]) -> Vec<u64> {
    let mut result: Vec<u64> = values.to_vec();
-    result.resize(STACK_TOP_SIZE, 0);
+    result.resize(MIN_STACK_DEPTH, 0);
    result
 }


I'm wondering if we need functions like this one and the one above now. Maybe we should just have StackOutputs::as_int_vec() and StackInputs::as_int_vec() or something like this.

We can get rid of the stack_to_ints function above (or, at least, use it for a "non-stack" inputs: for memory and hashes), but I think that this stack_top_to_ints function helps a lot: it guaranties that the final_stack array (provided to the test), which could be even empty, at the comparison time will have the MIN_STACK_DEPTH length.
As an alternative we can construct the StackOutputs from this array and then cast it to the vector of ints, but just a resize here should be much more efficient.

Update tests.

Fumuran · 2024-09-13T23:32:18Z

TODO:

Remove stdlib dependency from the test-utils

plafer

LGTM! (re: my previous review)

plafer · 2024-09-23T18:11:57Z

@bobbinth can we get a final review on this one?

bobbinth · 2024-09-23T19:00:17Z

One thing holding this up is that I think this will break the current block construction code in miden-node (because the code, if I remember correctly) relies on putting most inputs onto the stack. So, the question is whether we want to address 0xPolygonMiden/miden-node#78 before merging this one.

bobbinth

Looks good! Thank you. Not a full review, but I left some more comments inline.

core/src/stack/inputs.rs

core/src/stack/outputs.rs

bobbinth · 2024-09-30T07:54:48Z

core/src/stack/outputs.rs

    /// Returns mutable access to the stack outputs, to be used for testing or running examples.
-    /// TODO: this should be marked with #[cfg(test)] attribute, but that currently won't work with
-    /// the integration test handler util.
    pub fn stack_mut(&mut self) -> &mut [Felt] {
-        &mut self.stack
+        &mut self.elements
+    }


Should this not be behind the testing flag?

If we apply some test features (default test or feature = "testing") we can't use it anymore in the test-utils here.

Could we not import the dependency in test-utils with the testing feature enabled?

plafer · 2024-10-01T10:46:44Z

@Fumuran @bobbinth can we prioritize merging this? I am blocked on it so that I can move the stack overflow table to LogUp-GKR

This is currently broken and will be fixed in a subsequent PR

bobbinth

Looks good! Thank you! I left some more comments inline - but they are pretty minor.

test-utils/src/lib.rs

processor/src/stack/mod.rs

processor/src/operations/comb_ops.rs

processor/src/decoder/aux_trace/mod.rs

miden/tests/integration/operations/field_ops.rs

miden/tests/integration/operations/stack_ops.rs

processor/src/stack/tests.rs

bobbinth

Look good! Thank you! I think the main outstanding thing is to update the mdBook documentation - let's do this in a follow-up PR though.

Fumuran commented Aug 16, 2024

View reviewed changes

Fumuran force-pushed the andrew-limit-io-stack-to-16 branch 3 times, most recently from e62147e to fbf3df9 Compare August 21, 2024 03:35

Fumuran marked this pull request as ready for review August 21, 2024 03:37

Fumuran requested a review from bobbinth August 21, 2024 03:37

bobbinth requested a review from plafer August 23, 2024 23:14

Fumuran force-pushed the andrew-limit-io-stack-to-16 branch from fbf3df9 to 98ce230 Compare August 26, 2024 10:52

plafer requested changes Aug 26, 2024

View reviewed changes

bobbinth reviewed Aug 27, 2024

View reviewed changes

Fumuran force-pushed the andrew-limit-io-stack-to-16 branch from bb5fcc1 to 2c23e12 Compare August 29, 2024 14:54

Fumuran requested review from plafer and bobbinth August 29, 2024 20:47

bobbinth mentioned this pull request Sep 2, 2024

Dynamic kernel procedure invocation 0xPolygonMiden/miden-base#803

Merged

2 tasks

bobbinth requested changes Sep 7, 2024

View reviewed changes

Fumuran added 6 commits September 14, 2024 01:53

refactor: limit input and output stack to 16 elements

467b2fc

1/2 feat: implement new error to check stack overflow

c6ac62a

Update tests.

2/2 feat: implement new error to check stack overflow

096526c

chore: update comments to fix clippy errors

191fa31

refactor: code optimizations, renamings, minor improvements

dd9f54a

refactor: rename constant, impl deref for stacks

414a7c2

Fumuran force-pushed the andrew-limit-io-stack-to-16 branch from 2c23e12 to 414a7c2 Compare September 13, 2024 23:31

refactor: update test-utils: use local truncate_stack proc

5736aa2

Fumuran requested a review from bobbinth September 16, 2024 12:05

plafer approved these changes Sep 18, 2024

View reviewed changes

bobbinth reviewed Sep 30, 2024

View reviewed changes

refactor: fix bug in ser/de of inputs and outputs, add tests

4ea0ac0

Fumuran force-pushed the andrew-limit-io-stack-to-16 branch from 050c956 to 4ea0ac0 Compare October 1, 2024 16:01

plafer and others added 3 commits October 1, 2024 17:08

Merge branch 'next' into andrew-limit-io-stack-to-16

3331814

Remove p1 check of last row

4fe1f70

This is currently broken and will be fixed in a subsequent PR

refactor: fix overlooked tests

6c1cd08

bobbinth reviewed Oct 2, 2024

View reviewed changes

chore: update tests, improve docs

fde47d0

Fumuran force-pushed the andrew-limit-io-stack-to-16 branch from 91b25f7 to fde47d0 Compare October 2, 2024 16:11

Fumuran requested a review from bobbinth October 2, 2024 16:32

refactor: rename felt_array_to_ints into() felt_slice_to_ints()

a0e1076

bobbinth approved these changes Oct 3, 2024

View reviewed changes

bobbinth merged commit b2c5215 into next Oct 3, 2024
9 checks passed

bobbinth deleted the andrew-limit-io-stack-to-16 branch October 3, 2024 06:03

bobbinth mentioned this pull request Oct 3, 2024

Restrict the number of stack inputs and outputs to 16 #1428

Open

Restrict the number of stack inputs and outputs to 16 #1456

Restrict the number of stack inputs and outputs to 16 #1456

Conversation

Fumuran commented Aug 16, 2024 • edited by plafer Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fumuran Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

plafer Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plafer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fumuran commented Sep 13, 2024 • edited Loading

plafer left a comment

Choose a reason for hiding this comment

plafer commented Sep 23, 2024

bobbinth commented Sep 23, 2024

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plafer commented Oct 1, 2024

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Fumuran commented Aug 16, 2024 •

edited by plafer

Loading

Fumuran Aug 30, 2024 •

edited

Loading

plafer Sep 3, 2024 •

edited

Loading

Fumuran commented Sep 13, 2024 •

edited

Loading