I think we don't need the if-else branch. The default proc_threshold is 0 (right?) which is never greater than num_approvers, so the assertion should always succed.

drop drop drop
# => [proc_threshold, proc_index, num_approvers]

dup.2 u32assert2.err=ERR_PROC_THRESHOLD_NOT_U32
u32gt assertz.err=ERR_PROC_THRESHOLD_EXCEEDS_NUM_APPROVERS
# => [proc_index, num_approvers]

We could probably remove this because we also do u32assert2 later with the same value.

We could probably remove this because we also do u32assert2 later with the same value.

Nit: Would it be sufficient to call these psm::is_initialized and psm::verify_signature? We basically always use the module name, so "psm" does not really need to be duplicated.

Iiuc, the only reason these two slots need to be a map is because signature::verify_signatures reads the scheme IDs and pub keys from a map slot. As mentioned in the previous PR, I think verify_signatures is a specialized implementation for the multisig that still happens to be in miden::standards, but imo should be moved to the multisig account component code eventually.

I think a reasonable alternative to make public in miden::standards is verify_signature_by_scheme. If we used it for the PSM signature verification, it would avoid the need to pass a map slot, and in turn we could make PSM_SCHEME_ID_SLOT and PSM_PUBLIC_KEYS_SLOT value slots.

I think we can leave things as they are for this PR, but this is something we should do eventually.

Iiuc, the only reason these two slots need to be a map is because signature::verify_signatures reads the scheme IDs and pub keys from a map slot. As mentioned in the previous PR, I think verify_signatures is a specialized implementation for the multisig that still happens to be in miden::standards, but imo should be moved to the multisig account component code eventually.

This one seems better for later improvements.

Could we simplify this?

push.PSM_CONFIG_SLOT[0..2] exec.active_account::get_item
# => [0, 0, is_psm_initialized, is_psm_signature_required, new_is_psm_signature_required]

movup.3 drop
# => [0, 0, is_psm_initialized, new_is_psm_signature_required]
# => [NEW_PSM_CONFIG_WORD]

push.PSM_CONFIG_SLOT[0..2]
exec.native_account::set_item
dropw
# => []

One notable change is that exec.is_psm_initialized used get_initial_item and I've changed it to get_item. I don't think it matters for this particular case, but get_item + set_item would preserve updates to other parts of the slot made during the course of execution (of which there aren't currently any so for now it doesn't matter I believe).

Nit: Not super important currently, but pub procedures in miden::standards should ideally have type signatures. Here, I think i1 is the correct one (docs).

I think we can drop the push.1 eq part as is_psm_initialized is already a bool (or should be). If it isn't assert would panic, which I think is fine - or are we deliberately trying to be conservative?

Architecture question: Would it be possible to have two dedicated account components, AuthMultisig and AuthMultisigPsm?

AuthMultisig would be the same as it is now (plus the newly added validation), but without any PSM code.

AuthMultisigPsm would be the same as this module, but without the exec.psm::is_psm_initialized + if.true check and always calling exec.psm::verify_psm_signature. In other words, it would always be PSM-enabled and there is no runtime flag to opt out of it. If I understand correctly, then we could remove the is_psm_initialized flag entirely and simplify the PSM config slot to just contain is_psm_signature_required.

Or is there a need to enable/disable the PSM during the lifetime of an account (in which case the current approach makes sense)?

@PhilippGackstatter Thank you for your comment. My first approach was that one and I think that would be better and we don't need to add is_psm_initialized.

But, this time we will have almost the same code as AuthMultisig  and AuthMultsigPsm both in the MASM part and the Rust part. What are your thoughts on this?

Or is there a need to enable/disable the PSM during the lifetime of an account (in which case the current approach makes sense)?

No, enable and disable is just for is_psm_signature_required, we won't provide enable and disable procedures for is_psm_initialized

I think we should also get input from @bobbinth as both approaches have pros and cons, and this impacts the codebase. I will hold off on making changes until we hear back.

I think we should be able to deduplicate both of these.

