[Refactor] Decouple CKeyStore from CWatchOnlyStore #10980

1. 
   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)

2. fanquake added the label Refactoring on Aug 5, 2017
3. NicolasDorier force-pushed on Aug 7, 2017
4. [Refactor] Split CKeyStore from CWatchOnlyStore f63dad288e
5. NicolasDorier force-pushed on Aug 7, 2017
6. NicolasDorier force-pushed on Aug 7, 2017
7. [Refactor] Replace calls of ::IsMine to CWallet::IsMine 6b89306611
8. NicolasDorier force-pushed on Aug 8, 2017
9. NicolasDorier renamed this:
   [WIP] Decouple CKeyStore from CWatchOnlyStore
   [Refactor] Decouple CKeyStore from CWatchOnlyStore
   on Aug 17, 2017
10. 
    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

11. 
    instagibbs commented at 5:38 pm on August 17, 2017: member

    Will review.
12. 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
13. 
    instagibbs commented at 1:32 pm on August 24, 2017: member

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

    Concept ACK, needs rebase
15. 
    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.
16. 
    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)
17. 
    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.

18. 
    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.

19. 
    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…
20. 
    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.

21. 
    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)

22. 
    NicolasDorier commented at 5:06 pm on March 6, 2018: contributor

    closing this, my approach of slowly refactoring does not seem to get traction.
23. NicolasDorier closed this on Mar 6, 2018

    ---

24. MarcoFalke locked this on Sep 8, 2021

Contributors
NicolasDorier instagibbs jtimon sipa ryanofsky

Labels
Refactoring