Multisignature (rebase) #1741

Fraccaman · 2023-07-20T10:17:00Z

Describe your changes

Took #1606 and rebased on 0.19.0.
Close #1178 and #81

Indicate on which release or other PRs this topic is based on

Based on 0.19.0

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

…added max_signature_per_transaction genesis parameter

…ifiation, rebuild wasm_for_tests

tzemanovic · 2023-07-20T13:48:42Z

core/src/types/account.rs

+    }
+
+    /// Return an empty AccountPublicKeysMap
+    pub fn empty() -> Self {


I think you can derive(Default) for this

tzemanovic · 2023-07-20T13:52:33Z

core/src/ledger/parameters/storage.rs

+    matches!(&key.segments[..], [
+        DbKeySeg::AddressSeg(addr),
+        DbKeySeg::StringSeg(max_signatures_per_transaction),
+    ] if addr == &ADDRESS && max_signatures_per_transaction == Keys::VALUES.max_signatures_per_transaction)


Suggested change

matches!(&key.segments[..], [

DbKeySeg::AddressSeg(addr),

DbKeySeg::StringSeg(max_signatures_per_transaction),

] if addr == &ADDRESS && max_signatures_per_transaction == Keys::VALUES.max_signatures_per_transaction)

is_max_signatures_per_transaction_key_at_addr(key, &ADDRESS)

generated from derive(StorageKeys)

tzemanovic · 2023-07-20T13:53:07Z

core/src/ledger/parameters/storage.rs

+    Key {
+        segments: vec![
+            DbKeySeg::AddressSeg(ADDRESS),
+            DbKeySeg::StringSeg(
+                Keys::VALUES.max_signatures_per_transaction.to_string(),
+            ),
+        ],
+    }


Suggested change

Key {

segments: vec![

DbKeySeg::AddressSeg(ADDRESS),

DbKeySeg::StringSeg(

Keys::VALUES.max_signatures_per_transaction.to_string(),

),

],

}

get_max_signatures_per_transaction_key_at_addr(ADDRESS)

tzemanovic · 2023-07-20T13:57:38Z

core/src/ledger/storage_api/account.rs

+    storage.read(&threshold_key)
+}
+
+/// Get the public keys index map associated with an account


Suggested change

/// Get the public keys index map associated with an account

/// Get the public keys associated with an account

tzemanovic · 2023-07-20T14:01:40Z

core/src/ledger/storage_api/account.rs

+    let vp_key = Key::validity_predicate(owner);
+    storage.has_key(&vp_key)


Suggested change

let vp_key = Key::validity_predicate(owner);

storage.has_key(&vp_key)

match owner {

Address::Established(_) => {

let vp_key = Key::validity_predicate(owner);

storage.has_key(&vp_key)

}

Address::Implicit(_) | Address::Internal(_) => Ok(true),

}

tzemanovic · 2023-07-20T14:07:08Z

core/src/proto/types.rs

