feat: improve multisig utility and usability #5027

s8sato · 2024-09-03T00:10:25Z

BREAKING CHANGES:

(api-changes) CanRegisterAnyTrigger CanUnregisterAnyTrigger permission
(config-changes) defaults/genesis.json assumes wasm_triggers[*].action.executable is prebuilt under wasm_samples/target/prebuilt/

Major commits:

feat: support multisig recursion
feat: introduce multisig quorum and weights
feat: add multisig subcommand to client CLI
feat: introduce multisig transaction time-to-live
feat: predefine multisig world-level trigger in genesis
feat: allow accounts in domain to register multisig accounts

Context

Partially resolves Ergonomic MST #4930

Solution

Each commit is explained below, starting with the most recent. You can see the commit history here

feat: support multisig recursion

Allows multisig to function hierarchically, expected to be useful for e.g. automating organizational approval flows.

When a multisig transaction proposed, every subordinate node (multisig account and corresponding transactions registry) automatically get ready to accept approvals that will be coming up from individual signatories
Using CLI, individual signatories can query all pending multisig transactions they are involved in, not only those proposed to their direct multisig account

Tests:

cargo test -p iroha integration::multisig::multisig_recursion
bash scripts/tests/multisig.recursion.sh

feat: introduce multisig quorum and weights

Inspired by Sui's multisig. Allows for flexible, if not completely free, authentication policies beyond "m of n". For example, weight equivalent to quorum represents administrative privileges

feat: add multisig subcommand to client CLI

$ cargo build
$ ./target/debug/iroha multisig

The subcommand related to multisig accounts and transactions

Usage: iroha multisig <COMMAND>

Commands:
  register  Register a multisig account
  propose   Propose a multisig transaction
  approve   Approve a multisig transaction
  list      List pending multisig transactions relevant to you
  help      Print this message or the help of the given subcommand(s)

Options:
  -h, --help  Print help

You can see more usage in the testing script

feat: introduce multisig transaction time-to-live

Considers the latest block timestamp as the current time and determines timeout, when the transactions registry is called

feat: predefine multisig world-level trigger in genesis

Defines a global trigger in genesis that exercises authority over all domains. There will be three types of triggers on the system side related to multisig:

Domains initializer has world-level authority and in response to a domain creation event, registers an accounts registry handled by the domain owner authority
Accounts registry has domain-level authority and when called by a domain resident, registers a multisig account along with a transactions registry
Transactions registry has account-level authority and when called by a multisig signatory, accepts proposals or approvals for multisig transactions, and is responsible for executing them

feat: allow accounts in domain to register multisig accounts

Accounts registry has authority of the domain owner, so access was previously restricted. This commit allows anyone to organize any multisig account within the domain.

This may be too lenient. Discussion

Review notes

To get an overview,

cargo test -p iroha integration::multisig::multisig
bash scripts/tests/multisig.sh

sequenceDiagram
    autonumber
    participant oo as etc.
    participant DI as Domains Initializer
    Note over DI: /world
    oo-->>DI: domain created
    create participant AR as Accounts Registry
    DI-->>AR: register
    Note over AR: /world/domain
    create actor s0 as signatory 0
    oo->>s0: register
    create actor s1 as signatory 1
    oo->>s1: register
    s0->>AR: request new ms account
    create actor 01 as ms account 01
    AR-->>01: register
    create participant TR as Transactions Registry
    AR-->>TR: register
    AR-->>s0: grant ms role
    AR-->>s1: grant ms role
    Note over 01,TR: /world/domain/account
    s1->>TR: propose instructions
    create participant tx as pending ms transaction
    TR-->>tx: deploy ms transaction
    s0->>TR: approve instructions
    destroy tx
    TR-->>tx: execute instructions

The dotted line indicates an automatic process

Checklist

I've read CONTRIBUTING.md.
I've written integration tests for the code changes.
All review comments have been resolved.

github-actions · 2024-09-03T00:10:40Z

@BAStos525

nxsaken · 2024-09-03T08:07:19Z

Genesis needs to be updated

defaults/genesis.json

scripts/tests/multisig.recursion.sh

client/tests/integration/multisig.rs

tools/kagami/src/genesis/generate.rs

wasm_samples/multisig_accounts/src/lib.rs

wasm_samples/multisig_transactions/src/lib.rs

wasm_samples/multisig_domains/src/lib.rs

client/tests/integration/multisig.rs

Erigara

I think this change is great improvement to usability of multisigs.

client/tests/integration/multisig.rs

