Multisignature (rebase 0.20.0) #1765

Fraccaman · 2023-07-26T10:36:15Z

Describe your changes

Took #1741 and rebased on 0.20.0. Cleaned git history.
Close #1178 and #81

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

Based on 0.20.0

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

…added max_signature_per_transaction genesis parameter

…ification, rebuild wasm_for_tests

* origin/fraccaman/multisignature-draft-rebase-0.20.0: added changelog fix: e2e test fix: unit test wasm: added new account methods to tx_prelude, refactor signature verification, rebuild wasm_for_tests docs: update tx definitions apps,shared: added cli tx and query methods core: added multisignature tx format, refacctor account storage api, added max_signature_per_transaction genesis parameter

…1765)

murisi

Hi @Fraccaman ! The multi-signature component of this PR seems to be fine. However the SDK API changes pertaining to transaction building and signing will most likely break the web client's ability to sign transactions using the hardware wallet. Apologies for only bringing this up now, but actually trying to generate, inspect, and integrate the test vectors into the hardware wallet repository drew my attention to these issues.

Having the following transaction flow: build Tx -> generate test vectors -> sign Tx -> protocol filter, should eliminate/minimize incompatibilities with the hardware wallet/web client. But other approaches could also be valid.

@adrianbrink The web client and hardware wallet work fine with Namada v0.20.1 . But releasing #1772 (which contains this PR) as-is will break the web client's ability to use this SDK with the hardware wallet. We can either fix these issues now or release this (accepting a breakage) and do a follow-up release attempting to fix these issues.

murisi · 2023-08-07T06:51:43Z

apps/src/lib/client/tx.rs

+    if args.tx.dump_tx {
+        tx::dump_tx(&args.tx, tx_builder);
+    } else {
+        submit_reveal_aux(client, ctx, args.tx.clone(), &args.owner).await?;


This call to submit_reveal_aux is likely to invalidate the proof of work computed in the above build_custom call. Indeed, trying to do a Transfer from an implicit account for the first time fails with the error message Mempool validation failed: The given address does not have a sufficient balance to pay fee. This issue likely affects all the other submit_* functions that call submit_reveal_aux.

This seems to be fixed in your https://github.com/anoma/namada/tree/fraccaman/multisignature-fixes branch.

murisi · 2023-08-07T06:57:20Z

shared/src/ledger/tx.rs

@@ -291,10 +314,8 @@ pub async fn process_tx<
    client: &C,
    wallet: &mut Wallet<U>,
    args: &args::Tx,
-    mut tx: Tx,
+    tx_builder: TxBuilder,


The hardware wallet takes an unsigned Tx and produces a signed Tx object. However, the SDK function tx::process_tx now takes a TxBuilder (instead of a signed Tx). Hence this change could be problematic for the web client which uses the hardware wallet for signing.

This issue seems to be fixed in https://github.com/anoma/namada/tree/fraccaman/multisignature-fixes .

murisi · 2023-08-07T07:04:37Z

shared/src/types/tx.rs

+            tx.add_section(section);
+        }
+
+        tx.protocol_filter();


This protocol_filter call removes MaspBuilder sections from the transaction. But having this call in this function is problematic because this build function needs to return a Tx with MaspBuilder sections inside in order to give the hardware wallet a chance to validate the MaspTx section of the transaction being constructed.

murisi · 2023-08-07T07:10:52Z

shared/src/types/tx.rs

