Move keyring implementation from keychain to dcrwallet package #29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matheusd
force-pushed
the
dcrwallet-keyring
branch
from
7b2ae02
to
5714a20
Compare
matheusd
changed the title
fguisso
approved these changes
Aug 8, 2019
matheusd
force-pushed
the
dcrwallet-keyring
branch
from
5714a20
to
7b262c9
Compare
|
This is an excellent change that I'm happy to see. |
This adds two new buckets and a new function to the channel database: buckets for tracking keychain data and key family indexes and a function to return unique indices for individual key families. The main purpose of this is to allow keyring implementations to store the used indices for their keys without having to rely on an external store. This is mostly useful for using wallet-backed root HD keys while the wallet itself does not track off-chain keys and addresses. This change is self-contained and does not require database migrations.
This adds a test to ensure KeyRing implementations don't generate duplicate public keys when DeriveNextKey is called twice in a row for the same key family.
This adds a generic HDKeyRing type that implements both the KeyRing and SecretKeyRing interfaces. It can derive public and private keys purely from a set of root keys provided during setup.
This changes the interface test for the keychain package to use a single specific test function for each implementation. It also exposes the testing procedure via Check_X_Impl() functions, so that external implementations can test their correctness to the interface specification.
This commit moves the dcrwallet-backed implementation of the KeyRing and SecretKeyRing keychain interfaces from that package to the lnwallet/dcrwallet package. This uncouples the keychain package from dcrwallet, which allows future implementations to use modules that themselves use hdkeychain without causing import cycles. It also improves abstraction and possible package reuse by not having it depend on internal wallet packages and concentrates all dcrwallet-related functionality into a single package. The implementation itself of the WalletKeyRing is the same. The call sites were changed to use the new location for it. In order to allow moving the implementation to a different package, the interface tests were refactored to export check functions that are callable from client packages. This allows the keychain interface tests to be ran from external packages so that they can ensure they conform to the required semantics.
This moves the dcrwallet-backed WalletKeyRing from directly using pubkeys derived within the wallet to keeping track of used indices directly in dcrlnd's channel db and deriving the keys itself based on the wallet's root coin type extended private key. The main goal for this move is to prevent the wallet from tracking a (potentially) large number of accounts and addresses that were never meant for onchain use and that the current implementation of the wallet can't work with anyway due to their use being only on ln related scripts (which the wallet doesn't understand). In the upstream lnd this was solved by using a separate purpose branch. If the wallet eventually implements some way of using different purpose branches, than this can be easily adjusted in the future. Another improvement of this commit is dropping the need for a separate public WalletKeyRing by using it as a composed struct to the DcrWallet struct. This makes it so that the main package doesn't need to separately initialize the keyring (which doesn't make sense anyway given any KeyRing implementation is likely to be intemately tied to a corresponding WalletController implementation).
matheusd
force-pushed
the
dcrwallet-keyring
branch
from
7b262c9
to
e92cfd6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The main goal for this changeset is to move the keyring implementation (which requires an active and unlocked backing dcrwallet) from the keychain package to the lnwallet/dcrwallet package.
This makes the keychain package cleaner, more generic and removes its dependency to dcrwallet packages in particular and allows future keyring implementations to not be based on a local backing dcrwallet.
It also binds the existing keyring implementation (WalletKeyRing) to the existing wallet driver, which makes sense since it entirely relies on the internal backing wallet.
Another change introduced is that the public keys generated for use within the LN processes are no longer generated inside the backing dcrwallet (via
NextAddress()calls), but rather on the keyring itself, based on the root coin type extended private key provided by the wallet. This removes the need for the wallet to track the large(ish) amount of keys (with their associated addresses) used by LN scripts which should never appear on-chain and that the wallet doesn't even (currently) know how to deal with, given the fact that it doesn't understand how the LN scripts work. It also increases the uncoupling between dcrlnd and the backing dcrwallet, which might eventually allow dcrlnd to use remote (instead of embedded) wallets.The individual commits should be sane enough to be reviewed individually if needed, performing the required changes in stages.