[r2r] Global HD account #1512

sergeyboyko0791 · 2022-10-27T17:50:34Z

No description provided.

…key*`

* TODO: remove `MmCtx::secp256k1_key_pair_as_option()` and ``MmCtx::secp256k1_key_pair`

* Add `Secp256k1Secret` and `IguanaPrivKey` aliases, use it everywhere instead of the `[u8]` slice

* Add common `InternalError` * Add `DiscardMmResult`, `SplitMmResult` traits to `mm2_error_handle` * Move `MmCtx::public_id()` to `CryptoCtx::mm2_internal_public_id()`

* Add `hd_account_id` conf param * Cover swaps in tests

* Rename `DerivationMethod::Iguana` to `DerivationMethod::SingleAddress` * Rename `PrivKeyNotAllowed` to `PrivKeyPolicyNotAllowed`

*

* Use `NegotiationDataMsg::V3` even if maker and taker pubkeys are the same but differ from persistent pub * Log Swap error event in WASM

sergeyboyko0791 · 2022-11-01T11:21:10Z

mm2src/coins/lightning/ln_utils.rs

+pub fn init_keys_manager(platform: &Platform) -> EnableLightningResult<Arc<KeysManager>> {
    // The current time is used to derive random numbers from the seed where required, to ensure all random generation is unique across restarts.
-    let seed: [u8; 32] = ctx.secp256k1_key_pair().private().secret.into();
+    // TODO validate that this is right
+    let seed: [u8; 32] = platform
+        .coin
+        .as_ref()
+        .priv_key_policy
+        .key_pair_or_err()?
+        .private()
+        .secret
+        .into();


@shamardy could you please confirm that we can use the platform's key-pair to initialize the KeysManager?

My understanding is that if mm2 is started with a global hd account the key pair here will be the one corresponding to a valid child address so I don't think this will be a problem. Now, this private key will be used as seed in KeysManager to generate a new master key that should generate other multiple child keys (a total of six) for node public/private key, closing channels address, and some of these child keys (e.g. channel_master_key) are used as master keys again :) You can check the KeysManager::new function internals for the exact details https://github.com/KomodoPlatform/atomicDEX-API/blob/5627cded1616e34c67b795023514a2a86b4c3bba/mm2src/coins/lightning/ln_utils.rs#L95
We have two choices here, either have one lightning node per user that uses the below master seed https://github.com/KomodoPlatform/atomicDEX-API/blob/5627cded1616e34c67b795023514a2a86b4c3bba/mm2src/crypto/src/privkey.rs#L121-L122 as it's KeysManager seed, or to give the user the ability to have a lightning wallet/node for every child address using the current implementation in this PR. I think we would need @artemii235 input on which of these options is prefered.

@shamardy Will this effect from which address the funding transaction will be sent and to which address closed channel funds will be claimed?

You are right. For every key pair there will be a different funding/claiming address (funding and claiming addresses are one and the same address of the platform coin), but currently lightning can't be enabled in HDWallet mode, I even added a todo for this in the past, I thought then that HDWallet meant hardware :) https://github.com/KomodoPlatform/atomicDEX-API/blob/5627cded1616e34c67b795023514a2a86b4c3bba/mm2src/coins/lightning.rs#L1178-L1184
https://github.com/KomodoPlatform/atomicDEX-API/blob/5627cded1616e34c67b795023514a2a86b4c3bba/mm2src/coins/lightning/ln_events.rs#L528-L535
https://github.com/KomodoPlatform/atomicDEX-API/blob/5627cded1616e34c67b795023514a2a86b4c3bba/mm2src/coins/rpc_command/lightning/open_channel.rs#L148
If we agreed to have a lightning address/node for every child address, I should add HDWallet implementation for lightning to this checklist #1045 (comment)

One more note, we can always implement our own lightning KeysManager if we needed that. The only requirement for KeysManager is to implement the KeysInterface trait. But I think the one implemented in rust-lightning is very good for preserving privacy, we break this privacy in our lightning implementation when claiming the closing funds back to the same address the channel was opened from. I guess to continue the preserving of privacy with HDWallets we can let the user decide which child address to generate funding from and which address to claim the closing funds to for every channel, if we choose to use only one lightning node for all platform coin addresses then we would need to implement our own lightning KeysManager where there is one node address using the master seed and change the destination script, the shutdown pubkey when switching addresses.

HDWallet is related to Hardware wallet only.
Global HD account is implemented the way that if the user wants to switch its Global HD account, he needs to restart mm2 instance with another conf.hd_account_id index.
So for a running mm2 instance with an conf.hd_account_id value there is only one address (therefore one key-pair) for any coin.

mmm, I understand now but I think we should change DerivationMethod::HDWallet naming to convey that it's for hardware wallets only to avoid confusion.
So, in light of this info, for every conf.hd_account_id there will be a different lightning address/node with different channels backups etc..., I still see no problem caused in lightning by the current implementation :)

