Murisi/integrate hw masp #3797

murisi · 2024-09-10T10:44:54Z

Describe your changes

Made the following changes to enable hardware wallet signing of MASP Transactions:

Allow the outsourcing of generator randomness to the hardware wallet
Allow the generation of MASP extended full viewing keys using the hardware wallet
Allow the client to generate proofs using proof generation keys from the hardware wallet
Now apply signatures generated by the hardware wallet onto client build Transactions
The following transaction types are supported by MASP hardware wallet signing:
- Shielding transactions (only randomness parameters are taken from hardware wallet)
- Shielded, unshielding, and IBC transactions (randomness parameters and proof generation keys are taken from hardware wallet)
- Combined hardware wallet and software client signing where the the shielded keys are on the hardware wallet and wrapper signer keys are on the computer, or vice-versa, or both reside on the device is supported
Adjusted the hardware and localnet genesis balances files in order to ensure that the MASP inflation controller computations are the same for both kinds of tests
Recomputed/recalibrated the MASP test numerical expectations to work with the new adjusted balances files
Modified the MASP test commands to enable signing to happen on the hardware wallet when NAMADA_E2E_USE_DEVICE=true
Moved the MASP spending and viewing keys as well as payment addresses used in the the tests crate into the localnet and hardware genesis files to enable hardware wallet signing
Separated spending and viewing keys from their birthdays in the wallet's store.toml in order to ease the manual editing of these files (as done above)
Removed the possibility of using the hardware wallet to sign the wrapper header without signing the raw header because the former depends on the latter in the current design
Now ensure the removal of the Builder section in sign_tx because it contains private information and because it will be rejected on the basis of not being referred to in the wrapper signature section

This PR depends upon the following being merged:

Murisi/box impl build params masp#88 to reduce the constraints induced by the Builder API
Murisi/zip32 support Zondax/ledger-namada#61 to generate extended full viewing keys.
Fix signing for non-MASP IBC Transfers ledger-namada#2 to fix the signing of pure IBC Txs

Outstanding issues:

