[Refactor] Decouple CKeyStore from CWatchOnlyStore #10980

pull NicolasDorier wants to merge 2 commits into bitcoin:master from NicolasDorier:decouplewatchonly changing 14 files +101 −76

NicolasDorier commented at 2:45 PM on August 3, 2017: contributor

Breaking CKeyStore into two parts, one being the CKeyStore, the other CWatchOnlyStore.

The future externalhd would then just have to implement a new CKeyStore.

See #9728 (comment)
fanquake added the label Refactoring on Aug 5, 2017
NicolasDorier force-pushed on Aug 7, 2017
[Refactor] Split CKeyStore from CWatchOnlyStore f63dad288e
NicolasDorier force-pushed on Aug 7, 2017
NicolasDorier force-pushed on Aug 7, 2017
[Refactor] Replace calls of ::IsMine to CWallet::IsMine 6b89306611
NicolasDorier force-pushed on Aug 8, 2017
NicolasDorier renamed this:
~~[WIP] Decouple CKeyStore from CWatchOnlyStore~~
[Refactor] Decouple CKeyStore from CWatchOnlyStore
on Aug 17, 2017
NicolasDorier commented at 6:16 AM on August 17, 2017: contributor

This is the first step to achieve what is described on #9728 (comment)

Ping @instagibbs
instagibbs commented at 5:38 PM on August 17, 2017: member

Will review.

in src/wallet/wallet.h:653 in f63dad288e outdated

 649 | @@ -650,7 +650,7 @@ class CAccountingEntry
 650 |   * A CWallet is an extension of a keystore, which also maintains a set of transactions and balances,
 651 |   * and provides the ability to create new transactions.
 652 |   */
 653 | -class CWallet : public CCryptoKeyStore, public CValidationInterface
 654 | +class CWallet : public CCryptoKeyStore, public CWatchOnlyStore, public CValidationInterface

instagibbs commented at 8:39 PM on August 23, 2017:

update description above in comments to reflect new inheritance

instagibbs commented at 1:32 PM on August 24, 2017: member

concept ACK, needs rebase
jtimon commented at 6:52 PM on November 2, 2017: contributor

Concept ACK, needs rebase
sipa commented at 6:53 PM on November 2, 2017: member

Concept NACK, I think this is a step in the wrong direction. The concept of watching should be removed from the keystore entirely.
NicolasDorier commented at 10:13 PM on November 2, 2017: contributor

@sipa what do you mean? this is precisely the intent of this PR: Removing WatchOnlyStore from CKeyStore. (see the CWatchOnlyStore class)
sipa commented at 10:22 PM on November 2, 2017: member

@NicolasDorier There's no need for a separate store for watch-only, watching should be determined by the wallet, not the keys or anything related to signing.

I'm working on a design doc for how to evolve the watching/signing code and segwit/HD/... support. I'll let you know when it's in a usable state.
ryanofsky commented at 7:43 PM on January 9, 2018: member

I'm working on a design doc for how to evolve the watching/signing code and segwit/HD/... support. I'll let you know when it's in a usable state.

Doc was posted as https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2#future-design

It does make this PR look like a step in a different direction if the goal is to have unified representation and handling of watchonly and signing keys.

It also isn't obvious to me how this PR would simplify https://github.com/instagibbs/bitcoin/commits/externalhd2 or #9728. @NicolasDorier it would be good if you could explain more why this change is useful, and whether you think anything in sipa's design might make it more difficult to support external hd wallets.
instagibbs commented at 9:58 PM on January 9, 2018: member

I've implemented hww support, and I believe the design document posted goes more in the right direction to make it sane and palatable for upstream. Obviously it will be a much larger project...
NicolasDorier commented at 4:38 PM on January 10, 2018: contributor

@ryanofsky this PR was an attempt to fix my immediate problem on externalhd2. I was trying to fix the mess incrementally, one PR after another.

The design doc has a different approach: Complete redesign + one time migration. I agree with the general direction it takes (I need to re read it later though), it just seems to me it will be quite hard to do incrementally.
NicolasDorier commented at 4:47 PM on January 10, 2018: contributor

@ryanofsky Just additional information about what I was thinking. This is long time ago so I might not remember all details...

Currently externalhd2 works fine but generate the address as "watch-only". The problem is that core consider an address "IS_MINE" only if you can sign. (which is not the case for addresses generated by external hd)

But nevertheless, the watchonly addresses generated by externalhd2 should be considered IS_MINE even if I can't sign. Because I want Core to behave exactly as if it was a normal wallet, except without it the ability to sign with the generated keys. (Another PR related to that that I had was separating IS_MINE from IS_SIGNABLE)

My solution was to create a new implementation of CKeyStore for external hd keys. The old CKeyStore would still be used in non-external hd mode. And I would keep the WatchOnlyStore separately from those classes. (As the WatchOnlyStore still need to exist independently from the CKeyStore implementation use)

Because generated addresses would come from the CKeyStore, it would be considered "IS_MINE".

This PR was the first step: Separating CWatchStore from CKeyStore.

I was attempting to slowly refactor the current flawed design until it get better. The design document though takes the bulldozer approach. (which might be OK, just harder)
NicolasDorier commented at 5:06 PM on March 6, 2018: contributor

closing this, my approach of slowly refactoring does not seem to get traction.
NicolasDorier closed this on Mar 6, 2018
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#9728 Can create External HD wallet with -externalhd