refactor(multisig)!: move from triggers to custom instructions #5217

s8sato · 2024-11-02T21:27:50Z

Context

Calling a trigger with args as a multisig operation was not a clear interface
The multisig implementation as triggers introduced validation exceptions for registering and executing triggers to bridge the authority gap between the caller and the executor: feat: improve multisig utility and usability #5027

Addresses Ergonomic MST #4930: encapsulate multisig operations
Resolves as for multisig Dedup wasm blobs #5185

Solution

Assign dedicated custom instructions to the multisig operations
Switch to the executor context and package validation and execution logics as the custom instructions

As a side benefit of removing triggers i.e. no event pools, recursive approvals immediately propagate without ticking time

Migration Guide

Introduces MultisigRegister MultisigPropose MultisigApprove custom instructions that replace MultisigAccountArgs MultisigTransactionsArgs
Removes CanRegisterAnyTrigger CanUnregisterAnyTrigger permissions that become useless and are unlikely to be used
Though it becomes useless for multisig, retains the capability to register wasm triggers in genesis

Future Work

Checklist

I've read CONTRIBUTING.md.
All review comments have been resolved.
All CI checks pass.

github-actions · 2024-11-02T21:28:06Z

@BAStos525

wasm/libs/default_executor/src/lib.rs

wasm/libs/default_executor/src/multisig/account.rs

s8sato · 2024-11-03T09:24:47Z

wasm/libs/default_executor/src/multisig/transaction.rs

+                let mut executor = executor.clone();
+                executor.context_mut().authority = target_account.clone();
+                propose_to_approve_me.execute(&executor)?;


context_mut here is only to indicate what authority is executing the instruction:

I've realized that host.submit() goes through no validation, which seems to me to be rather a correct behavior. That's why context mutations whose purpose is to pass permission validation are removed at 2e2dcd1

mversic

amazing stuff, so much better

crates/iroha_executor/src/default.rs

crates/iroha_executor/src/lib.rs

crates/iroha_multisig_data_model/src/lib.rs

crates/iroha_telemetry_derive/Cargo.toml

defaults/genesis.json

crates/iroha_multisig_data_model/src/lib.rs

crates/iroha/tests/multisig.rs

crates/iroha_multisig_data_model/src/lib.rs

s8sato · 2024-11-04T07:15:19Z

Self-reviewed and made fixes 4e4a6b9 0e4fc3a for now. I'm going to address your comments tomorrow @mversic

crates/iroha_multisig_data_model/src/lib.rs

wasm/libs/default_executor/src/multisig/account.rs

wasm/libs/default_executor/src/multisig/transaction.rs

BREAKING CHANGES: - (api-changes) `MultisigRegister` `MultisigPropose` `MultisigApprove` custom instructions Signed-off-by: Shunkichi Sato <[email protected]>

Signed-off-by: Shunkichi Sato <[email protected]>

…ssions Signed-off-by: Shunkichi Sato <[email protected]>

…ions Signed-off-by: Shunkichi Sato <[email protected]>

Signed-off-by: Shunkichi Sato <[email protected]>

Signed-off-by: Ekaterina Mekhnetsova <[email protected]>

Signed-off-by: Shunkichi Sato <[email protected]>