The hardware wallet applies ZIP 32 derivations to a seed obtained from doing SLIP 10 derivation with a fixed path. (This was necessary because the hardware wallet's ZIP 32 functions are unable to directly access the root seed.) This PR currently modifies namadaw's key derivation to match the hardware wallet's to ease checking that the same extended keys are derived for a given mnemonic. However, it might be desirable to revert namadaw's derivation to pure ZIP 32 if we desire interoperability with systems implementing this standard strictly. Or perhaps both types of ZIP 32 could be supported with a namadaw CLI flag to choose which derivation is desired. See a relevant discusssion at https://zecsec.com/audits/zcash-ledger-audit-report-v2.pdf#subsection.3.2 .

Checklist before merging

If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
- This will require 2 reviewers to approve the changes

Fraccaman · 2024-09-18T13:02:00Z

@murisi thanks for PR! Do u mind rebasing this on main?

codecov · 2024-09-23T10:53:46Z

Codecov Report

Attention: Patch coverage is 49.43820% with 135 lines in your changes missing coverage. Please review.

Project coverage is 74.49%. Comparing base (cc531ec) to head (79106c5).
Report is 106 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/wallet/src/store.rs	25.00%	60 Missing ⚠️
crates/wallet/src/lib.rs	35.55%	29 Missing ⚠️
crates/wallet/src/keys.rs	32.14%	19 Missing ⚠️
crates/sdk/src/tx.rs	73.91%	6 Missing ⚠️
crates/apps_lib/src/wallet/defaults.rs	0.00%	4 Missing ⚠️
crates/sdk/src/lib.rs	0.00%	4 Missing ⚠️
crates/core/src/masp.rs	86.95%	3 Missing ⚠️
crates/tx/src/types.rs	84.21%	3 Missing ⚠️
crates/apps_lib/src/config/genesis/transactions.rs	0.00%	2 Missing ⚠️
crates/apps_lib/src/config/genesis/utils.rs	0.00%	2 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3797      +/-   ##
==========================================
- Coverage   74.58%   74.49%   -0.09%     
==========================================
  Files         341      341              
  Lines      107484   107642     +158     
==========================================
+ Hits        80167    80190      +23     
- Misses      27317    27452     +135

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tzemanovic · 2024-09-26T10:33:19Z

I'm trying to run e2e::ledger_tests::masp_txs_and_queries with these changes and 713f954:

Something is wrong with the wrapper signature. It fails on shield --source Albert --target znam1ky620tz7z658cralqt693qpvk42wvth468zp38nqvq2apmex5rfut3dfqm2asrsqv0tc7saqje7 --token BTC --amount 20 --use-device with "Mempool validation failed: WrapperTx signature verification failed: The wrapper signature is invalid". Same thing if I try --dry-run-wrapper.

Relatedly, do you recall why we disallow --use-device with --dry-run? We allow it with --dry-run-wrapper. (it was added in #3570)

murisi · 2024-09-26T10:52:46Z

Thanks for looking into this PR.

Relatedly, do you recall why we disallow --use-device with --dry-run? We allow it with --dry-run-wrapper. (it was added in #3570)

I believe that combining --use_device with --dry-run is disallowed because the hardware wallet does not support displaying and signing raw transactions. And I think (at least at the time when the code was written) --dry-run causes the creation of raw transactions instead of wrapper transactions.

Something is wrong with the wrapper signature. It fails on shield --source Albert --target znam1ky620tz7z658cralqt693qpvk42wvth468zp38nqvq2apmex5rfut3dfqm2asrsqv0tc7saqje7 --token BTC --amount 20 --use-device with "Mempool validation failed: WrapperTx signature verification failed: The wrapper signature is invalid". Same thing if I try --dry-run-wrapper.

If the hardware wallet produced the wrapper signature for a Tx containing a MASP Transaction, then the issue might be due to the hardware wallet's reponse. But I need to check. I'd actually been planning to add a last commit to this PR to integrate MASP hardware wallet testing with the E2E and integration tests. This will be done by Monday evening.

murisi · 2024-10-01T13:52:46Z

I'm trying to run e2e::ledger_tests::masp_txs_and_queries with these changes and 713f954:

Hi @tzemanovic . All the MASP tests now work (when using a variant of the Ledger app with the above PRs merged in) except the integration::masp::masp_fee_payment ones (failure probably due to simultaneous construction of Transactions requiring the hardware wallet) and integration::masp::expired_masp_tx (failure probably due to the time taken to press "approve" on the hardware wallet interferes with the logic of the test). Please try and run them again when you get the chance.

tzemanovic · 2024-10-03T12:38:05Z

crates/wallet/src/store.rs

@@ -63,10 +62,12 @@ pub struct ValidatorData {
 /// A Storage area for keys and addresses
 #[derive(Serialize, Deserialize, Debug, Default)]
 pub struct Store {


These changes in the wallet store are breaking, but I think we could make them compatible by e.g. trying to deserialize to the current format in case of a failure and migrate to the new format

Thanks, good point. I've written code to deserialise the current format and migrate it to the new format. I had to slightly modify the new format to support encryptions of data in the old format (which do not seem to be easily migratable).

tzemanovic · 2024-10-03T12:57:32Z

I'm trying to run e2e::ledger_tests::masp_txs_and_queries with these changes and 713f954:

Hi @tzemanovic . All the MASP tests now work...

Everything works well, thanks! But there are some affected unrelated integration tests and it looks like there's maybe a breaking change for IBC/hermes.

An observation from UX perspective - a shielded transfer and unshielding transfer require multiple wallet interactions, first to access a viewing key and then 2 more to sign the transfer(s) and again to obtain the signature from the wallet. The last 2 interactions print the same information on the wallet screen, so it appears that it's repeating the same action. I'm not sure if we can do much about that in the wallet, but I think we could at least print something out in the CLI before the request to make it clear what's happening.

I'll also add emulator automation for one of the masp tests and we'll need to update the elf file we use in the CI. For local testing I just merged together all the open PRs in ledger-namada. Am I okay to use that?

tzemanovic · 2024-10-04T10:15:19Z

One more thing - during a shielded and unshielding transfer when the amount being transferred is lower than the balance of the source, the change that stays in the sender's balance is shown as an obscured receiver payment address so it's not apparent that the destination is the same as the sender. Is there something we can do to improve this?

murisi · 2024-10-30T10:58:01Z

Everything works well, thanks! But there are some affected unrelated integration tests and it looks like there's maybe a breaking change for IBC/hermes.

You're right. I just checked, and the root causes of this were the following:

This PR changes the spending keys, viewing keys, and payment addresses used in the IBC tests from raw bech32m literals to aliases of things in the wallet. Unfortunately some of the IBC tests were still checking whether the aliases began with zvk and zsk. Furthermore literals need to be specified as receivers for IBC transactions. Both of these are now fixed.
The second cause was my breaking changes to the Store (that you mentioned above). This PR will need to be fixed to achieve backwards compatibility with the old format (as you mentioned), but regardless Hermes will need its namada_sdk dependency updated to support the new representation I think...

…et wallet.

…eters cannot be reused. Fixed MASP integration tests depending on access to secret keys.

murisi · 2024-11-27T17:10:10Z

One more thing - during a shielded and unshielding transfer when the amount being transferred is lower than the balance of the source, the change that stays in the sender's balance is shown as an obscured receiver payment address so it's not apparent that the destination is the same as the sender. Is there something we can do to improve this?

This is a very good point. I think we can open an issue at https://github.com/Zondax/ledger-namada to propose that the Namada Ledger app checks whether Sapling outputs are going to the default payment address of the signer, and if so annotates the output with an indication that it is actually change.

Done that here: https://github.com/Zondax/ledger-namada-zip32/issues/7 . Thanks again for the suggestion.

…sis files.

* for anoma/namada#3797 * fix unbonding period * use pos_params.withdrawable_epoch_offset * just unbonding_len * Namada v1.1.0 * update Namada url for CI * ubuntu 24 for Namada binaries * fix for trusting period

* Update CHANGELOG.md * Create unclog release * Bump versions * Update Cargo.lock * Revert "Add support for Namada (#3705)" This reverts commit 00a0cf5. * Cargo clippy fix * Add support for Namada (#3705) Add support for relaying between Namada and Cosmos-base chains. - Add Namada to `ChainConfig` - Reuse `CosmosSdkConfig` for generating Tendermint light clients - Add `NamadaChain` as `ChainEndpoint` implementation - See `crates/relayer/src/chain/namada.rs` and files under `crates/relayer/src/chain/namada` - To submit Namada transactions and query - Store Namada key to KeyRing --------- Signed-off-by: Yuji Ito <[email protected]> Co-authored-by: Luca Joss <[email protected]> Co-authored-by: Luca Joss <[email protected]> Co-authored-by: Gianmarco Fraccaroli <[email protected]> Co-authored-by: Tomáš Zemanovič <[email protected]> Co-authored-by: Romain Ruetschi <[email protected]> * Update release to v1.10.5 * Add support for Namada (#3705) Add support for relaying between Namada and Cosmos-base chains. - Add Namada to `ChainConfig` - Reuse `CosmosSdkConfig` for generating Tendermint light clients - Add `NamadaChain` as `ChainEndpoint` implementation - See `crates/relayer/src/chain/namada.rs` and files under `crates/relayer/src/chain/namada` - To submit Namada transactions and query - Store Namada key to KeyRing --------- Signed-off-by: Yuji Ito <[email protected]> Co-authored-by: Luca Joss <[email protected]> Co-authored-by: Luca Joss <[email protected]> Co-authored-by: Gianmarco Fraccaroli <[email protected]> Co-authored-by: Tomáš Zemanovič <[email protected]> Co-authored-by: Romain Ruetschi <[email protected]> * Spellcheck * fix clippy * Update nix flake * Fix query balance command to be compatible with Osmosis v27 * Fix command output parsing for Osmosis * Set explicit timeout to 4 hours for Namada to Namada CI job * Add memory swap and reduce CPU overload for Namada<->Namada CI job * Add jlumbroso/free-disk-space@main step for Namada CI job * to Namada v1.1.0 (#4293) * for anoma/namada#3797 * fix unbonding period * use pos_params.withdrawable_epoch_offset * just unbonding_len * Namada v1.1.0 * update Namada url for CI * ubuntu 24 for Namada binaries * fix for trusting period * Use Namada SDK release from crates.io * Update nix flake * Update CI with Namada v1.1.1 (#4295) * update CI for Namada v1.1.1 * update error message --------- Signed-off-by: Yuji Ito <[email protected]> Co-authored-by: Yuji Ito <[email protected]> Co-authored-by: Gianmarco Fraccaroli <[email protected]> Co-authored-by: Tomáš Zemanovič <[email protected]> Co-authored-by: Romain Ruetschi <[email protected]>

murisi force-pushed the murisi/integrate-hw-masp branch from 2e32593 to 8fac040 Compare September 10, 2024 10:48

murisi marked this pull request as draft September 10, 2024 10:48

murisi force-pushed the murisi/integrate-hw-masp branch 2 times, most recently from 3f79328 to bad72be Compare September 14, 2024 14:56

murisi force-pushed the murisi/integrate-hw-masp branch 3 times, most recently from cd0decd to 3fe7ee9 Compare September 23, 2024 10:29

murisi force-pushed the murisi/integrate-hw-masp branch from 216861c to e448c1a Compare September 24, 2024 19:44

murisi requested review from cwgoes, karbyshev and tzemanovic September 24, 2024 19:45

murisi force-pushed the murisi/integrate-hw-masp branch from e448c1a to e08c8b7 Compare September 24, 2024 21:26

murisi added MASP externally blocked labels Sep 26, 2024

murisi marked this pull request as ready for review September 26, 2024 10:58

murisi force-pushed the murisi/integrate-hw-masp branch 2 times, most recently from 0332a63 to a375e6e Compare September 28, 2024 08:16

tzemanovic reviewed Oct 3, 2024

View reviewed changes

This was referenced Oct 4, 2024

hw masp auto test #3890

Open

Avoid redundant wrapper signatures #3896

Closed

murisi force-pushed the murisi/integrate-hw-masp branch from 69704a0 to 7748a05 Compare October 29, 2024 13:23

murisi mentioned this pull request Oct 30, 2024

Fix signing for non-MASP IBC Transfers anoma/ledger-namada#2

Open

murisi and others added 4 commits November 27, 2024 12:20

Moved the shielded keys used in the integration tests into the localn…

0ef7b66

…et wallet.

Dont do a dry run if using a device in the MASP integration tests.

2d8c41c

Always sign MASP Transactions before dumping because randomness param…

3e72fe1

…eters cannot be reused. Fixed MASP integration tests depending on access to secret keys.

test/e2e/masp: add support for testing with HW wallet

fd48de2

jurevans mentioned this pull request Nov 27, 2024

SDK: Finalize v0.14.0 Release anoma/namada-interface#1324

Open

murisi added 2 commits November 27, 2024 13:15

Sometimes use literals instead of aliases in the IBC tests.

bdd8e9f

Make shielded balance checking more robust for IBC tests.

706a143

murisi force-pushed the murisi/integrate-hw-masp branch 2 times, most recently from e4db3ce to 24fcb3b Compare November 27, 2024 12:55

murisi force-pushed the murisi/integrate-hw-masp branch from 1f49cad to 4be78ff Compare December 1, 2024 15:15

murisi added 2 commits December 2, 2024 09:05

Adjust integration tests to work with both hardware and localnet gene…

e59aaf6

…sis files.

Make the the software wallet support the old Store format.

2105731

murisi force-pushed the murisi/integrate-hw-masp branch from d5ec056 to 829e292 Compare December 2, 2024 07:06

murisi mentioned this pull request Dec 2, 2024

Updated Namada SDK to the one provided by the hardware integration branch heliaxdev/hermes#43

Closed

murisi force-pushed the murisi/integrate-hw-masp branch from 829e292 to ba5dbbb Compare December 2, 2024 07:31

Test that the transfer source now displays as a viewing key.

79106c5

murisi force-pushed the murisi/integrate-hw-masp branch from ba5dbbb to 79106c5 Compare December 2, 2024 07:42

murisi mentioned this pull request Dec 2, 2024

Murisi/masp for namadillo #4127

Merged

3 tasks

yito88 added a commit to heliaxdev/hermes that referenced this pull request Dec 2, 2024

for anoma/namada#3797

4c0309d

mergify bot merged commit 79106c5 into main Dec 5, 2024
14 of 22 checks passed

mergify bot deleted the murisi/integrate-hw-masp branch December 5, 2024 08:59

yito88 added a commit to heliaxdev/hermes that referenced this pull request Dec 5, 2024

for anoma/namada#3797

29fab66

mergify bot mentioned this pull request Dec 11, 2024

Murisi/masp for namadillo (backport #4127) #4167

Closed

3 tasks

This was referenced Jan 6, 2025

Murisi/re enable masp hw support #4219

Merged

Murisi/fix hw tests by unbatching #4228

Merged

yito88 added a commit to heliaxdev/hermes that referenced this pull request Jan 31, 2025

for anoma/namada#3797

1001945

yito88 added a commit to heliaxdev/hermes that referenced this pull request Feb 3, 2025

for anoma/namada#3797

0b6b66b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Murisi/integrate hw masp #3797

Murisi/integrate hw masp #3797

murisi commented Sep 10, 2024 •

edited

Loading

Fraccaman commented Sep 18, 2024

codecov bot commented Sep 23, 2024 •

edited

Loading

tzemanovic commented Sep 26, 2024

murisi commented Sep 26, 2024

murisi commented Oct 1, 2024 •

edited

Loading

tzemanovic Oct 3, 2024

murisi Dec 2, 2024

tzemanovic commented Oct 3, 2024

tzemanovic commented Oct 4, 2024

murisi commented Oct 30, 2024

murisi commented Nov 27, 2024

Murisi/integrate hw masp #3797

Murisi/integrate hw masp #3797

Conversation

murisi commented Sep 10, 2024 • edited Loading

Describe your changes

Checklist before merging

Fraccaman commented Sep 18, 2024

codecov bot commented Sep 23, 2024 • edited Loading

Codecov Report

tzemanovic commented Sep 26, 2024

murisi commented Sep 26, 2024

murisi commented Oct 1, 2024 • edited Loading

tzemanovic Oct 3, 2024

Choose a reason for hiding this comment

murisi Dec 2, 2024

Choose a reason for hiding this comment

tzemanovic commented Oct 3, 2024

tzemanovic commented Oct 4, 2024

murisi commented Oct 30, 2024

murisi commented Nov 27, 2024

murisi commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 23, 2024 •

edited

Loading

murisi commented Oct 1, 2024 •

edited

Loading