refactor(standards): Multisig Auth Component cycle-cost optimizations, comment fixes, and PROC_ROOT validation by onurinanc · Pull Request #3211 · 0xMiden/protocol

onurinanc · 2026-07-03T14:51:13Z

This PR cleans up and improves the multisig auth component:

Removes the always-true initial dup neq.0 loop guards in assert_proc_thresholds_lte_num_approvers, compute_transaction_threshold, and update_signers_and_threshold, entering each loop unconditionally with push.1 since the counters are guaranteed non-zero.
Isolates scheme_id in get_signer_at with a single movdn.7, dropping the redundant trailing movdn.4.
Reads num_approvers in update_signers_and_threshold from the intact MULTISIG_CONFIG on the stack via dup.1 instead of reloading it from local memory.
Fixes the assert_new_tx comments to reflect that IS_EXECUTED_FLAG is [1, 0, 0, 0] (flag at position 0), matching the layout movdn.3 drop drop drop relies on for replay protection.
Corrects the update_signers_and_threshold advice-map doc comment to show the interleaved per-signer layout (PUB_KEY_i, SCHEME_ID_i) that the consuming loop actually reads, instead of a grouped layout.
Expands the get_signer_at slot-push comments to show both felts (..._slot_suffix, ..._slot_prefix) placed by push.SLOT[0..2], matching the convention used elsewhere in the file.
Documents on both the standard and smart update_signers_and_threshold procedures and in the AuthMultisig component docs that growing the signer set does not re-scale existing per-procedure overrides (they stay absolute counts), advising operators to re-evaluate and raise them via set_procedure_threshold in the same transaction.
Adds an active_account::has_procedure check in set_procedure_threshold (new ERR_PROC_ROOT_NOT_IN_ACCOUNT error) so a threshold override cannot be silently stored under a PROC_ROOT the account does not have.

PhilippGackstatter

Looks good, left a few nits.

PhilippGackstatter · 2026-07-06T06:50:40Z

+#! Note: like [`multisig::update_signers_and_threshold`], this procedure does NOT re-scale existing
+#! smart procedure policies. Their immediate/delayed thresholds are absolute signature counts, not
+#! ratios, and `assert_proc_policies_lte_num_approvers` only enforces the reachability bound
+#! (`threshold <= num_approvers`). Growing the signer set silently lowers the effective signing ratio
+#! of every policy; operators must re-evaluate and, where appropriate, raise the affected policies in
+#! the same transaction that grows the signer set.
+#!


nit: same comment as here: #3208 (comment)

PhilippGackstatter · 2026-07-06T06:52:26Z

+#! Note: per-procedure threshold overrides in `PROC_THRESHOLD_ROOTS_SLOT` are NOT re-scaled by this
+#! procedure. They are absolute signature counts, not ratios, and the only cross-check is
+#! `assert_proc_thresholds_lte_num_approvers`, which enforces `override <= num_approvers`. Growing
+#! the signer set therefore leaves every override unchanged and silently lowers its effective signing
+#! ratio (e.g. a 2-of-2 override becomes 2-of-N). To preserve the intended security level, operators
+#! must re-evaluate and, where appropriate, raise the affected overrides via `set_procedure_threshold`
+#! in the same transaction that grows the signer set; this procedure neither performs nor prompts it.
+#!


nit: same comment as above

PhilippGackstatter · 2026-07-06T07:04:52Z


 const ERR_PROC_THRESHOLD_EXCEEDS_NUM_APPROVERS = "procedure threshold exceeds new number of approvers"

+const ERR_PROC_ROOT_NOT_IN_ACCOUNT = "procedure root is not one of the account's procedures"


Suggested change

const ERR_PROC_ROOT_NOT_IN_ACCOUNT = "procedure root is not one of the account's procedures"

const ERR_PROC_ROOT_NOT_IN_ACCOUNT = "cannot set procedure threshold for procedure root that is not one of the account's procedures"

nit: I'd provide a bit more context

PhilippGackstatter · 2026-07-06T07:07:07Z

    # => [SCHEME_ID_WORD, PUB_KEY]

-    movdn.3 drop drop drop
-    # => [scheme_id, PUB_KEY]
+    # move scheme_id below the whole PUB_KEY word in one step, then drop the zero padding
+    movdn.7 drop drop drop
+    # => [PUB_KEY, scheme_id, pad(15)]


nit: this would be clearer if we wrote the initial stack state as:

exec.active_account::get_initial_map_item # => [[scheme_id, 0, 0, 0], PUB_KEY]

