-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restrict the number of stack inputs and outputs to 16 #1456
Conversation
core/src/stack/mod.rs
Outdated
/// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we should leave only one constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
orsyscall
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have the constant STACK_TOP_SIZE
here, right? My point is:
- Rename
STACK_TOP_SIZE
toMIN_STACK_DEPTH
- Add this docstring to it
- Remove this constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
e62147e
to
fbf3df9
Compare
fbf3df9
to
98ce230
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! We're missing a few things but all are minor
test-utils/Cargo.toml
Outdated
@@ -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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like test-utils
depends on stdlib
, and stdlib
depends on test-utils
. What am I missing? Why isn't this an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the stdlib
dependency here? I would prover not to introduce it (if we can avoid it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
core/src/stack/mod.rs
Outdated
/// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
/// 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>>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can actually fully remove the definition, since it has an implementation in the trait directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, we can just use the default implementation, indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I also left some comments inline.
core/src/stack/mod.rs
Outdated
/// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
test-utils/Cargo.toml
Outdated
@@ -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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the stdlib
dependency here? I would prover not to introduce it (if we can avoid it).
bb5fcc1
to
2c23e12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
test-utils/src/lib.rs
Outdated
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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
2c23e12
to
414a7c2
Compare
TODO:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! (re: my previous review)
@bobbinth can we get a final review on this one? |
One thing holding this up is that I think this will break the current block construction code in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you. Not a full review, but I left some more comments inline.
/// 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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not be behind the testing
flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we apply some test features (default test
or feature = "testing"
) we can't use it anymore in the test-utils
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we not import the dependency in test-utils
with the testing
feature enabled?
050c956
to
4ea0ac0
Compare
This is currently broken and will be fixed in a subsequent PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left some more comments inline - but they are pretty minor.
91b25f7
to
fde47d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Closes #1428
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 theOverflowArray
, since we don't allow the stack to have more than 16 elements.TODO:
OverflowTable
at the end of the execution.