…::custom::multisig` Signed-off-by: Shunkichi Sato <[email protected]>

Signed-off-by: Shunkichi Sato <[email protected]>

This reverts commit 255782e. Signed-off-by: Shunkichi Sato <[email protected]>

Signed-off-by: Shunkichi Sato <[email protected]>

s8sato

Updates:

rename to visit_custom_instructions
move to {iroha_executor::default, iroha_executor_data_model}::custom::multisig
minor updates since refactor(multisig)!: move from triggers to custom instructions #5217 (review)

Notes:

Something happened on merging main branch and @outoftardis involved, but no effect on changed files. Never mind

s8sato · 2024-11-06T12:34:38Z

crates/iroha_multisig_data_model/src/lib.rs

    pub signatories: BTreeMap<AccountId, Weight>,
-    /// Threshold of total weight at which the multisig is considered authenticated
+    /// Threshold of total weight at which the multisig account is considered authenticated
    pub quorum: u16,
    /// Multisig transaction time-to-live in milliseconds based on block timestamps. Defaults to [`DEFAULT_MULTISIG_TTL_MS`]
    pub transaction_ttl_ms: u64,


If we prohibited 0ms, why not also prohibit 1ms?

s8sato · 2024-11-06T12:37:42Z

crates/iroha_multisig_data_model/src/lib.rs

    pub signatories: BTreeMap<AccountId, Weight>,
-    /// Threshold of total weight at which the multisig is considered authenticated
+    /// Threshold of total weight at which the multisig account is considered authenticated
    pub quorum: u16,


What good is this type safety, since 0 doesn't lead to errors and there's no incentive for users to set quorum 0?

crates/iroha_multisig_data_model/src/lib.rs

s8sato · 2024-11-06T13:38:24Z

wasm/libs/default_executor/src/multisig/account.rs

+            // Temporarily grant a multisig role to the registrant to delegate the role to the signatories
+            Role::new(multisig_role.clone(), registrant.clone()),


The main topic here should be why Grant::account_role has passed but hasn't taken the expected effect, but okay,

authenticate intrinsics?

My understanding is that authentication by signature is required on facing extrinsic transactions and queries. Imo intrinsics should not be signed in cryptographic sense, especially in the case of multisig, proposals and approvals are extrinsic while automatically deployed multisig transactions themselves are intrinsic, and collecting sufficient signatures will be the authentication

Btw the concept of "keys of a multisig account" or "sign as a multisig account" will be removed by #5022 because keys should represent a personal identity

authorize intrinsics?

The only instructions that should be validated would be extrinsics which hasn't yet experienced authorization i.e. the contains of proposal. Other auxiliary instructions that compose the multisig logic itself should be infallible

s8sato · 2024-11-06T13:38:36Z

wasm/libs/default_executor/src/multisig/account.rs

+            // Temporarily grant a multisig role to the registrant to delegate the role to the signatories
+            Role::new(multisig_role.clone(), registrant.clone()),


Previously it was the design that the registrant of a multisig account got equivalent to the domain owner, so as to register and configure a multisig account in the domain. Now that host.submit() from executor goes through no validation, there's no need to elevate the authority. There should be essentially no difference between assigning any desired authority and bypassing any validation

wasm/libs/default_executor/src/multisig/transaction.rs

wasm/libs/default_executor/src/multisig/account.rs

crates/iroha/src/lib.rs

crates/iroha_executor/src/default/custom/mod.rs

Signed-off-by: Shunkichi Sato <[email protected]>

crates/iroha_executor/src/default/custom/multisig/transaction.rs

Cargo.toml

crates/iroha_data_model/src/isi.rs

crates/iroha_executor/src/default/custom/mod.rs

crates/iroha_data_model/src/visit.rs

wasm/libs/default_executor/src/multisig/account.rs

crates/iroha_executor_data_model/src/lib.rs

mversic · 2024-11-07T08:19:12Z

crates/iroha_executor_data_model/Cargo.toml

@@ -16,5 +16,7 @@ iroha_executor_data_model_derive = { path = "../iroha_executor_data_model_derive
 iroha_data_model.workspace = true
 iroha_schema.workspace = true

+derive_more = { workspace = true, features = ["constructor", "from"] }
+parity-scale-codec.workspace = true


Suggested change

parity-scale-codec.workspace = true

why is it needed?

Because multisig data models need to impl either Encode Decode or WrapperTypeEncode WrapperTypeDecode for IntoSchema

I'm a bit confused because none of the default permissions in executor_data_model/src/permission.rs implement Decode/Encode

That's probably because the permissions are not listed in types!

crates/iroha_executor/src/default/mod.rs

Signed-off-by: Shunkichi Sato <[email protected]>

s8sato · 2024-11-08T06:09:32Z

Updates:

minor updates since refactor(multisig)!: move from triggers to custom instructions #5217 (review)
nonzero quorum and transaction_ttl_ms
replace host.submit with executor.visit_*
- resolves Unexpected Grant::account_role behavior in custom instructions #5222
- keeps Panics in custom instructions on registering a new account with metadata #5223

Signed-off-by: Shunkichi Sato <[email protected]>

github-actions bot added api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha labels Nov 2, 2024

s8sato force-pushed the refactor/4930/custom_instructions branch from e8e1316 to a6b0dce Compare November 2, 2024 21:39

s8sato commented Nov 2, 2024

View reviewed changes

wasm/libs/default_executor/src/lib.rs Outdated Show resolved Hide resolved

wasm/libs/default_executor/src/multisig/account.rs Outdated Show resolved Hide resolved

s8sato self-assigned this Nov 2, 2024

s8sato marked this pull request as ready for review November 2, 2024 22:11

s8sato requested review from mversic, SamHSmith, outoftardis, 0x009922 and dima74 as code owners November 2, 2024 22:11

s8sato enabled auto-merge (squash) November 2, 2024 22:13

s8sato commented Nov 3, 2024

View reviewed changes

mversic requested changes Nov 3, 2024

View reviewed changes

s8sato force-pushed the refactor/4930/custom_instructions branch from 5b0dbeb to 4e4a6b9 Compare November 4, 2024 07:14

s8sato force-pushed the refactor/4930/custom_instructions branch 2 times, most recently from 48753cb to 0e4fc3a Compare November 4, 2024 10:42

mversic reviewed Nov 5, 2024

View reviewed changes

crates/iroha_multisig_data_model/src/lib.rs Outdated Show resolved Hide resolved

mversic reviewed Nov 5, 2024

View reviewed changes

wasm/libs/default_executor/src/multisig/account.rs Outdated Show resolved Hide resolved

mversic reviewed Nov 5, 2024

View reviewed changes

wasm/libs/default_executor/src/multisig/transaction.rs Outdated Show resolved Hide resolved

s8sato added 8 commits November 5, 2024 16:39

refactor!: move multisig logics to custom instructions

e37bb4d

BREAKING CHANGES: - (api-changes) `MultisigRegister` `MultisigPropose` `MultisigApprove` custom instructions Signed-off-by: Shunkichi Sato <[email protected]>

fix: don't continue to grant a role on failing to register the role

00ec667

Signed-off-by: Shunkichi Sato <[email protected]>

docs: fix doc test

a4ac5b0

Signed-off-by: Shunkichi Sato <[email protected]>

chore: remove CanRegisterAnyTrigger CanUnregisterAnyTrigger permi…

657d3ad

…ssions Signed-off-by: Shunkichi Sato <[email protected]>

refactor: don't branch into visit and execute until leaf instruct…

b158c59

…ions Signed-off-by: Shunkichi Sato <[email protected]>

refactor: clone and drop executor instance on recursive execution

4eedd61

Signed-off-by: Shunkichi Sato <[email protected]>

fix: don't just execute arbitrary approved instructions

da8e7fb

Signed-off-by: Shunkichi Sato <[email protected]>

fix: propagate executor verdict

f54bd6e

Signed-off-by: Shunkichi Sato <[email protected]>

s8sato mentioned this pull request Nov 5, 2024

chore: reorganize data_model crates #5199

Merged

outoftardis and others added 7 commits November 7, 2024 00:20

docs: Update readmes (hyperledger-iroha#5219)

0e8f7e6

Signed-off-by: Ekaterina Mekhnetsova <[email protected]>

review: rename to visit_custom_instructions

3c1888d

Signed-off-by: Shunkichi Sato <[email protected]>

review: move to `{iroha_executor::default, iroha_executor_data_model}…