MASM deduplication

Basically, the multisig.masm code is identical except for the extra psm::verify_psm_signature call, right? What we would need ideally is a multisig library that we can use to build the AuthMultisig and AuthMultisigPsm variants. This multisig library would not be part of miden::standards because imo it is not reusable enough. Then we could do:

# AuthMultisig code
use multisig

@auth_script
pub proc auth_tx_multisig(salt: word)
    exec.multisig::auth_tx
end

# AuthMultisigPsm code
use multisig
use psm

@auth_script
pub proc auth_tx_multisig_psm(salt: word)
    exec.multisig::auth_tx
    exec.psm::verify_signature
end

The main difficulty is setting this up with our current approach to building account component code. Account component code in crates/miden-standards/asm/account_components/auth/* cannot rely on other libraries (only miden::{standards, protocol}), so setting this up is difficult right now. I believe once we have proper support for projects (0xMiden/miden-vm#2510) this may become relatively easy and we could define a multisig library and two account components that statically link against that library: a standard multisig account component (AuthMultisig) and a PSM-enabled multisig account component (AuthMultisigPsm).

So instead of doing a lot of extra lifting in build.rs to make this happen, the simplest thing to do for now is to move the multisig code back into miden::standards to make it accessible in library form (where it should not live long-term). This should allow us to do the above while only swapping out the imports for use miden::standards::auth::multisig and use miden::standards::auth::psm.

Rust deduplication

For the Rust side, I imagine we should be able to wrap AuthMultisig into AuthMultisigPsm, something roughly like this:

// todo
pub struct PsmConfig;

pub struct AuthMultisigPsm {
    multisig: AuthMultisig,
    psm_config: PsmConfig,
}

impl From<AuthMultisigPsm> for AccountComponent {
    fn from(multisig: AuthMultisigPsm) -> Self {
        let AuthMultisigPsm { multisig, psm_config } = multisig;

        // Convert the inner multisig into a component to reuse its storage slots and metadata.
        let multisig_component = AccountComponent::from(multisig);
        let (psm_slots, psm_slot_metadata) = psm_config.into_component_parts();

        // TODO: We can probably add AccountComponent::into_parts for this.
        let mut storage_slots = multisig_component.storage_slots().clone();
        storage_slots.extend(psm_slots);

        let mut metadata = multisig_component.metadata().clone();
        metadata.extend(psm_slot_metadata);

        AccountComponent::new(multisig_psm_library(), storage_slots, metadata).expect("todo")
    }
}

So to summarize:

• We can (temporarily) move the multisig code into miden::standards to make it accessible in library-form for both multisig components.
• We should be able to wrap AuthMultisig into AuthMultisigPsm for deduplication.

Does this makes sense?

Thank you @PhilippGackstatter. Yes, this makes sense, and I think it is better than the current architecture. I'll make the changes relatively and move the multisig.masm to the miden::standards::auth

And, what's @auth_script can we use it now, and do we need some other changes for this?

You can use it as-is. This was a recent change to identify auth procedures via an attribute rather than the procedure's name (#2534).

Same nit as above: I'd put newlines before and after inputs/outputs section.

I think I would remove the docs here and in account_components/auth/multisig.masm since the docs already exist at the original procedure definition, and now we have it duplicated three times.

A few comments about this procedure (probably for future PRs):

• This procedure should probably require one of the highest thresholds.
• I wonder if we should enforce that if this procedure is called, no other procedure should be called in the same (i.e., PSM rotation requires a "single-purpose" transaction).
• It is not clear to me why we need to disable/enable PSM requirement using the storage. Could we not detect that this procedure has been called in the verify_signature by using native_account::was_procedure_called kernel procedure?

Separately, this is probably one of the first procedures that we should put a "delay" on in the next iteration (i.e., rotating PSM should not take the effect immediately, and ideally, users should be able to cancel the rotation with enough signatures).

I wonder if we should enforce that if this procedure is called, no other procedure should be called in the same (i.e., PSM rotation requires a "single-purpose" transaction).

Similar to my comment above related to procedure threshold overrides, we can also efficiently compute and take the highest threshold if one of them are called from the list of overrides.

Let's say, we have the the following procedure and the corresponding threshold configuration, for an M-of-N

receive_asset -> 1
update_psm_signature -> 4
update_signers_and_thresholds -> 4

Also, we have the default_threshold -> 3

So, if we call both of the above 3 procedures at the same time, the highest thresholds will apply. So, in any case, since update_psm_signature is one of the highest thresholds, then, its corresponding thresholds will apply any case.

If we need "single purpose" transaction, as an addition, we might also add this, but I don't think we need it when we compute the highest threshold.

It is not clear to me why we need to disable/enable PSM requirement using the storage. Could we not detect that this procedure has been called in the verify_signature by using native_account::was_procedure_called kernel procedure?

We just enable/disable signature requirement, for the I'm not sure about the exact logic, but PSM is updating the storage without depending on the PSM signature requirement. It is also audited and no issues found related to that, so I believe it would be better to stick it to the current implementation.

Separately, this is probably one of the first procedures that we should put a "delay" on in the next iteration (i.e., rotating PSM should not take the effect immediately, and ideally, users should be able to cancel the rotation with enough signatures).

Yes, the timelock controller is on the way after this PR, and I agree that any key rotation requires a delay of ideally ~48 hours.

As mentioned in the previous comment, it should be possible to replace this with something like

procref.update_psm_public_key
exec.native_account::was_procedure_called
# => [was_update_psm_procedure_called]

It would be good sticking to the current implementation as it's already audited, but for the next audit iteration, I agree this should be a good candidate to change the logic a bit. wdyt?

Fine for this PR - but I'd add this to the list of follow-up items.

Added here: #2527 (comment)

Do we not need to pass the scheme as a parameter here as well? Or is the assumption that the PSAM signature scheme cannot be changed post deployment? (if so, we should state this somewhere).

I think I would remove the docs here and in account_components/auth/multisig.masm since the docs already exist at the original procedure definition, and now we have it duplicated three times.

Nit: I think this is already mentioned in the first sentence.

Nit: I'd remove the MSG output if it is indeed identical to the input (see #2523 for motivation). If callers need it, they should dupw it themselves.

Nit

Not for this PR, but at the next opportunity, I think we should introduce a wrapper for these two, because they often appear together and there should be a type encapsulating these, so we could avoid these tuple types (PublicKeyCommitment, AuthScheme) in a couple of places. Since we often call these approvers, we could call that ApproverPublicKey.

Alternatively, we could move AuthScheme into PublicKeyCommitment directly, but I haven't checked if this works at all or whether it's a good idea.

Nit

After the PSM key rotation, we're running two transactions: "reenable" and "next". Do these test different things? It seems like it would be sufficient to try execute with the old PSM key against the a tx, which should fail, and then try again with the new PSM key against the same tx and it should succeed. I think this is what we're already doing for "next", so can't we drop the "reenable" case?

Could we move the PSM-related test to a new crates/miden-testing/tests/auth/multisig_psm.rs module? This module is already 1500 lines of code without the PSM tests.

Edit: Nevermind, I saw these are duplicated there.

Same comment as in the other file: proc_root is already the Word we need.

Also, I think the tests in crates/miden-testing/tests/auth/hybrid_multisig.rs are duplicated here, right? If so, I'd remove the ones in hybrid multisig because I don't think we're really testing additional code paths by duplicating these.

Similar comment as before: If these tests are duplicated in hybrid multisig, I would remove the duplication and only keep the ones here, as I don't think we're testing additional code paths.

I think we can later think about ways to run these tests with rstest using uniform and hybrid schemes. This should probably apply to hybrid_multisig.rs and multisig.rs / multisig_psm.rs in general, (but for now I'd only remove the duplicated tests added in this PR).

I don't think we need to single out ECDSA signature here as PSM could work with all standardized signature schemes.