For the privacy issues I detailed here #1512 (comment), there will be a different destination script and shutdown pubkey also for every hd_account_id, I guess we can add an option for the user to claim the closing transaction of a channel to a different hd_account_id or a different address of his choosing making lightning channels opening/closing completely private. Of Course this is not a priority now, but just adding my thoughts about HD accounts implications for lightning.

Ok thank you! DerivationMethod::HDWallet is really a confusing name, because it overlaps with Global HD account. But they are both related to HD wallet:

Global HD account: one coin -> one address.

DerivationMethod::HDWallet: one coin -> multiple accounts, account -> multiple addresses (internals and/or externals).

I explained it in more details here: https://github.com/KomodoPlatform/air_dex/issues/502#issuecomment-1193167304

DerivationMethod::HDWallet is currently used with Trezor only, but it's designed the way that it's independent from the private key policy. Thus later we can add new PrivKeyPolicy::CoinHDAccount, for example. The only difference from PrivKeyPolicy::CoinHDAccount will be how transactions are signed.

If we're sure that we don't plan to support such derivation in the future, I can rename DerivationMethod::HDWallet to DerivationMethod::HardwareWallet.
@artemii235 could you please advice?

If we're sure that we don't plan to support such derivation in the future

We can't 100% sure, I guess 🙂 Though, I would prefer the naming to reflect the current meaning of the code. We can figure out one more renaming later if required.

mm2src/mm2_main/src/lp_ordermatch.rs

shamardy

Huge work! Only one question for now since I am still trying to wrap my head around how all this supposed to work :)

mm2src/crypto/src/global_hd_ctx.rs

artemii235

A couple of questions. I will likely have more notes.

mm2src/coins/eth/v2_activation.rs

mm2src/mm2_core/src/mm_ctx.rs

* TODO fix mm2_main tests # Conflicts: # mm2src/coins/eth.rs # mm2src/coins/qrc20.rs # mm2src/coins/solana.rs # mm2src/coins/tendermint/tendermint_coin.rs # mm2src/coins/utxo/qtum.rs # mm2src/coins/z_coin.rs # mm2src/mm2_main/src/docker_tests.rs # mm2src/mm2_main/src/mm2_tests/lp_bot_tests.rs # mm2src/mm2_main/tests/docker_tests/docker_tests_common.rs # mm2src/mm2_main/tests/mm2_tests/bch_and_slp_tests.rs # mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs # mm2src/mm2_test_helpers/src/for_tests.rs

* Rename `PrivKeyBuildPolicy::IguanaPrivKey` to `Secp256k1Secret`

artemii235

Few more notes.

mm2src/crypto/src/crypto_ctx.rs

mm2src/crypto/src/global_hd_ctx.rs

mm2src/mm2_main/src/lp_swap/maker_swap.rs

mm2src/mm2_main/src/lp_swap/taker_swap.rs

# Conflicts: # mm2src/coins/eth.rs # mm2src/coins/lightning.rs # mm2src/coins/lightning/ln_errors.rs # mm2src/coins/lp_coins.rs # mm2src/coins/qrc20.rs # mm2src/coins/solana.rs # mm2src/coins/tendermint/tendermint_coin.rs # mm2src/coins/utxo/qtum.rs # mm2src/coins/utxo/slp.rs # mm2src/coins/utxo/utxo_standard.rs # mm2src/coins/z_coin.rs # mm2src/mm2_core/src/mm_ctx.rs # mm2src/mm2_main/tests/docker_tests/qrc20_tests.rs

* Fix `CryptoCtx` comments * Simplify `if` conditions for Negotiation message