client_cli/src/main.rs

wasm_samples/executor_custom_data_model/src/multisig.rs

wasm_samples/multisig_accounts/build.rs

wasm_samples/multisig_domains/src/lib.rs

wasm_samples/multisig_transactions/src/lib.rs

tools/kagami/src/genesis/generate.rs

wasm_samples/multisig_domains/src/lib.rs

s8sato · 2024-10-03T07:33:45Z

Updates:

review doc: add a reference to multisig recursion diagram
review fix: signatories and weights must be equal in length
review fix: condition to rebuild multisig subordinate triggers
review fix: genesis.json includes not wasm blobs but hashes
review fix: autofill multisig account domains
review fix: avoid name collisions of multisig role and triggers
review fix: perpetuate multisig domain-level authority
review fix: add a domain for domain-free system accounts

commit history

Notes:

Currently script tests (scripts/tests/multisig.*) are failing after rebase for some reason

hooks/pre-commit.sample

defaults/genesis.json

crates/iroha_data_model/src/transaction.rs

s8sato · 2024-10-08T14:15:29Z

Updates:

review fix: exception for multisig role registration
review fix: give genesis.json a special field to register wasm triggers
DROP: review fix: genesis.json includes not wasm blobs but hashes
EDIT: review fix: avoid name collisions of multisig role and triggers

commit history

Notes:

Currently script tests (scripts/tests/multisig.*) are failing after rebase for some reason

This was because only integration tests passed due to my overlook of privileged Alice in TestGenesis. Fixed
Stopped pursuing the cause of the failing integration::extra_functional::restart_peer::restarted_peer_should_have_the_same_asset_amount

BREAKING CHANGES: - (api-changes) `CanRegisterAnyTrigger` `CanUnregisterAnyTrigger` permission - (config-changes) `defaults/genesis.json` assumes `wasm_triggers[*].action.executable` is prebuilt under `wasm_samples/target/prebuilt/` Major commits: - feat: support multisig recursion - feat: introduce multisig quorum and weights - feat: add multisig subcommand to client CLI - feat: introduce multisig transaction time-to-live - feat: predefine multisig world-level trigger in genesis - feat: allow accounts in domain to register multisig accounts Signed-off-by: Shunkichi Sato <[email protected]>

mversic · 2024-10-09T02:52:58Z

crates/iroha_cli/Cargo.toml

@@ -30,6 +30,7 @@ path = "src/main.rs"
 iroha = { workspace = true }
 iroha_primitives = { workspace = true }
 iroha_config_base = { workspace = true }
+executor_custom_data_model = { version = "=2.0.0-rc.1.0", path = "../../wasm_samples/executor_custom_data_model" }


it doesn't make sense that this is a dependency of CLI because executor_custom_data_model is intended to only be used in tests

you should create a separate crate for MST data model

This is a temporary workaround. I'm planning to move those *Args for ExecuteTrigger to somewhere under the regular iroha_data_model when iroha introduces custom instructions, then CLI will only depend on iroha.

Or should we pack everything relevant to multisig to one crate for more clear modularity? Related to this, it might be better to move wasm_samples/multisig_* to somewhere as they are not just samples

mversic · 2024-10-09T02:55:37Z

crates/iroha_cli/src/main.rs

+ pub quorum: u16,
+ /// Time-to-live of multisig transactions made by the multisig account
+ #[arg(short, long)]
+ pub transaction_ttl_secs: Option<u32>,


Suggested change

pub transaction_ttl_secs: Option<u32>,

pub transaction_ttl: Duration,

why is it optional? and why not use Duration?

Yeah, instead of being optional, its default value can be explicitly specified in the arg attribute.

u32 because it needs to be parsed from stdin

mversic · 2024-10-09T03:10:35Z

crates/iroha_cli/src/main.rs

+ let register_multisig_account =
+ iroha::data_model::isi::ExecuteTrigger::new(registry_id).with_args(&args);


so the trigger has to already exist before executing this. Who should register it? Shouldn't it first be registered here?

The domains initializer should register the accounts registry on the domain creation. See diagram in review notes

mversic · 2024-10-09T03:11:50Z

crates/iroha_cli/src/main.rs

+ let propose_multisig_transaction =
+ iroha::data_model::isi::ExecuteTrigger::new(registry_id).with_args(&args);


I assume this trigger is registered by the trigger from the Register CLI subcommand

No, the accounts registry should register the transactions registry on the multisig account registration. See diagram in review notes

mversic · 2024-10-09T03:12:44Z