bc1d497

…::custom::multisig` Signed-off-by: Shunkichi Sato <[email protected]>

review: minor updates since #pullrequestreview-2414675315

bdd2a09

Signed-off-by: Shunkichi Sato <[email protected]>

review: submit a new account with metadata filled

255782e

Signed-off-by: Shunkichi Sato <[email protected]>

Revert "review: submit a new account with metadata filled"

b0208cc

This reverts commit 255782e. Signed-off-by: Shunkichi Sato <[email protected]>

Merge branch 'main' into refactor/4930/custom_instructions

e95f743

Signed-off-by: Shunkichi Sato <[email protected]>

s8sato requested a review from DCNick3 as a code owner November 6, 2024 16:25

s8sato commented Nov 6, 2024

View reviewed changes

crates/iroha/src/lib.rs Show resolved Hide resolved

crates/iroha_executor/src/default/custom/mod.rs Outdated Show resolved Hide resolved

fix: prevent a double grant to the registrant who is also a signatory

45b24d9

Signed-off-by: Shunkichi Sato <[email protected]>

s8sato force-pushed the refactor/4930/custom_instructions branch from 7b437ec to 45b24d9 Compare November 6, 2024 19:20

s8sato mentioned this pull request Nov 6, 2024

#[derive(CustomInstruction)] or something for custom instructions #5221

Open

s8sato commented Nov 7, 2024

View reviewed changes

crates/iroha_executor/src/default/custom/multisig/transaction.rs Outdated Show resolved Hide resolved

mversic reviewed Nov 7, 2024

View reviewed changes

crates/iroha_executor_data_model/src/lib.rs Outdated Show resolved Hide resolved

mversic reviewed Nov 7, 2024

View reviewed changes

s8sato mentioned this pull request Nov 7, 2024

Unexpected Grant::account_role behavior in custom instructions #5222

Open

mversic reviewed Nov 7, 2024

View reviewed changes

crates/iroha_executor/src/default/mod.rs Outdated Show resolved Hide resolved

This was referenced Nov 7, 2024

Panics in custom instructions on registering a new account with metadata #5223

Open

Concurrent MSTs of the same proposals? #5224

Open

s8sato added 2 commits November 8, 2024 15:06

review: minor updates since #pullrequestreview-2418103555

e0a96f4

Signed-off-by: Shunkichi Sato <[email protected]>

review: nonzero quorum and transaction_ttl_ms

183ff26

Signed-off-by: Shunkichi Sato <[email protected]>

review: replace host.submit with executor.visit_*

70d51d0

Signed-off-by: Shunkichi Sato <[email protected]>

s8sato force-pushed the refactor/4930/custom_instructions branch from 9d5c74f to 70d51d0 Compare November 8, 2024 06:24

s8sato changed the title ~~refactor!: move multisig logics to custom instructions~~ refactor(multisig)!: move from triggers to custom instructions Nov 8, 2024

s8sato disabled auto-merge November 8, 2024 07:39

s8sato enabled auto-merge (squash) November 8, 2024 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(multisig)!: move from triggers to custom instructions #5217

refactor(multisig)!: move from triggers to custom instructions #5217

s8sato commented Nov 2, 2024 •

edited

Loading

github-actions bot commented Nov 2, 2024

s8sato Nov 3, 2024 •

edited

Loading

mversic left a comment

s8sato commented Nov 4, 2024 •

edited

Loading

s8sato left a comment •

edited

Loading

s8sato Nov 6, 2024

s8sato Nov 6, 2024

s8sato Nov 6, 2024

s8sato Nov 6, 2024

mversic Nov 7, 2024

s8sato Nov 7, 2024

mversic Nov 7, 2024

s8sato Nov 7, 2024

s8sato commented Nov 8, 2024

		// Temporarily grant a multisig role to the registrant to delegate the role to the signatories
		Role::new(multisig_role.clone(), registrant.clone()),

refactor(multisig)!: move from triggers to custom instructions #5217

Are you sure you want to change the base?

refactor(multisig)!: move from triggers to custom instructions #5217

Conversation

s8sato commented Nov 2, 2024 • edited Loading

Context

Solution

Migration Guide

Future Work

Checklist

github-actions bot commented Nov 2, 2024

s8sato Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

mversic left a comment

Choose a reason for hiding this comment

s8sato commented Nov 4, 2024 • edited Loading

s8sato left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

authenticate intrinsics?

authorize intrinsics?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s8sato commented Nov 8, 2024

s8sato commented Nov 2, 2024 •

edited

Loading

s8sato Nov 3, 2024 •

edited

Loading

s8sato commented Nov 4, 2024 •

edited

Loading

s8sato left a comment •

edited

Loading