* Remove unnecessary methods from `GlobalHDAccountCtx` * Rename `CryptoCtx::mm2_internal_privkey_bytes` to `mm2_internal_privkey_secret`

artemii235

Few more comments.

mm2src/mm2_main/src/lp_swap/maker_swap.rs

mm2src/mm2_err_handle/src/discard_mm.rs

mm2src/coins/z_coin.rs

* Remove `mm2_internal_key_pair` from `GlobalHDAccountCtx`

shamardy

LGTM!

laruh

Thanks, LGTM!

* Allo the user to specify `32`, `44`, `49`, `84` purposes in `derivation_path` * Rename `Bip44DerivationPath` to `StandardHDPath`

* Rename `DiscardMmResult::discard_mm` to `DiscardMmTrace::discard_mm_trace`

sergeyboyko0791 · 2022-11-15T23:28:51Z

@artemii235 PR is ready for the next review round

artemii235

Huge work, thanks a lot!

sergeyboyko0791 added 17 commits October 22, 2022 14:34

Add HdAccountCtx to CryptoCtx as an alternative to IguanaCtx

54f638c

Rename CryptoCtx::secp256k1_pubkey* to `CryptoCtx::mm2_internal_pub…

d180072

…key*`

Remove MmCtx::secp256k1_key_pair()

6621606