crates/iroha_cli/src/main.rs

+ pub account: AccountId,
+ /// Instructions to approve
+ #[arg(short, long)]
+ pub instructions_hash: iroha::crypto::Hash,


Suggested change

pub instructions_hash: iroha::crypto::Hash,

pub instructions: iroha::crypto::HashOf<Vec<Instruction>>,

Just utilized iroha::crypto::Hash: FromStr. It is immediately converted to HashOf<Vec<InstructionBox>>

mversic · 2024-10-09T03:48:33Z

crates/iroha_genesis/src/lib.rs

@@ -39,12 +41,13 @@ pub struct RawGenesisTransaction {
 chain: ChainId,
 /// Path to the [`Executor`] file
 executor: ExecutorPath,
+ /// Initial topology
+ topology: Vec<PeerId>,


this field should be last as it was before

The idea is no gap with the actual execution order.

Reviewed discussions around #4739 and didn't find reason for the position in RawGenesisTransaction. Is it because topology is supposed to be edited at genesis.json?

mversic · 2024-10-09T03:48:58Z

crates/iroha_genesis/src/lib.rs

 /// Parameters
 #[serde(skip_serializing_if = "Option::is_none")]
 parameters: Option<Parameters>,
 instructions: Vec<InstructionBox>,
- /// Initial topology
- topology: Vec<PeerId>,
+ wasm_triggers: Vec<GenesisWasmTrigger>,


Suggested change

wasm_triggers: Vec<GenesisWasmTrigger>,

triggers: Vec<GenesisWasmTrigger>,

I'd also prefer to put it after instructions so that they can execute triggers but it's just not possible atm because triggers depend on some instructions executing before them

wasm_ prefix implies the executable is supposed to be Wasm, not Instructions.

Yes, ideally it would be in a free position as one of ordinary instructions as before, which would require WasmSmartContract to serialize to a short string and deserialize to a full blob

mversic · 2024-10-09T04:11:13Z

crates/iroha_genesis/src/lib.rs

+#[derive(Debug, Clone, Serialize, Deserialize, IntoSchema, Encode, Decode)]
+pub struct GenesisWasmAction {
+ /// Expected to be converted by [`iroha_test_samples::load_sample_wasm`]
+ executable: String,


Suggested change

executable: String,

executable: PathBuf,

like ExecutorPath

mversic · 2024-10-09T04:17:19Z

crates/iroha_executor/src/default.rs

+ // iroha_test_samples::MULTISIG_SYSTEM_ID
+ "ed01201F677E0900C2F633391310D12D155112DF65EDF9DC800D13797CEE5DAF47B890@system"
+ .parse()
+ .unwrap();


if you want to have hardcoded system domain and account, then they shouldn't be defined in the genesis.json. Rather they should be always implicitly created

Like genesis domain and account, right?

mversic · 2024-10-09T04:24:00Z

wasm_samples/executor_custom_data_model/src/multisig.rs

+ /// Threshold of total weight at which the multisig is considered authenticated
+ pub quorum: u16,
+ /// Multisig transaction time-to-live based on block timestamps. Defaults to [`DEFAULT_MULTISIG_TTL_SECS`]
+ pub transaction_ttl_secs: Option<u32>,


Suggested change

pub transaction_ttl_secs: Option<u32>,

#[serde(default = DEFAULT_MULTISIG_TTL_SECS)]

pub transaction_ttl_ms: u32,

can we do it like this?

we use ms in transaction.rs as unix timestamp

I think there should always be a TTL defined. User an just put a huge number

Ok, we can accept seconds as an user input and convert it to milliseconds here

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 Sep 3, 2024

s8sato added the Enhancement New feature or request label Sep 3, 2024

s8sato force-pushed the feat/4930 branch from d7efb23 to f5790c2 Compare September 3, 2024 00:37

s8sato marked this pull request as ready for review September 3, 2024 00:38

s8sato requested review from mversic, SamHSmith, outoftardis, Erigara, 0x009922, DCNick3 and dima74 as code owners September 3, 2024 00:38

s8sato commented Sep 3, 2024

View reviewed changes

defaults/genesis.json Show resolved Hide resolved

Erigara assigned s8sato and Erigara Sep 3, 2024

s8sato commented Sep 3, 2024

View reviewed changes

s8sato force-pushed the feat/4930 branch from f5790c2 to a409cca Compare September 3, 2024 13:48

s8sato commented Sep 3, 2024

View reviewed changes

client/tests/integration/multisig.rs Outdated Show resolved Hide resolved

Erigara reviewed Sep 3, 2024

View reviewed changes

mversic self-assigned this Sep 4, 2024

Erigara reviewed Sep 4, 2024

View reviewed changes

tools/kagami/src/genesis/generate.rs Outdated Show resolved Hide resolved

wasm_samples/multisig_domains/src/lib.rs Outdated Show resolved Hide resolved

s8sato force-pushed the feat/4930 branch from a409cca to 6d653c1 Compare October 3, 2024 07:32

This was referenced Oct 3, 2024

Ergonomic MST #4930

Open

Deny external access to non-personal accounts #5022

Open

s8sato commented Oct 3, 2024

View reviewed changes

hooks/pre-commit.sample Outdated Show resolved Hide resolved

s8sato force-pushed the feat/4930 branch from 6d653c1 to 076617d Compare October 3, 2024 10:22

s8sato commented Oct 4, 2024

View reviewed changes

defaults/genesis.json Outdated Show resolved Hide resolved

mversic reviewed Oct 5, 2024

View reviewed changes

defaults/genesis.json Outdated Show resolved Hide resolved

Erigara reviewed Oct 8, 2024

View reviewed changes

crates/iroha_data_model/src/transaction.rs Outdated Show resolved Hide resolved

s8sato force-pushed the feat/4930 branch from 076617d to d197e49 Compare October 8, 2024 14:13

s8sato force-pushed the feat/4930 branch 3 times, most recently from 8d4f2f2 to 84c1ec8 Compare October 8, 2024 15:28

s8sato force-pushed the feat/4930 branch from 84c1ec8 to dca696a Compare October 8, 2024 15:58

mversic requested changes Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve multisig utility and usability #5027

feat: improve multisig utility and usability #5027

s8sato commented Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading

nxsaken commented Sep 3, 2024 •

edited

Loading

Erigara left a comment

s8sato commented Oct 3, 2024

s8sato commented Oct 8, 2024 •

edited

Loading

mversic Oct 9, 2024

mversic Oct 9, 2024

s8sato Oct 9, 2024 •

edited

Loading

mversic Oct 9, 2024

s8sato Oct 9, 2024

mversic Oct 9, 2024

s8sato Oct 9, 2024

mversic Oct 9, 2024

s8sato Oct 9, 2024

mversic Oct 9, 2024

s8sato Oct 9, 2024

mversic Oct 9, 2024

s8sato Oct 9, 2024

mversic Oct 9, 2024

mversic Oct 9, 2024

s8sato Oct 9, 2024

mversic Oct 9, 2024

mversic Oct 9, 2024

s8sato Oct 9, 2024

mversic Oct 9, 2024

s8sato Oct 9, 2024

	pub transaction_ttl_secs: Option<u32>,
	pub transaction_ttl: Duration,

		let register_multisig_account =
		iroha::data_model::isi::ExecuteTrigger::new(registry_id).with_args(&args);

		let propose_multisig_transaction =
		iroha::data_model::isi::ExecuteTrigger::new(registry_id).with_args(&args);

	pub instructions_hash: iroha::crypto::Hash,
	pub instructions: iroha::crypto::HashOf<Vec<Instruction>>,

	wasm_triggers: Vec<GenesisWasmTrigger>,
	triggers: Vec<GenesisWasmTrigger>,

	pub transaction_ttl_secs: Option<u32>,
	#[serde(default = DEFAULT_MULTISIG_TTL_SECS)]
	pub transaction_ttl_ms: u32,

feat: improve multisig utility and usability #5027

Are you sure you want to change the base?

feat: improve multisig utility and usability #5027

Conversation

s8sato commented Sep 3, 2024 • edited Loading

Context

Solution

feat: support multisig recursion

feat: introduce multisig quorum and weights

feat: add multisig subcommand to client CLI

feat: introduce multisig transaction time-to-live

feat: predefine multisig world-level trigger in genesis

feat: allow accounts in domain to register multisig accounts

Review notes

Checklist

github-actions bot commented Sep 3, 2024 • edited Loading

nxsaken commented Sep 3, 2024 • edited Loading

Erigara left a comment

Choose a reason for hiding this comment

s8sato commented Oct 3, 2024

s8sato commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s8sato Oct 9, 2024 • 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

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

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

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 Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading

nxsaken commented Sep 3, 2024 •

edited

Loading

s8sato commented Oct 8, 2024 •

edited

Loading

s8sato Oct 9, 2024 •

edited

Loading