Skip to content
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

feat: DYN takes a memory address instead of digest on stack #1535

Draft
wants to merge 2 commits into
base: next
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
- Debug instructions can be enabled in the cli `run` command using `--debug` flag (#1502)
- [BREAKING] ExecutionOptions::new constructor requires a boolean to explicitly set debug mode (#1502)
- [BREAKING] The `run` and the `prove` commands in the cli will accept `--trace` flag instead of `--tracing` (#1502)
- [BREAKING] `DYN` operation now expects a memory address pointing to the procedure hash (#1535)
- [BREAKING] `DYNCALL` operation fixed, and now expects a memory address pointing to the procedure hash (#1535)


#### Fixes
Expand Down
6 changes: 3 additions & 3 deletions air/src/constraints/stack/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,8 @@ trait EvaluationFrameExt<E: FieldElement> {
fn user_op_helper(&self, index: usize) -> E;

/// Returns the value if the `h6` helper register in the decoder which is set to ONE if the
/// ending block is a `CALL` block.
fn is_call_end(&self) -> E;
/// ending block is a `CALL` or `DYNCALL` block.
fn is_call_or_dyncall_end(&self) -> E;

/// Returns the value if the `h7` helper register in the decoder which is set to ONE if the
/// ending block is a `SYSCALL` block.
Expand Down Expand Up @@ -359,7 +359,7 @@ impl<E: FieldElement> EvaluationFrameExt<E> for &EvaluationFrame<E> {
}

#[inline]
fn is_call_end(&self) -> E {
fn is_call_or_dyncall_end(&self) -> E {
self.current()[DECODER_TRACE_OFFSET + IS_CALL_FLAG_COL_IDX]
}

Expand Down
27 changes: 23 additions & 4 deletions air/src/constraints/stack/op_flags/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,9 @@ impl<E: FieldElement> OpFlags<E> {
+ degree7_op_flags[47]
+ degree7_op_flags[46]
+ split_loop_flag
+ shift_left_on_end;
+ shift_left_on_end
+ degree5_op_flags[8] // DYN
+ degree5_op_flags[12]; // DYNCALL

left_shift_flags[2] = left_shift_flags[1] + left_change_1_flag;
left_shift_flags[3] =
Expand Down Expand Up @@ -398,9 +400,15 @@ impl<E: FieldElement> OpFlags<E> {
// Flag if the stack has been shifted to the right.
let right_shift = f011 + degree5_op_flags[11] + degree6_op_flags[4]; // PUSH; U32SPLIT

// Flag if the stack has been shifted to the left.
let left_shift =
f010 + add3_madd_flag + split_loop_flag + degree4_op_flags[5] + shift_left_on_end;
// Flag if the stack has been shifted to the left. Note that `DYNCALL` is not included in
// this flag even if it shifts the stack to the left. See `Opflags::left_shift()` for more
// information.
let left_shift = f010
+ add3_madd_flag
+ split_loop_flag
+ degree4_op_flags[5]
+ shift_left_on_end
+ degree5_op_flags[8]; // DYN

// Flag if the current operation being executed is a control flow operation.
// first row: SPAN, JOIN, SPLIT, LOOP
Expand Down Expand Up @@ -923,6 +931,12 @@ impl<E: FieldElement> OpFlags<E> {
self.degree4_op_flags[get_op_index(Operation::SysCall.op_code())]
}

/// Operation Flag of DYNCALL operation.
#[inline(always)]
pub fn dyncall(&self) -> E {
self.degree5_op_flags[get_op_index(Operation::Dyncall.op_code())]
}

/// Operation Flag of END operation.
#[inline(always)]
pub fn end(&self) -> E {
Expand Down Expand Up @@ -982,6 +996,11 @@ impl<E: FieldElement> OpFlags<E> {
}

/// Returns the flag when the stack operation shifts the flag to the left.
///
/// Note that although `DYNCALL` shifts the entire stack, it is not included in this flag. This
/// is because this "aggregate left shift" flag is used in constraints related to the stack
/// helper columns, and `DYNCALL` uses them unconventionally.
///
/// Degree: 5
#[inline(always)]
pub fn left_shift(&self) -> E {
Expand Down
13 changes: 8 additions & 5 deletions air/src/constraints/stack/overflow/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ pub fn enforce_constraints<E: FieldElement>(
/// - If the operation is a left shift op, then, depth should be decreased by 1 provided the
/// existing depth of the stack is not 16. In the case of depth being 16, depth will not be
/// updated.
/// - If the current op being executed is `CALL` or `SYSCALL`, then the depth should be reset to 16.
/// - If the current op being executed is `CALL`, `SYSCALL` or `DYNCALL`, then the depth should be
/// reset to 16.
///
/// TODO: This skips the operation when `END` is exiting for a `CALL` or a `SYSCALL` block. It
/// should be handled later in multiset constraints.
Expand All @@ -77,13 +78,15 @@ pub fn enforce_stack_depth_constraints<E: FieldElement>(
let depth = frame.stack_depth();
let depth_next = frame.stack_depth_next();

let call_or_syscall = op_flag.call() + op_flag.syscall();
let call_or_syscall_end = op_flag.end() * (frame.is_call_end() + frame.is_syscall_end());
let call_or_dyncall_or_syscall = op_flag.call() + op_flag.dyncall() + op_flag.syscall();
let call_or_dyncall_or_syscall_end =
op_flag.end() * (frame.is_call_or_dyncall_end() + frame.is_syscall_end());

let no_shift_part = (depth_next - depth) * (E::ONE - call_or_syscall - call_or_syscall_end);
let no_shift_part = (depth_next - depth)
* (E::ONE - call_or_dyncall_or_syscall - call_or_dyncall_or_syscall_end);
let left_shift_part = op_flag.left_shift() * op_flag.overflow();
let right_shift_part = op_flag.right_shift();
let call_part = call_or_syscall * (depth_next - E::from(16u32));
let call_part = call_or_dyncall_or_syscall * (depth_next - E::from(16u32));

// Enforces constraints of the transition of depth of the stack.
result[0] = no_shift_part + left_shift_part - right_shift_part + call_part;
Expand Down
2 changes: 1 addition & 1 deletion air/src/trace/decoder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ pub const IS_LOOP_BODY_FLAG_COL_IDX: usize = HASHER_STATE_RANGE.start + 4;
/// Index of a flag column which indicates whether an ending block is a LOOP block.
pub const IS_LOOP_FLAG_COL_IDX: usize = HASHER_STATE_RANGE.start + 5;

/// Index of a flag column which indicates whether an ending block is a CALL block.
/// Index of a flag column which indicates whether an ending block is a CALL or DYNCALL block.
pub const IS_CALL_FLAG_COL_IDX: usize = HASHER_STATE_RANGE.start + 6;

/// Index of a flag column which indicates whether an ending block is a SYSCALL block.
Expand Down
4 changes: 3 additions & 1 deletion air/src/trace/main_trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ impl MainTrace {

/// Returns a specific element from the hasher state at row i.
pub fn decoder_hasher_state_element(&self, element: usize, i: RowIndex) -> Felt {
self.columns.get_column(DECODER_TRACE_OFFSET + HASHER_STATE_OFFSET + element)[i + 1]
self.columns.get_column(DECODER_TRACE_OFFSET + HASHER_STATE_OFFSET + element)[i]
}
Comment on lines 139 to 141
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this a bug previously?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but for some reason it never manifested itself. I think only RESPAN used it, so maybe looking in the next row there was always what we expected? I'm really not sure why it went unnoticed for this long.


/// Returns the current function hash (i.e., root) at row i.
Expand Down Expand Up @@ -240,6 +240,8 @@ impl MainTrace {
([e0, b3, b2, b1] == [ONE, ZERO, ONE, ZERO]) ||
// REPEAT
([b6, b5, b4, b3, b2, b1, b0] == [ONE, ONE, ONE, ZERO, ONE, ZERO, ZERO]) ||
// DYN
([b6, b5, b4, b3, b2, b1, b0] == [ONE, ZERO, ONE, ONE, ZERO, ZERO, ZERO]) ||
// END of a loop
([b6, b5, b4, b3, b2, b1, b0] == [ONE, ONE, ONE, ZERO, ZERO, ZERO, ZERO] && h5 == ONE)
}
Expand Down
7 changes: 2 additions & 5 deletions assembly/src/assembler/instruction/procedures.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,12 @@ impl Assembler {
Ok(Some(dyn_node_id))
}

/// Creates a new CALL block whose target is DYN.
/// Creates a new DYNCALL block for the dynamic function call and return.
pub(super) fn dyncall(
&self,
mast_forest_builder: &mut MastForestBuilder,
) -> Result<Option<MastNodeId>, AssemblyError> {
let dyn_call_node_id = {
let dyn_node_id = mast_forest_builder.ensure_dyn()?;
mast_forest_builder.ensure_call(dyn_node_id)?
};
let dyn_call_node_id = mast_forest_builder.ensure_dyncall()?;

Ok(Some(dyn_call_node_id))
}
Expand Down
5 changes: 5 additions & 0 deletions assembly/src/assembler/mast_forest_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,11 @@ impl MastForestBuilder {
self.ensure_node(MastNode::new_dyn())
}

/// Adds a dyncall node to the forest, and returns the [`MastNodeId`] associated with it.
pub fn ensure_dyncall(&mut self) -> Result<MastNodeId, AssemblyError> {
self.ensure_node(MastNode::new_dyncall())
}

/// Adds an external node to the forest, and returns the [`MastNodeId`] associated with it.
pub fn ensure_external(&mut self, mast_root: RpoDigest) -> Result<MastNodeId, AssemblyError> {
self.ensure_node(MastNode::new_external(mast_root))
Expand Down
2 changes: 1 addition & 1 deletion assembly/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1898,7 +1898,7 @@ fn program_with_dynamic_code_execution_in_new_context() -> TestResult {
let program = context.assemble(source)?;
let expected = "\
begin
call.0xc75c340ec6a69e708457544d38783abbb604d881b7dc62d00bfc2b10f52808e6
dyncall
end";
assert_str_eq!(format!("{program}"), expected);
Ok(())
Expand Down
5 changes: 5 additions & 0 deletions core/src/mast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,11 @@ impl MastForest {
self.add_node(MastNode::new_dyn())
}

/// Adds a dyncall node to the forest, and returns the [`MastNodeId`] associated with it.
pub fn add_dyncall(&mut self) -> Result<MastNodeId, MastForestError> {
self.add_node(MastNode::new_dyncall())
}

/// Adds an external node to the forest, and returns the [`MastNodeId`] associated with it.
pub fn add_external(&mut self, mast_root: RpoDigest) -> Result<MastNodeId, MastForestError> {
self.add_node(MastNode::new_external(mast_root))
Expand Down
89 changes: 75 additions & 14 deletions core/src/mast/node/dyn_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,44 +6,90 @@ use miden_formatting::prettier::{const_text, nl, Document, PrettyPrint};

use crate::{
mast::{DecoratorId, MastForest},
OPCODE_DYN,
OPCODE_DYN, OPCODE_DYNCALL,
};

// DYN NODE
// ================================================================================================

/// A Dyn node specifies that the node to be executed next is defined dynamically via the stack.
#[derive(Debug, Clone, Default, PartialEq, Eq)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct DynNode {
is_dyncall: bool,
before_enter: Vec<DecoratorId>,
after_exit: Vec<DecoratorId>,
}

/// Constants
impl DynNode {
/// The domain of the Dyn block (used for control block hashing).
pub const DOMAIN: Felt = Felt::new(OPCODE_DYN as u64);
pub const DYN_DOMAIN: Felt = Felt::new(OPCODE_DYN as u64);

/// The domain of the Dyncall block (used for control block hashing).
pub const DYNCALL_DOMAIN: Felt = Felt::new(OPCODE_DYNCALL as u64);
}

/// Public accessors
impl DynNode {
/// Creates a new [`DynNode`] representing a dynexec operation.
pub fn new_dyn() -> Self {
Self {
is_dyncall: false,
before_enter: Vec::new(),
after_exit: Vec::new(),
}
}

/// Creates a new [`DynNode`] representing a dyncall operation.
pub fn new_dyncall() -> Self {
Self {
is_dyncall: true,
before_enter: Vec::new(),
after_exit: Vec::new(),
}
}

/// Returns true if the [`DynNode`] represents a dyncall operation, and false for dynexec.
pub fn is_dyncall(&self) -> bool {
self.is_dyncall
}

/// Returns the domain of this dyn node.
pub fn domain(&self) -> Felt {
if self.is_dyncall() {
Self::DYNCALL_DOMAIN
} else {
Self::DYN_DOMAIN
}
}

/// Returns a commitment to a Dyn node.
///
/// The commitment is computed by hashing two empty words ([ZERO; 4]) in the domain defined
/// by [Self::DOMAIN], i.e.:
/// by [Self::DYN_DOMAIN] or [Self::DYNCALL_DOMAIN], i.e.:
///
/// ```
/// # use miden_core::mast::DynNode;
/// # use miden_crypto::{hash::rpo::{RpoDigest as Digest, Rpo256 as Hasher}};
/// Hasher::merge_in_domain(&[Digest::default(), Digest::default()], DynNode::DOMAIN);
/// Hasher::merge_in_domain(&[Digest::default(), Digest::default()], DynNode::DYN_DOMAIN);
/// Hasher::merge_in_domain(&[Digest::default(), Digest::default()], DynNode::DYNCALL_DOMAIN);
/// ```
pub fn digest(&self) -> RpoDigest {
RpoDigest::new([
Felt::new(8115106948140260551),
Felt::new(13491227816952616836),
Felt::new(15015806788322198710),
Felt::new(16575543461540527115),
])
if self.is_dyncall {
RpoDigest::new([
Felt::new(8751004906421739448),
Felt::new(13469709002495534233),
Felt::new(12584249374630430826),
Felt::new(7624899870831503004),
])
} else {
RpoDigest::new([
Felt::new(8115106948140260551),
Felt::new(13491227816952616836),
Felt::new(15015806788322198710),
Felt::new(16575543461540527115),
])
}
}

/// Returns the decorators to be executed before this node is executed.
Expand Down Expand Up @@ -132,7 +178,11 @@ impl DynNodePrettyPrint<'_> {

impl crate::prettier::PrettyPrint for DynNodePrettyPrint<'_> {
fn render(&self) -> crate::prettier::Document {
let dyn_text = const_text("dyn");
let dyn_text = if self.node.is_dyncall() {
const_text("dyncall")
} else {
const_text("dyn")
};

let single_line = self.single_line_pre_decorators()
+ dyn_text.clone()
Expand Down Expand Up @@ -164,8 +214,19 @@ mod tests {
#[test]
pub fn test_dyn_node_digest() {
assert_eq!(
DynNode::default().digest(),
Rpo256::merge_in_domain(&[RpoDigest::default(), RpoDigest::default()], DynNode::DOMAIN)
DynNode::new_dyn().digest(),
Rpo256::merge_in_domain(
&[RpoDigest::default(), RpoDigest::default()],
DynNode::DYN_DOMAIN
)
);

assert_eq!(
DynNode::new_dyncall().digest(),
Rpo256::merge_in_domain(
&[RpoDigest::default(), RpoDigest::default()],
DynNode::DYNCALL_DOMAIN
)
);
}
}
7 changes: 5 additions & 2 deletions core/src/mast/node/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,10 @@ impl MastNode {
}

pub fn new_dyn() -> Self {
Self::Dyn(DynNode::default())
Self::Dyn(DynNode::new_dyn())
}
pub fn new_dyncall() -> Self {
Self::Dyn(DynNode::new_dyncall())
}

pub fn new_external(mast_root: RpoDigest) -> Self {
Expand Down Expand Up @@ -173,7 +176,7 @@ impl MastNode {
MastNode::Split(_) => SplitNode::DOMAIN,
MastNode::Loop(_) => LoopNode::DOMAIN,
MastNode::Call(call_node) => call_node.domain(),
MastNode::Dyn(_) => DynNode::DOMAIN,
MastNode::Dyn(dyn_node) => dyn_node.domain(),
MastNode::External(_) => panic!("Can't fetch domain for an `External` node."),
}
}
Expand Down
Loading
Loading