+
+impl Ord for SignatureIndex {
+    fn cmp(&self, other: &Self) -> Ordering {
+        self.index.cmp(&other.index)


I think if you put the index field first in the struct then you can derive the impl as it will be sorted by the index (you might need to add Ord to common::Signature which is fine)

I think we can't, because the ed25519_consensus::Signature doesn't implement Ord

ah, I see. I think we can add it, we already have a manual PartialOrd using the bytes:

impl Ord for Signature { fn cmp(&self, other: &Self) -> std::cmp::Ordering { self.0.to_bytes().cmp(&other.0.to_bytes()) } }

tzemanovic · 2023-07-20T14:26:43Z

core/src/proto/types.rs

@@ -722,6 +869,7 @@ impl borsh::BorshSchema for MaspBuilder {
    Serialize,
    Deserialize,
 )]
+#[allow(clippy::large_enum_variant)]


Suggested change

#[allow(clippy::large_enum_variant)]

it looks like this is not needed

tzemanovic · 2023-07-20T14:29:15Z

core/src/proto/types.rs

+        let tx_filename_clone_error = tx_filename.clone();
+        let tx_filename_clone_ok = tx_filename.clone();
+        let out = File::create(path.join(tx_filename)).unwrap();
+        serde_json::to_writer_pretty(out, &self)
+            .map_err(|_| {
+                Error::InvalidJSONDeserialization(tx_filename_clone_error)
+            })
+            .map(|_| path.join(tx_filename_clone_ok))


Suggested change

let tx_filename_clone_error = tx_filename.clone();

let tx_filename_clone_ok = tx_filename.clone();

let out = File::create(path.join(tx_filename)).unwrap();

serde_json::to_writer_pretty(out, &self)

.map_err(|_| {

Error::InvalidJSONDeserialization(tx_filename_clone_error)

})

.map(|_| path.join(tx_filename_clone_ok))

let out = File::create(path.join(&tx_filename)).unwrap();

serde_json::to_writer_pretty(out, &self)

.map_err(|_| Error::InvalidJSONDeserialization(tx_filename.clone()))

.map(|_| path.join(tx_filename))

tzemanovic · 2023-07-20T14:35:50Z

genesis/e2e-tests-single-node.toml

@@ -4,7 +4,7 @@

 genesis_time = "2021-09-30T10:00:00Z"
 native_token = "NAM"
-faucet_pow_difficulty = 1
+faucet_pow_difficulty = 0


do we need to switch it off here?

not at all, but it would speed up e2e test a little

I see - we can do this change, but let's override it to 1 in ledger_txs_and_queries e2e test to still cover this

tzemanovic · 2023-07-20T14:42:02Z

wasm_for_tests/wasm_source/src/lib.rs

-/// A tx that attempts to mint tokens in the transfer's target without debiting
-/// the tokens from the source. This tx is expected to be rejected by the
-/// token's VP.
-#[cfg(feature = "tx_mint_tokens")]


was this unused? Let's also remove it from the Makefile and Cargo.toml

tzemanovic · 2023-07-20T14:45:30Z

.changelog/unreleased/features/1606-multisignature-draft.md

@@ -0,0 +1,2 @@
+- Introduce multisignature account and transaction format.


it would be nice to write a little bit more about this and/or link to docs for anyone who'll want to try it in the new release

we have docs almost ready, should I link them here?

apps/src/lib/config/mod.rs

tzemanovic · 2023-07-20T14:56:06Z

shared/src/ledger/tx.rs

@@ -241,13 +253,29 @@ pub async fn prepare_tx<
    wallet: &mut Wallet<U>,
    args: &args::Tx,
    tx: Tx,
+    owner: Option<Address>,


it looks like this arg is not needed here - it's just returned untouched

this requires a higher refactor of the client methods I was planning to do in the next multisig PR. Should I continue here? maybe I should.

let's please refactor it here so we don't have to add this arg only to remove it later

tzemanovic · 2023-07-20T15:51:06Z

shared/src/ledger/signing.rs

+    client: &C,
+    owner: Option<Address>,
+    public_keys: Vec<common::PublicKey>,
+) -> (AccountPublicKeysMap, u8) {


I think this function is not very clear - the default threshold 0 when there's no owner doesn't seem right, is it? In e.g. init_account that doesn't have an owner beforehand it should respect the tx arg.

I suggest that we change this and use it for all txs except for reveal_pk, init_account and init_validator to only take owner: &Address without the public_keys and fail if the account info cannot be queried.

For the reveal_pk, init_account and init_validator, we can call AccountPublicKeysMap::from_iter(public_keys) directly and take the threshold from the tx arg (with the logic where we set it to 1 when the length of the keys == 1 for new accounts and 1 for reveal_pk)

I think you are right, it would be clearer. I think the approach here should be:

aux_signing_data

prepare_tx

sign

process_tx

Ill push another commit to refactor this whole thing.

Namada 0.20.0 is a minor release addressing several improvements to the PoS system and the ledger stability. * tag 'v0.20.0': (113 commits) Namada 0.20.0 refator e2e masp tests fix tx_transfer ci: fix github colon parsing changelog: add #1693 channgelog: add #1738 [feat]: Moved cli commands common to testing, cli, and sdk out into apps ci: pre-download masp parameters changelog: add #1733 pos/epoched: keep 2 past epochs of data by default changelog: add #1729 test/ledger/finalize_block: fix slashing tests pos/slash: fix the validator state update to be in sync with set changes app/ledger/finalize_block: log block rewards before recording slashes test/e2e/slashing: extend the test to discover rewards issues pos: error out if validator is not in expected state for rewards changelog: add #1717 test/shell/finalize_block: add some txs to DB commit test refactor: rename function refactor: rename method remove test_invalid_sender remove negative check ci: clean up fix formatting don't use flaky sleep refactor creation of namada nodes make Who clonable refactor: use immutable reference refactor: use immutable reference refactor: remove duplicated code test/e2e/slashing: wait for wasm on original validator add changelog fix changes in finalize_block remove vp_token changelog: add #1173 client/rpc: use the new token balance method shared/ledger/queries: add token balance client-only method changelog: add #1692 changelog: add #1670 changelog: add #1667 [fix]: Fixing errors introduced by merging in main [chore]: Incorporating in review comments Update core/src/types/uint.rs Update apps/src/lib/config/genesis.rs [fix]: Fixed the faucet to use uint instead of amount. This makes it portable across different assets converts faucet_withdrawal_limit to be correct changelog: add #1656 pos: return sorted validator sets and code re-use for queries Expanding and fixing slashes query CLI query a validator's state query bonded-stake and order fix client_connections encoding handle errors for unjail-validator tx in the client expand and fix e2e test `double_signing_gets_slashed` add unjail tx at CLI changelog: add #1621 ci/test/e2e: add new test apps: move epoch-sleep client cmd under utils changelog: add #1656 pos: return sorted validator sets and code re-use for queries Expanding and fixing slashes query CLI query a validator's state update pr template test/e2e/proposal_submission: wait for proposal to be committed test/e2e/double_signing: path for validator copy that keeps logs in CI add comments fix according to feedback refactor: fix formatting add changelog refactor: simplify code refactor: introduce name for magic constant add test case feat: store total consensus stake; garbage collect validator sets query bonded-stake and order wait for wasm-precompile before expecting block hash changelog: add #1605 apps: use fd-lock instead of file-lock for cross-platform support changelog: add #1695 deps: update sysinfo to latest 0.29.4 improve error handling for `set_initial_validators` changelog: add #1686 app/ledger/finalize_block: refactor validator set update for re-use app/ledger/init_chain: get genesis validator set using the new fn pos: add a function to get genesis validator consensus set for TM fix a test fix for tx signing for clippy fix multitoken vp to check the minter for clippy modify EthBridge VP and EthBridgePool VP for multitoken revert unexpected changes change eth_bridge balance keys to multitoken keys add unit tests remove InternalAddress::Minted rename fix token decoding fix balance print add multitoken VP file change to multitokens change multitoken ...

Fraccaman added 7 commits July 20, 2023 11:22

core: added multisignature tx format, refacctor account storage api, …

bc39e05

…added max_signature_per_transaction genesis parameter

apps,shared: added cli tx and query methods

9f498bf

docs: update tx definitions

673d161

wasm: added new account methods to tx_prelude, refactor signature ver…

a56ae26

…ifiation, rebuild wasm_for_tests

test: fix unit tests

60fc86c

test: fix e2e test

85c7559

added changelog

39113c6

Fraccaman mentioned this pull request Jul 20, 2023

Alternative draft + multisig #1742

Closed

tzemanovic reviewed Jul 20, 2023

View reviewed changes

apps/src/lib/config/mod.rs Show resolved Hide resolved

tzemanovic reviewed Jul 20, 2023

View reviewed changes

Fraccaman added 5 commits July 25, 2023 16:22

client: refactor client methods

d5c8251

fixup! client: refactor client methods

40603dc

cli: fix custom tx

b962646

e2e: fix custom tx tests, added multisig tests

78cbaaa

Fraccaman mentioned this pull request Jul 26, 2023

Multisignature (rebase 0.20.0) #1765

Closed

2 tasks

Fraccaman closed this Jul 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multisignature (rebase) #1741

Multisignature (rebase) #1741

Fraccaman commented Jul 20, 2023 •

edited

Loading

tzemanovic Jul 20, 2023

tzemanovic Jul 20, 2023

tzemanovic Jul 20, 2023

tzemanovic Jul 20, 2023

tzemanovic Jul 20, 2023 •

edited

Loading

tzemanovic Jul 20, 2023

Fraccaman Jul 20, 2023

tzemanovic Jul 24, 2023

tzemanovic Jul 20, 2023

tzemanovic Jul 20, 2023

tzemanovic Jul 20, 2023

Fraccaman Jul 20, 2023

tzemanovic Jul 24, 2023

tzemanovic Jul 20, 2023

Fraccaman Jul 20, 2023

tzemanovic Jul 20, 2023

Fraccaman Jul 20, 2023

tzemanovic Jul 24, 2023

tzemanovic Jul 20, 2023

Fraccaman Jul 20, 2023

tzemanovic Jul 24, 2023

tzemanovic Jul 20, 2023

Fraccaman Jul 20, 2023 •

edited

Loading

tzemanovic Jul 24, 2023

	/// Get the public keys index map associated with an account
	/// Get the public keys associated with an account

		let vp_key = Key::validity_predicate(owner);
		storage.has_key(&vp_key)

		@@ -0,0 +1,2 @@
		- Introduce multisignature account and transaction format.

Multisignature (rebase) #1741

Multisignature (rebase) #1741

Conversation

Fraccaman commented Jul 20, 2023 • edited Loading

Describe your changes

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

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

tzemanovic Jul 20, 2023 • 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

Fraccaman Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fraccaman commented Jul 20, 2023 •

edited

Loading

Checklist before merging to `draft`

tzemanovic Jul 20, 2023 •

edited

Loading

Fraccaman Jul 20, 2023 •

edited

Loading