Refactor: Separate CKeyStore interface from its basic implementation #5251

jtimon commented at 11:47 PM on November 9, 2014: contributor

And move CCriticalSection cs_KeyStore up from CKeyStore to CBasicKeyStore. Also cleanup some dependencies. This serves as preparation for #5208

jtimon force-pushed on Nov 10, 2014

theuni commented at 12:05 AM on November 12, 2014: member

ut ACK on this part, but please consider that as long as CKey is used in the interface(ish) class, it's still dependent on app-state and cannot be used for libs. Any suggestions there?

theuni commented at 12:06 AM on November 12, 2014: member

Not verified move-only yet, btw.

sipa commented at 12:06 PM on November 12, 2014: member

@theuni what do you mean by "app state"?

sipa commented at 12:45 PM on November 12, 2014: member

utACK, verified move-only. This is an improvement regardless of what we do with #5208 and @theuni's suggestions.

jtimon commented at 1:59 PM on November 15, 2014: contributor

Thanks for the acks, I don't think this really needs any testing at all besides compiling. As for using Ckey in the abstract class, @theuni, I don't really have any suggestions but I would like to understand the problem better. Can you elaborate a little bit on that? Is it the problem that key depends on allocators.h (and thus boost/thread/mutex.hpp and boost/thread/once.hpp? If so, the only thing that comes to mind is creating yet another interface that CKey implements (inherits from). Would that solve the concerns?

theuni commented at 6:11 PM on November 17, 2014: member

Sorry for the delay. As @sipa mentioned on the other PR, by "app state", I mean that the allocators pull in the global singleton which is fine for bitcoind and friends, but I'd very much like to avoid that for libs.

Yea, a base class beneath CKey would solve the problem I should think. Imo, it'd be nice to have it somewhat storage-agnostic, so that a lib could potentially pass in its own backing store. Ultimately the key would have to be accessed (obviously), but we could at least limit how much it's passed around so that the caller could retain as much control over the sensitive bits as possible.

Running with that idea a bit, would it be feasible for CKey to retain only a pointer, and we pass in a pointer from our securely-allocated buffer which is held elsewhere?

sipa commented at 6:28 PM on November 17, 2014: member

Sounds all good, but out of scope here :)

theuni commented at 6:35 PM on November 17, 2014: member

Sure, I've ACK'd this change, I only mentioned the app state in case there was a chance of solving both issues at once here. Looks like there's not, so I have no problem with doing that later.

in src/basic_keystore.h:None in 70536c3092 outdated

   0 | @@ -0,0 +1,49 @@
   1 | +// Copyright (c) 2009-2010 Satoshi Nakamoto
   2 | +// Copyright (c) 2009-2014 The Bitcoin developers
   3 | +// Distributed under the MIT software license, see the accompanying
   4 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +
   6 | +#ifndef H_BASIC_BITCOIN_KEYSTORE

Diapolo commented at 6:57 PM on November 17, 2014:

Nit: Should be BITCOIN_BASIC_KEYSTORE_H, if you want to use the new naming that was recently introduced in all other headers (not by me ^^).

jtimon commented at 10:11 PM on November 17, 2014:

When was that introduced? I've seen many other files with the H at the beginning instead of the end, which I find much more convenient when creating new files (just copy from an old file, and replace the end).

fanquake commented at 12:21 AM on November 18, 2014:

@jtimon In #5196

jtimon commented at 10:14 PM on November 17, 2014: contributor

@sipa @theuni Yes, solving that belongs to another PR but there's no harm starting to discussing it here. I'm not sure I fully understand the solution with the pointer.

theuni commented at 1:17 AM on November 18, 2014: member

@jtimon I think it's basically:

Remove CPrivKey from key.h
Remove LockObject/UnlockObject from key.h/cpp
Teach CKey Load()/GetPrivKey()/etc to use pointers rather than CPrivKey
Teach CKey to use a passed-in buffer rather than its internal vch[32]

At that point it's up to the caller (app-level) to lock the data before sending it into CKey and to maintain its backing store. I'm sure all of that could be organized somehow into something that doesn't involve actually messing with pointers all over the place.

I do think we should probably discuss this elsewhere though, I don't want to slow down this PR for something unrelated. I'll see if I can throw together a quick (ugly) POC as a starting point.

sipa commented at 10:06 AM on November 19, 2014: member

@theuni @jtimon here is an alternative: https://github.com/sipa/bitcoin/commits/keynoalloc

I'm not entirely happy with it, and I'm not sure how much it solves, but it sounds like what you were aiming to do?

theuni commented at 3:58 AM on November 20, 2014: member

@sipa After discussing on IRC, yes, very much so. Most of what remains will be worked out by #5227. Thanks!

jtimon commented at 12:48 PM on November 20, 2014: contributor

Fixed @Diapolo 's nit.

laanwj added the label Improvement on Dec 5, 2014

MOVEONLY: Separate CKeyStore interface from its basic implementation 461456386f

Move CCriticalSection cs_KeyStore up from CKeyStore to CBasicKeyStore

(also cleanup some dependencies)

df2a149721

MOVEONLY: Move CBasicKeyStore method implementations to cpp file 6192efb02b

jtimon force-pushed on Dec 27, 2014

jtimon commented at 6:17 PM on December 27, 2014: contributor

Needed rebase. Squashed nit commit.

in src/keystore.h:None in 6192efb02b

  42 | +        vchPubKeyOut = key.GetPubKey();
  43 | +        return true;
  44 | +    }
  45 |  
  46 |      //! Support for BIP 0013 : see https://github.com/bitcoin/bips/blob/master/bip-0013.mediawiki
  47 |      virtual bool AddCScript(const CScript& redeemScript) =0;

laanwj commented at 4:17 PM on January 20, 2015:

If the new keystore is meant to be a simple private key store for e.g. raw transaction API usage, then do *WatchOnly methods still belong on it?

jtimon commented at 2:11 AM on February 4, 2015:

Sorry for not answering earlier. If you comment those methods you'll get this errors:

wallet_ismine.cpp: In function ‘isminetype IsMine(const CKeyStore&, const CScript&)’:
wallet_ismine.cpp:42:22: error: ‘const class CKeyStore’ has no member named ‘HaveWatchOnly’
         if (keystore.HaveWatchOnly(scriptPubKey))
                      ^
wallet_ismine.cpp:88:18: error: ‘const class CKeyStore’ has no member named ‘HaveWatchOnly’
     if (keystore.HaveWatchOnly(scriptPubKey))

The other 3 methods can be safely removed from the interface. I mean, it's not unsolvable but I wanted to keep this simple...

jtimon renamed this:
~~Separate CKeyStore interface from its basic implementation~~
Refactor: Separate CKeyStore interface from its basic implementation
on Jan 21, 2015

jtimon commented at 4:29 PM on March 26, 2015: contributor

Needs rebase but @theuni was working on a better alternative. Should I rebase it or not?

theuni commented at 11:21 AM on March 27, 2015: member

I have something but I'm not sure if it's worth using. I'm away until Monday so don't hold on me if this blocks further work.

sipa commented at 6:15 PM on March 27, 2015: member

I think CBasicKeyStore can actually just go in script/sign; it's basically the interface that users of script/sign need to implement to enable arbitrary-key signing.

jtimon commented at 11:57 PM on April 8, 2015: contributor

@sipa that makes sense to me. Anyway, it already needs rebase and I would probably close this in favor of @theuni 's improved proposal once it's ready, so I'm closing for now.

jtimon closed this on Apr 8, 2015

MarcoFalke locked this on Sep 8, 2021