* TODO: remove `MmCtx::secp256k1_key_pair_as_option()` and ``MmCtx::secp256k1_key_pair`

Fix some TODOs

31c36f1

Add PrivKeyBuildPolicy::GlobalHDAccount

28d52d3

* Add `Secp256k1Secret` and `IguanaPrivKey` aliases, use it everywhere instead of the `[u8]` slice

Merge branch 'dev' into mm2-hd-account

fc26e99

Remove MmCtx::secp256k1_key_pair

15ebe4c

* Add common `InternalError` * Add `DiscardMmResult`, `SplitMmResult` traits to `mm2_error_handle` * Move `MmCtx::public_id()` to `CryptoCtx::mm2_internal_public_id()`

Remove unused dependencies

5ea08c0

Implement CryptoCtx::init_with_global_hd_account

2d60518

* Add `hd_account_id` conf param * Cover swaps in tests

Fix TODOs

a8791b5

* Rename `DerivationMethod::Iguana` to `DerivationMethod::SingleAddress` * Rename `PrivKeyNotAllowed` to `PrivKeyPolicyNotAllowed`

Cover hd_account_id with tests

ebcc10d

*

Fix fmt warnings

8cb09d2

Fix wasm tests

7a845f4

Fix RICK-MORTY swap

137c8fe

* Use `NegotiationDataMsg::V3` even if maker and taker pubkeys are the same but differ from persistent pub * Log Swap error event in WASM

Fix TODOs

e62cfdf

Merge branch 'dev' into mm2-hd-account

4477c1d

Minor changes

5627cde

sergeyboyko0791 changed the title ~~[wip] Global HD account~~ [r2r] Global HD account Nov 1, 2022

sergeyboyko0791 commented Nov 1, 2022

View reviewed changes

sergeyboyko0791 requested review from artemii235, laruh and shamardy November 1, 2022 15:17

shamardy reviewed Nov 3, 2022

View reviewed changes

mm2src/crypto/src/global_hd_ctx.rs Show resolved Hide resolved

artemii235 reviewed Nov 4, 2022

View reviewed changes

mm2src/coins/eth/v2_activation.rs Show resolved Hide resolved

mm2src/mm2_core/src/mm_ctx.rs Show resolved Hide resolved

sergeyboyko0791 changed the title ~~[r2r] Global HD account~~ [wip] Global HD account Nov 6, 2022

sergeyboyko0791 added 2 commits November 6, 2022 13:12

Remove PrivKeyBuildPolicy::GlobalHDAccount

5752e52

* Rename `PrivKeyBuildPolicy::IguanaPrivKey` to `Secp256k1Secret`

artemii235 requested changes Nov 8, 2022

View reviewed changes

sergeyboyko0791 added 2 commits November 10, 2022 11:21

Fix PR issues

e7416e5

* Fix `CryptoCtx` comments * Simplify `if` conditions for Negotiation message

sergeyboyko0791 changed the title ~~[wip] Global HD account~~ [r2r] Global HD account Nov 10, 2022

sergeyboyko0791 added 2 commits November 10, 2022 11:56

Use is_my_order within request_and_fill_orderbook

827a4f4

Remove IguanaCtx, move secp256k1_key_pair to CryptoCtx

7b6d130

* Remove unnecessary methods from `GlobalHDAccountCtx` * Rename `CryptoCtx::mm2_internal_privkey_bytes` to `mm2_internal_privkey_secret`

artemii235 requested changes Nov 11, 2022

View reviewed changes

mm2src/mm2_main/src/lp_swap/maker_swap.rs Show resolved Hide resolved

mm2src/mm2_err_handle/src/discard_mm.rs Outdated Show resolved Hide resolved

mm2src/coins/z_coin.rs Outdated Show resolved Hide resolved

Distinguish the common implementation of CryptoCtx initializers

e0c60ad

* Remove `mm2_internal_key_pair` from `GlobalHDAccountCtx`

shamardy mentioned this pull request Nov 11, 2022

Lightning integration #1045

Open

24 tasks

shamardy previously approved these changes Nov 11, 2022

View reviewed changes

laruh previously approved these changes Nov 13, 2022

View reviewed changes

sergeyboyko0791 changed the title ~~[r2r] Global HD account~~ [wip] Global HD account Nov 14, 2022

sergeyboyko0791 added 2 commits November 15, 2022 10:55

Merge branch 'dev' into mm2-hd-account

7bdbd6d

Return PrivKeyBuildPolicy::GlobalHDAccount

95a266e

sergeyboyko0791 dismissed stale reviews from laruh and shamardy via 95a266e November 15, 2022 12:24

sergeyboyko0791 added 6 commits November 15, 2022 17:46

Fix ZCoin BIP32 derivation

2548a0e

* Allo the user to specify `32`, `44`, `49`, `84` purposes in `derivation_path` * Rename `Bip44DerivationPath` to `StandardHDPath`

Add z_derivation_path to ZCoinProtocolInfo

68db7be

Check if BTC-segwit derivation works

185f74a

Minor fix

5c59055

* Rename `DiscardMmResult::discard_mm` to `DiscardMmTrace::discard_mm_trace`

Ignore Tendermint tests

1c253db

Fix test_withdraw_segwit

27b0928

sergeyboyko0791 changed the title ~~[wip] Global HD account~~ [r2r] Global HD account Nov 15, 2022

artemii235 approved these changes Nov 17, 2022

View reviewed changes

artemii235 merged commit a05819d into dev Nov 17, 2022

artemii235 deleted the mm2-hd-account branch November 17, 2022 10:19

shamardy mentioned this pull request Oct 13, 2023

Handle BIP49 purpose to support P2WPKH addresses with Trezor #1507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[r2r] Global HD account #1512

[r2r] Global HD account #1512

sergeyboyko0791 commented Oct 27, 2022

sergeyboyko0791 Nov 1, 2022

shamardy Nov 3, 2022

artemii235 Nov 4, 2022

shamardy Nov 4, 2022 •

edited

Loading

shamardy Nov 4, 2022

sergeyboyko0791 Nov 4, 2022

shamardy Nov 4, 2022

shamardy Nov 4, 2022

sergeyboyko0791 Nov 4, 2022

artemii235 Nov 10, 2022

shamardy left a comment •

edited

Loading

artemii235 left a comment

artemii235 left a comment

artemii235 left a comment

shamardy left a comment

laruh left a comment

sergeyboyko0791 commented Nov 15, 2022

artemii235 left a comment

[r2r] Global HD account #1512

[r2r] Global HD account #1512

Conversation

sergeyboyko0791 commented Oct 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy Nov 4, 2022 • 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

shamardy left a comment • edited Loading

Choose a reason for hiding this comment

artemii235 left a comment

Choose a reason for hiding this comment

artemii235 left a comment

Choose a reason for hiding this comment

artemii235 left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

laruh left a comment

Choose a reason for hiding this comment

sergeyboyko0791 commented Nov 15, 2022

artemii235 left a comment

Choose a reason for hiding this comment

shamardy Nov 4, 2022 •

edited

Loading

shamardy left a comment •

edited

Loading