PhilippGackstatter · 2026-07-06T07:09:55Z

-    # => [is_signer]
+    # => [is_signer, pad(16)]
+
+    # restore the call ABI stack depth of 16


nit: the usual phrase we have for this is "truncate the stack" so I'd stick to that here and in the rest of the PR

PhilippGackstatter · 2026-07-06T07:20:39Z

+/// The index `0` is supplied via `tx_script_args`. The script asserts the returned scheme id is
+/// `1` (`EcdsaK256Keccak`), exercising the getter's five-felt output through the 16-felt `call`
+/// ABI.
+#[tokio::test]
+async fn test_get_signer_at_call_abi() -> anyhow::Result<()> {
+    let script_code = "
+        begin
+            call.::miden::standards::components::auth::multisig::get_signer_at
+            # => [PUB_KEY, scheme_id, pad(11)]
+            movup.4 push.1 eq assert
+            # => [PUB_KEY, pad(11)]
+            dropw
+            # => [pad(12)]
+        end
+    ";


nit: Instead of documenting the scheme ID in the docs I'd remove those low level details and instead document them by using the type:

let expected_scheme_id = AuthScheme::EcdsaK256Keccak.as_u8(); let script_code = format!( r#" begin call.::miden::standards::components::auth::multisig::get_signer_at # => [PUB_KEY, scheme_id, pad(11)] movup.4 eq.{expected_scheme_id} assert.err="expected scheme ID {expected_scheme_id}" # => [PUB_KEY, pad(11)] dropw # => [pad(12)] end "# );

PhilippGackstatter · 2026-07-06T07:22:48Z

+    let script_code = "
+        begin
+            call.::miden::standards::components::auth::multisig::is_signer
+            # => [is_signer, pad(15)]
+            assert
+            # => [pad(15)]
+        end
+    ";
+
+    execute_multisig_getter_call(script_code, |public_keys| public_keys[0].to_commitment().into())
+        .await


What's the reason we need to provide the pub key as tx script args rather than pushing it directly in the script? If these are equivalent, consider using the push-approach to make the test easier to follow.

I've tried it, but it's not equivalent. These getters truncate their output to depth 16 assuming a 16-deep input frame. Pushing the key in script grows the stack to 20, so after the getter's truncation main returns at depth 19 (stack depth must be 16, but was 19). tx_script_args fills the frame exactly, so I kept it and added a comment explaining why.

This is why most tests have exec.sys::truncate_stack at the end to fix this conveniently.

onurinanc · 2026-07-06T13:19:41Z

@PhilippGackstatter Could you review this again as I've added more fixes after the approval?

PhilippGackstatter

Thanks for the updates! I left some questions about the multisig code changes.

PhilippGackstatter · 2026-07-07T08:20:56Z

+    # assert the stored threshold is reachable with the new signer count (threshold <= new count)
+    dup dup.4
+    # => [new_num_of_approvers, threshold, threshold, num_approvers, init, new]
+    u32assert2.err=ERR_CLEANUP_THRESHOLD_CONFIG_NOT_RECONCILED
+    u32gt assertz.err=ERR_CLEANUP_THRESHOLD_CONFIG_NOT_RECONCILED
+    # => [threshold, num_approvers, init, new]


This check is duplicate with the threshold <= new_num_approvers check in multisig::update_signers_and_threshold. It seems unintuitive to me that we do this important check in a "cleanup" procedure rather than in the "update" procedure.

I assume the motivation is to add this missing check to multisig_smart::update_signers_and_threshold? If so, the other options would be:

adding it in that procedure

keeping it here and removing it from the multisig update procedure

adding a dedicated assert_threshold_is_reachable that is called from both update procedures.

I think the second option is my least favorite and the other two are both fine, but I'd suggest the third option.

PhilippGackstatter · 2026-07-07T08:27:25Z

+    # Guard against callers that shrink the approver maps without reconciling THRESHOLD_CONFIG_SLOT.
+    exec.get_current_threshold_and_num_approvers
+    # => [threshold, num_approvers, init_num_of_approvers, new_num_of_approvers]
+
+    # assert the stored threshold is reachable with the new signer count (threshold <= new count)
+    dup dup.4
+    # => [new_num_of_approvers, threshold, threshold, num_approvers, init, new]
+    u32assert2.err=ERR_CLEANUP_THRESHOLD_CONFIG_NOT_RECONCILED
+    u32gt assertz.err=ERR_CLEANUP_THRESHOLD_CONFIG_NOT_RECONCILED


nit: I think the word "reconcile" is a bit unclear here. Do we mean that num approvers could no longer reach the threshold? If so, I'd say this more explicitly.

PhilippGackstatter · 2026-07-07T08:29:24Z

+    # assert the stored approver count already reflects the reduced signer set
+    dup.1 dup.4 eq assert.err=ERR_CLEANUP_THRESHOLD_CONFIG_NOT_RECONCILED
+    # => [threshold, num_approvers, init, new]


I don't understand why we need this check: We call cleanup_pubkey_and_scheme_id_mapping right after we have updated the threshold in storage, so fetching the threshold from storage and comparing it against the value that we have just set seems unnecessary.

The main motivation is this issue: #3227

PhilippGackstatter · 2026-07-07T08:35:50Z

@@ -43,9 +42,6 @@ pub proc assert_only_one_non_auth_procedure_called
                loc_load.0 add.1 loc_store.0
                # => [proc_index]
            end
-        else
-            dropw
-            # => [proc_index]
        end


nit: Probably in a different PR, we may want to refactor assert_only_one_non_auth_procedure_called to change the loop condition to be neq.1 so we naturally exclude the auth procedure and can simplify the branch here. We also currently use neq.0 as the condition for entering the loop, but we can also unconditionally enter it (because num procedures is always >= 2), like we did somewhere else recently.

There may be some other similar occurrences in the codebase, as we have quite a few loops over account procedures.

onurinanc · 2026-07-08T09:03:20Z

@PhilippGackstatter Is it possible that we merge this today? (so that auditors can review it one more round)

PhilippGackstatter

LGTM! I think we can merge with one approval as the changes are not too involved.

onurinanc added 7 commits July 3, 2026 14:43

fix multisig getters call API

7eae9cc

reduce cycle costs

f863f77

correct stack-layout and advice-map comments

273f8d4

warn that growing the signer set does not re-scale overrides

1a77fe5

validate PROC_ROOT belongs to the account in set_procedure_threshold

3207826

changelog

e137464

Merge remote-tracking branch 'origin/next' into refactor-multisig

069028a

onurinanc requested review from PhilippGackstatter, bobbinth and mmagician and removed request for mmagician July 3, 2026 15:09

PhilippGackstatter approved these changes Jul 6, 2026

View reviewed changes

onurinanc added 12 commits July 6, 2026 11:48

apply suggestions

a3a2caf

Merge remote-tracking branch 'origin/next' into refactor-multisig

96368fc

lint

1abe0bb

clarify auth_tx replay protection comes from assert_new_tx, not SALT

4ce5509

remove unused @Locals(1) from auth_tx

0b22e7a

remove redundant dupw/dropw in tx_policy proc-call check

f92d4d4

explain advice-loaded config is covered by ACCOUNT_DELTA_COMMITMENT

23f78ae

rename assert_new_tx to record_and_assert_new_tx

196cc11

fix inaccurate auth-component comments

e8f3a20

assert reconciled threshold config before approver-map cleanup

690e3d4

use truncate_stack in tests

b0271a7

fix

4603e80

PhilippGackstatter reviewed Jul 7, 2026

View reviewed changes

onurinanc added 2 commits July 7, 2026 15:15

apply suggestions

220e61f

merge next

6430320

onurinanc requested a review from mmagician July 8, 2026 09:07

PhilippGackstatter approved these changes Jul 8, 2026

View reviewed changes

PhilippGackstatter added this pull request to the merge queue Jul 8, 2026

Merged via the queue into next with commit 498a696 Jul 8, 2026
19 checks passed

PhilippGackstatter deleted the refactor-multisig branch July 8, 2026 10:43


		const ERR_PROC_THRESHOLD_EXCEEDS_NUM_APPROVERS = "procedure threshold exceeds new number of approvers"

		const ERR_PROC_ROOT_NOT_IN_ACCOUNT = "procedure root is not one of the account's procedures"

	const ERR_PROC_ROOT_NOT_IN_ACCOUNT = "procedure root is not one of the account's procedures"
	const ERR_PROC_ROOT_NOT_IN_ACCOUNT = "cannot set procedure threshold for procedure root that is not one of the account's procedures"

Uh oh!

Conversation

onurinanc commented Jul 3, 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!

onurinanc commented Jul 6, 2026

Uh oh!

PhilippGackstatter left a comment

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!

onurinanc commented Jul 8, 2026

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants