feat: introduce shared procedure policies for multisig auth by onurinanc · Pull Request #2670 · 0xMiden/protocol

onurinanc · 2026-03-23T18:22:21Z

Related Issue: #2669

PhilippGackstatter

Looks good! I think the overall logic makes sense.

I left a few comments about structure, validation and readability. Overall:

There is a lot of terminology that would be good to explain in a bit more detail (I think ProcedurePolicy type would be a good place):
- immediate_threshold
- delay_threshold
- lane / mode
- note restrictions
- multisig runtime (I'd try to avoid this term if possible)
- transaction-shape constraint / note-shape constraint
We should ensure we construct only valid instances of types. Applies to AuthMultisigConfig and ProcedurePolicyMode.
In general, it would be good to avoid making fields in structs public because that makes it harder to evolve these structs without breaking the API.
Afaict, we use a couple of terms interchangebly and it'd be great to pick one and use it consistently:
- "lane" and "mode" (I like the term lane in this context)
- "constraint" and "restriction"
- "direct execution lane" and "immediate execution"

PhilippGackstatter · 2026-03-25T09:02:40Z

crates/miden-standards/asm/standards/auth/multisig.masm

+
+const MAX_PROC_POLICY_NOTE_RESTRICTIONS = 3


Nit: This constant should probably go into the constants section above rather than the errors section. Would also be good to add a short doc comment.

PhilippGackstatter · 2026-03-25T09:07:02Z

crates/miden-standards/asm/standards/auth/multisig.masm

+pub proc get_procedure_policy(proc_root: word)
+    push.PROC_POLICY_ROOTS_SLOT[0..2]
+    exec.active_account::get_initial_map_item
+end
+
+#! Returns the current number of approvers after any in-transaction signer update has been applied.
+#! 
+#! Inputs:  []
+#! Outputs: [num_approvers]
+pub proc get_current_num_approvers


If these procedures do not need to be public, I'd keep them private.

Agreed to keep pub proc get_current_num_approvers it private but get_procedure_policy will be used by the smart multisig.

PhilippGackstatter · 2026-03-25T09:07:50Z

crates/miden-standards/asm/standards/auth/multisig.masm

+#! Inputs:  [proc_root]
+#! Outputs: [immediate_threshold, delayed_threshold, note_restrictions, 0]
+pub proc get_procedure_policy(proc_root: word)


Suggested change

#! Inputs: [proc_root]

#! Outputs: [immediate_threshold, delayed_threshold, note_restrictions, 0]

pub proc get_procedure_policy(proc_root: word)

#! Inputs: [PROC_ROOT]

#! Outputs: [immediate_threshold, delayed_threshold, note_restrictions, 0]

pub proc get_procedure_policy(proc_root: word)

Nit: This is a word, right?

Also, the type signature is different from the doc-comment signature.

PhilippGackstatter · 2026-03-25T09:26:47Z

crates/miden-standards/asm/standards/auth/multisig.masm

+
+        loc_store.3
+        loc_store.2
+        loc_store.1
+        loc_store.0
+        # => [proc_index, num_approvers]


Do we need to store these in that order or would the reverse also work? If so, I'd change it to use loc_storew_le.

PhilippGackstatter · 2026-03-25T09:27:06Z

crates/miden-standards/asm/standards/auth/multisig.masm

+        loc_load.3
+        swap
+        # => [num_approvers, immediate_threshold, proc_index, num_approvers]


It would be great to introduce local constants for this procedure, so it reads like loc_load.IMMEDIATE_THRESHOLD_LOC.

PhilippGackstatter · 2026-03-25T10:14:25Z

crates/miden-standards/src/account/auth/multisig/mod.rs

    pub fn new(config: AuthMultisigConfig) -> Result<Self, AccountError> {
+        for (_, policy) in config.procedure_policies() {
+            if !matches!(policy.mode(), ProcedurePolicyMode::ImmediateOnly { .. }) {
+                return Err(AccountError::other(
+                    "basic multisig procedure policies must be immediate-only",
+                ));
+            }
+
+            if policy.constraints() != ProcedurePolicyConstraints::none() {
+                return Err(AccountError::other(
+                    "basic multisig procedure policies cannot set constraints",
+                ));
+            }
+        }
+


Is there a reason we couldn't validate this in AuthMultisigConfig already? iiuc, this currently allows constructing an invalid config (e.g. using ProcedurePolicyMode::DelayOnly).

PhilippGackstatter · 2026-03-25T10:21:36Z

crates/miden-standards/asm/standards/auth/multisig.masm

+            loc_load.3 eq.0
+            if.false
+                loc_load.2
+                loc_load.3
+                dup.1 swap
+                u32assert2.err=ERR_PROC_THRESHOLD_NOT_U32
+                u32gt assertz.err=ERR_DELAYED_THRESHOLD_EXCEEDS_IMMEDIATE
+                drop
+                # => [proc_index, num_approvers]
+            end


Nit: Could we add some intermediate stack comments here to make this easier to read?

PhilippGackstatter · 2026-03-25T10:22:46Z

crates/miden-standards/asm/standards/auth/multisig.masm

+#! - delayed threshold exceeds immediate threshold.
+#! - note_restrictions is not in the range 0..=3.
+#! - constraints are configured without any threshold.
+@locals(5)


I think we only use 4 locals, but would be good to double-check.

PhilippGackstatter · 2026-03-25T10:28:18Z

crates/miden-standards/asm/standards/auth/multisig.masm

+            exec.get_procedure_policy
+            # => [immediate_threshold, delayed_threshold, note_restrictions, 0, num_procedures-1, max_called_proc_immediate_threshold]

            movdn.3 drop drop drop dup dup.3
-            # => [transaction_threshold, proc_threshold, proc_threshold, num_procedures-1, transaction_threshold]
+            # => [max_called_proc_immediate_threshold, immediate_threshold, immediate_threshold, num_procedures-1, max_called_proc_immediate_threshold]


Nit: This procedure has quite a bit of logic in it. It might make sense to call a wrapper like get_immediate_threshold that extracts the immediate threshold internally instead of get_procedure_policy and extracting the threshold here.

PhilippGackstatter · 2026-03-25T10:31:05Z

crates/miden-standards/asm/standards/auth/multisig.masm

            u32gt
-            # => [is_gt, proc_threshold, num_procedures-1, transaction_threshold]
-            # 2c. if proc_threshold > transaction_threshold, update transaction_threshold
+            # => [is_gt, immediate_threshold, num_procedures-1, max_called_proc_immediate_threshold]
+
            movup.2 movdn.3
-            # => [is_gt, proc_threshold, transaction_threshold, num_procedures-1]
+            # => [is_gt, immediate_threshold, max_called_proc_immediate_threshold, num_procedures-1]
            cdrop
-            # => [updated_transaction_threshold, num_procedures-1]
+            # => [updated_max_called_proc_immediate_threshold, num_procedures-1]


Nit/Optional: Another thing that would make this procedure easier to read is add a small u32_max procedure that does the logic internally.

…re-policies

PhilippGackstatter · 2026-03-26T13:25:44Z

crates/miden-standards/src/account/auth/multisig/mod.rs

+    pub fn with_proc_thresholds(
+        self,
+        proc_thresholds: Vec<(Word, u32)>,


Nit: We don't need a vec, so we could take impl IntoIterator<...> instead.

crates/miden-standards/src/account/auth/multisig/mod.rs

PhilippGackstatter · 2026-03-26T14:23:33Z

crates/miden-standards/src/account/auth/multisig/mod.rs

 pub struct AuthMultisigConfig {
    approvers: Vec<(PublicKeyCommitment, AuthScheme)>,
    default_threshold: u32,
-    proc_thresholds: Vec<(Word, u32)>,
+    procedure_policies: Vec<(Word, ProcedurePolicy)>,
 }


Based on validate_procedure_policies_auth_multisig, AuthMultisigConfig only supports ProcedurePolicyExecutionMode::ImmediateOnly and ProcedurePolicyNoteRestrictions::None, right?

If so, I would not change this to ProcedurePolicy because this allows representing invalid states in the first place. If we keep it as is, we do not need validate_procedure_policies_auth_multisig at all. We should also remove all constructors that take ProcedurePolicy as input for the same reason - it's an invalid state.

We can still make use of ProcedurePolicy in From<AuthMultisig> for AccountComponent to initialize storage, but it would be an implementation detail rather than a part of the public contract.

Thanks for this comment!

I initially approached this with the idea of having a shared base AuthMultisigConfig for all multisig components, but I now think that this ended up creating a structure and helper procedures that do not add much meaning.

The other reason of having procedure_policies was to keep a common structure for the MASM templates, because that would allow the items using this config slot in multisig.masm (PROC_THRESHOLD_ROOTS_SLOT) to remain common. If we do not do this, then the common procedures currently implemented for AuthMultisigSmart would most likely need to be reintroduced under similar names, such as for these procedures: assert_proc_thresholds_lte_num_approvers, compute_transaction_threshold, and set_procedure_threshold. I had also made this choice with the next PR (AuthMultisigSmart) in mind, but at this point I agree with you. I think proc_thresholds: Vec<(Word, u32)> should remain as it is.

PhilippGackstatter · 2026-03-26T14:25:40Z

crates/miden-standards/src/account/auth/multisig/procedure_policies.rs

+/// or create output notes.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
+#[repr(u8)]
+pub enum ProcedurePolicyNoteRestrictions {


Suggested change

pub enum ProcedurePolicyNoteRestrictions {

pub enum ProcedurePolicyNoteRestriction {

Nit: It's a single restriction (that may encapsulate multiple restrictions) we set and not multiple ones, so I'd still suggest using singular.

PhilippGackstatter · 2026-03-26T14:26:39Z

crates/miden-standards/src/account/auth/multisig/procedure_policies.rs

+    None = 0,
+    NoInputNotes = 1,
+    NoOutputNotes = 2,
+    NoInputOutputNotes = 3,


I think it would be worth making the last variant more precise because it could be read as "no input and output notes" or "no input or output notes", and I think it's the latter.

PhilippGackstatter · 2026-03-26T17:10:58Z

crates/miden-standards/asm/standards/auth/multisig.masm

    dup.1 swap
-    # => [num_approvers, proc_threshold, proc_threshold, PROC_ROOT]
-
    u32assert2.err=ERR_NUM_APPROVERS_OR_PROC_THRESHOLD_NOT_U32
    u32gt assertz.err=ERR_PROC_THRESHOLD_EXCEEDS_NUM_APPROVERS
    # => [proc_threshold, PROC_ROOT]

-    # Store [proc_threshold, 0, 0, 0] = PROC_THRESHOLD_WORD, where proc_threshold == 0 acts as clear.
-    push.0.0.0
-    movup.3
-    swapw
-    # => [PROC_ROOT, PROC_THRESHOLD_WORD]
+    drop
+    # => [PROC_ROOT]


If we don't need the proc_threshold, we shouldn't duplicate it a couple of lines above and remove the drop.

PhilippGackstatter · 2026-03-26T17:14:56Z

crates/miden-standards/asm/standards/auth/multisig.masm

+#!
 #! Invocation: call
+@locals(2)
 pub proc set_procedure_threshold


Why do we have both of these?

set_procedure_threshold(proc_threshold, PROC_ROOT)

set_procedure_policy(immediate_threshold, delayed_threshold, note_restrictions, PROC_ROOT)

We can do everything with set_procedure_policy and we don't need set_procedure_threshold anymore, right?

If we do need the latter, we should rename it to make it clear that it sets an immediate threshold, e.g. set_immediate_procedure_threshold.

PhilippGackstatter · 2026-03-26T17:15:17Z

crates/miden-standards/asm/standards/auth/multisig.masm

+    # => [PROC_ROOT, proc_threshold, 0, 0, 0]

+    push.PROC_POLICY_ROOTS_SLOT[0..2]
+    exec.native_account::set_map_item


This overwrites any previously set delay threshold or note restrictions with 0 - is that intended? Related to a previous comment about whether we even need this procedure anymore.

PhilippGackstatter · 2026-03-26T17:26:30Z

crates/miden-standards/src/account/auth/multisig/mod.rs


 /// Configuration for [`AuthMultisig`] component.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct AuthMultisigConfig {


This is a question to better understand the direction this is going to take, so I can better review the PR.

We plan to eventually have three multisig components, AuthMultisig, AuthMultisigSmart and AuthGuardedMultisig, right? And they all share the same code (crates/miden-standards/asm/standards/auth/multisig.masm)?

If so, would we also pay the execution price for the more advanced multisig features (note restrictions, delayed execution) for the basic AuthMultisig? And if so, it seems it would make sense to just have two components AuthMultisigSmart and AuthGuardedMultisig, since the smart version can just be configured to be like the "basic" one, right?

We could still define struct AuthMultisig(AuthMultisigSmart) (i.e. a simpler wrapper that configures the smart version without advanced features), but at the MASM level we'd only have to think about two versions, which would be helpful in terms of complexity.

Thank you for this comment! I think in this PR the path is not much clear, so related to with this comment: #2670 (comment) I've removed the procedure_policies for Multisig and the GuardedMultisig versions, and also for the AuthMultisigConfig.

But, I've kept the procedure_policies in this PR rather than the following MultisigSmart PR. (Or, do you want me to combine this PR with the following one?)

PhilippGackstatter · 2026-03-26T17:32:39Z

crates/miden-standards/asm/standards/auth/multisig.masm

+        u32assert2.err=ERR_PROC_THRESHOLD_NOT_U32
+        u32gt assertz.err=ERR_PROC_THRESHOLD_EXCEEDS_NUM_APPROVERS
+        # => [immediate_threshold, proc_index, num_approvers]



I think this assertion makes sense to have in this procedure, but the validation starting from here (threshold and note restriction validation) seems unnecessary because we assert the same things in set_procedure_policy already, right?

If we do somehow need it, it'd be awesome if we could deduplicate the validation code into a helper procedure, because we can maintain it in one place.

onurinanc added 2 commits March 23, 2026 18:52

add procedure policies for multisig auth

f42ac2c

rename procedure policy slot

8259a5a

onurinanc changed the title ~~refactor: Introduce procedure_policies rather than proc_thresholds for AuthMultisig~~ feat: introduce shared procedure policies for multisig auth Mar 23, 2026

onurinanc added 2 commits March 23, 2026 19:29

changelog

27c8a1f

clippy

55fad1f

This was referenced Mar 23, 2026

Introduce procedure_policies replacing proc_thresholds for AuthMultisig OpenZeppelin/miden-confidential-contracts#91

Open

Introduce procedure_policies replacing proc_thresholds for AuthMultisig #2669

Open

onurinanc added 3 commits March 24, 2026 08:23

Merge remote-tracking branch 'upstream/next' into procedure-policies

39f2a3b

change storage structure

90aaa8c

fix test

e297009

bobbinth requested review from PhilippGackstatter and bobbinth March 25, 2026 06:20

PhilippGackstatter reviewed Mar 25, 2026

View reviewed changes

onurinanc added 4 commits March 25, 2026 18:16

Merge branch 'next' into procedure-policies

d071a9a

fix comments

16343fd

Merge remote-tracking branch 'upstream/next' into procedure-policies

52989bc

Merge remote-tracking branch 'origin/procedure-policies' into procedu…

4bc9a0e

…re-policies

onurinanc requested a review from PhilippGackstatter March 26, 2026 10:15

PhilippGackstatter reviewed Mar 26, 2026

View reviewed changes

onurinanc added 4 commits March 27, 2026 13:29

procedure_policies -> procedure_thresholds in Basic Multisig types

9d7cd05

fix

23deaab

fix

82b0912

fix changelog

5f2a848

onurinanc requested a review from PhilippGackstatter March 27, 2026 13:02

	pub enum ProcedurePolicyNoteRestrictions {
	pub enum ProcedurePolicyNoteRestriction {


		const MAX_PROC_POLICY_NOTE_RESTRICTIONS = 3

Conversation

onurinanc commented Mar 23, 2026

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants