Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[r2r] Global HD account #1512
[r2r] Global HD account #1512
Changes from all commits
54f638c
d180072
6621606
31c36f1
28d52d3
fc26e99
15ebe4c
5ea08c0
2d60518
a8791b5
ebcc10d
8cb09d2
7a845f4
137c8fe
e62cfdf
4477c1d
5627cde
2857e58
5752e52
c607380
e7416e5
827a4f4
7b6d130
e0c60ad
7bdbd6d
95a266e
2548a0e
68db7be
185f74a
5c59055
1c253db
27b0928
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shamardy could you please confirm that we can use the platform's key-pair to initialize the
KeysManager
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theKeysManager::new
function internals for the exact details https://github.com/KomodoPlatform/atomicDEX-API/blob/5627cded1616e34c67b795023514a2a86b4c3bba/mm2src/coins/lightning/ln_utils.rs#L95We 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shamardy Will this effect from which address the funding transaction will be sent and to which address closed channel funds will be claimed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-L1184https://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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more note, we can always implement our own lightning
KeysManager
if we needed that. The only requirement forKeysManager
is to implement theKeysInterface
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 lightningKeysManager
where there is one node address using the master seed and change the destination script, the shutdown pubkey when switching addresses.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 anotherconf.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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 differenthd_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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thank you!
DerivationMethod::HDWallet
is really a confusing name, because it overlaps withGlobal HD account
. But they are both related to HD wallet:one coin -> one address
.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 newPrivKeyPolicy::CoinHDAccount
, for example. The only difference fromPrivKeyPolicy::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
toDerivationMethod::HardwareWallet
.@artemii235 could you please advice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.