+                    _ => None,
+                })
+                .collect();
+            tx.add_section(Section::SectionSignature(MultiSignature::new(


The transaction signing correctly happens after the Tx::protocol_filter call which is good because the MaspBuilder section will not be sent to the protocol. However, if the Tx::protocol_filter function were to be moved outside this function, this signing call would need to follow it out to ensure that it still happens temporally after the filtering is done.

murisi · 2023-08-07T07:12:09Z

shared/src/types/tx.rs

+                .map(|section| section.get_hash())
+                .collect::<Vec<Hash>>();
+            sections_hashes.push(tx.header_hash());
+            tx.add_section(Section::Signature(Signature::new(


Comments analogous to those for MultiSignature should also apply here.

murisi · 2023-08-07T07:16:47Z

shared/src/ledger/tx.rs

+
+    #[cfg(feature = "std")]
+    {
+        super::signing::generate_test_vector(client, wallet, &tx).await;


The test vectors need to be constructed while the MaspBuilder section is still inside the Tx because those sections contain the information necessary for a hardware wallet to sign a MASP transaction. More precisely, this call needs to happen before protocol_filter is called (which happens to be inside the let tx = tx_builder.build(); call right now).

murisi · 2023-08-07T07:29:26Z

apps/src/lib/client/tx.rs

+        let tx_builder = signing::sign_tx(
+            &mut ctx.wallet,
+            &args.tx,
+            tx_builder,


It probably is possible to have signing::sign_tx work over TxBuilders. However, doing this will probably cause an incongruence/divergence with web client's signing logic (which is constrained by the hardware wallet's implementation). Specifically, the web client needs to build a (unsigned) Tx first which is the object that is signed by hardware wallet. (In contrast, the approach taken in this PR has the build coming after the signing.) Keeping the same signing flow would probably ease the parallel development of the CLI and the web client in the context of hardware wallets.

murisi · 2023-08-07T07:39:06Z

apps/src/lib/cli.rs

@@ -57,7 +57,7 @@ pub mod cmds {
        TxCustom(TxCustom),
        TxTransfer(TxTransfer),
        TxIbcTransfer(TxIbcTransfer),
-        TxUpdateVp(TxUpdateVp),
+        TxUpdateAccount(TxUpdateAccount),


Running the scripts/generator.sh client script (which is necessary for the hardware wallet test vector generation) throws the error The application panicked (crashed). Command add-erc20-transfer: Argument names must be unique, but 'fee-payer' is in use by more than one argument or group on the first 23 commands. This suggests a slight issue with the CLI argument configuration.

This seems to be fixed in your https://github.com/anoma/namada/tree/fraccaman/multisignature-fixes branch.

adrianbrink · 2023-08-09T14:49:03Z

How much

Hi @Fraccaman ! The multi-signature component of this PR seems to be fine. However the SDK API changes pertaining to transaction building and signing will most likely break the web client's ability to sign transactions using the hardware wallet. Apologies for only bringing this up now, but actually trying to generate, inspect, and integrate the test vectors into the hardware wallet repository drew my attention to these issues.

Having the following transaction flow: build Tx -> generate test vectors -> sign Tx -> protocol filter, should eliminate/minimize incompatibilities with the hardware wallet/web client. But other approaches could also be valid.

@adrianbrink The web client and hardware wallet work fine with Namada v0.20.1 . But releasing #1772 (which contains this PR) as-is will break the web client's ability to use this SDK with the hardware wallet. We can either fix these issues now or release this (accepting a breakage) and do a follow-up release attempting to fix these issues.

How much work is involved in fixing it so that it works with the sdk? In general I'm in favor of not breaking the sdk.

murisi · 2023-08-09T15:29:13Z

@adrianbrink I believe #1794 should fix it. That PR is currently undergoing review.

Fraccaman added 6 commits July 26, 2023 12:25

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

37f5e88

…added max_signature_per_transaction genesis parameter

apps,shared: added cli tx and query methods

2d1b22d

docs: update tx definitions

7fd8e40

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

49f6613

…ification, rebuild wasm_for_tests

fix: unit test

1661995

fix: e2e test

f9105ad

Fraccaman requested a review from tzemanovic July 26, 2023 10:36

added changelog

4f7c639

Fraccaman added a commit that referenced this pull request Jul 26, 2023

fixup! Merge branch 'fraccaman/multisignature-draft-rebase-0.20.0' (#…

70b0152

…1765)

tzemanovic approved these changes Jul 27, 2023

View reviewed changes

Fraccaman mentioned this pull request Jul 27, 2023

Draft 0.21.0 #1772

Closed

Fraccaman closed this Jul 29, 2023

murisi reviewed Aug 7, 2023

View reviewed changes

murisi mentioned this pull request Aug 9, 2023

Murisi/multisig reordering #1794

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multisignature (rebase 0.20.0) #1765

Multisignature (rebase 0.20.0) #1765

Fraccaman commented Jul 26, 2023 •

edited

Loading

murisi left a comment

murisi Aug 7, 2023

murisi Aug 7, 2023

murisi Aug 7, 2023

murisi Aug 7, 2023

murisi Aug 7, 2023

murisi Aug 7, 2023

murisi Aug 7, 2023

murisi Aug 7, 2023

adrianbrink commented Aug 9, 2023

murisi commented Aug 9, 2023

Multisignature (rebase 0.20.0) #1765

Multisignature (rebase 0.20.0) #1765

Conversation

Fraccaman commented Jul 26, 2023 • edited Loading

Describe your changes

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

Checklist before merging to draft

murisi left a 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

adrianbrink commented Aug 9, 2023

murisi commented Aug 9, 2023

Fraccaman commented Jul 26, 2023 •

edited

Loading

Checklist before merging to `draft`