From ef2d515af3c74f61acca5c0c8936a30e7befc507 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 9 Apr 2019 13:13:03 -0400 Subject: [PATCH 1/3] [wallet] move-only: move CReserveKey to be next to CKeyPool reviewer tip: use git diff --color-moved=dimmed-zebra --- src/wallet/wallet.h | 56 ++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 53de1ea06..370085d0b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -181,6 +181,34 @@ public: } }; +/** A key allocated from the key pool. */ +class CReserveKey +{ +protected: + CWallet* pwallet; + int64_t nIndex{-1}; + CPubKey vchPubKey; + bool fInternal{false}; + +public: + explicit CReserveKey(CWallet* pwalletIn) + { + pwallet = pwalletIn; + } + + CReserveKey(const CReserveKey&) = delete; + CReserveKey& operator=(const CReserveKey&) = delete; + + ~CReserveKey() + { + ReturnKey(); + } + + void ReturnKey(); + bool GetReservedKey(CPubKey &pubkey, bool internal = false); + void KeepKey(); +}; + /** Address book data */ class CAddressBookData { @@ -1218,34 +1246,6 @@ public: */ void MaybeResendWalletTxs(); -/** A key allocated from the key pool. */ -class CReserveKey -{ -protected: - CWallet* pwallet; - int64_t nIndex{-1}; - CPubKey vchPubKey; - bool fInternal{false}; - -public: - explicit CReserveKey(CWallet* pwalletIn) - { - pwallet = pwalletIn; - } - - CReserveKey(const CReserveKey&) = delete; - CReserveKey& operator=(const CReserveKey&) = delete; - - ~CReserveKey() - { - ReturnKey(); - } - - void ReturnKey(); - bool GetReservedKey(CPubKey &pubkey, bool internal = false); - void KeepKey(); -}; - /** RAII object to check and reserve a wallet rescan */ class WalletRescanReserver { From 37796b2dd49772b17ff39a1a71b73f6d2248ac6d Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 9 Apr 2019 14:27:25 -0400 Subject: [PATCH 2/3] [docs] Add doxygen comment for CKeyPool --- src/wallet/wallet.h | 53 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 370085d0b..435f2b2a2 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -135,14 +135,61 @@ enum WalletFlags : uint64_t { static constexpr uint64_t g_known_wallet_flags = WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET | WALLET_FLAG_KEY_ORIGIN_METADATA; -/** A key pool entry */ +/** A key from a CWallet's keypool + * + * The wallet holds one (for pre HD-split wallets) or several keypools. These + * are sets of keys that have not yet been used to provide addresses or receive + * change. + * + * The Bitcoin Core wallet was originally a collection of unrelated private + * keys with their associated addresses. If a non-HD wallet generated a + * key/address, gave that address out and then restored a backup from before + * that key's generation, then any funds sent to that address would be + * lost definitively. + * + * The keypool was implemented to avoid this scenario (commit: 10384941). The + * wallet would generate a set of keys (100 by default). When a new public key + * was required, either to give out as an address or to use in a change output, + * it would be drawn from the keypool. The keypool would then be topped up to + * maintain 100 keys. This ensured that as long as the wallet hadn't used more + * than 100 keys since the previous backup, all funds would be safe, since a + * restored wallet would be able to scan for all owned addresses. + * + * A keypool also allowed encrypted wallets to give out addresses without + * having to be decrypted to generate a new private key. + * + * With the introduction of HD wallets (commit: f1902510), the keypool + * essentially became an address look-ahead pool. Restoring old backups can no + * longer definitively lose funds as long as the addresses used were from the + * wallet's HD seed (since all private keys can be rederived from the seed). + * However, if many addresses were used since the backup, then the wallet may + * not know how far ahead in the HD chain to look for its addresses. The + * keypool is used to implement a 'gap limit'. The keypool maintains a set of + * keys (by default 1000) ahead of the last used key and scans for the + * addresses of those keys. This avoids the risk of not seeing transactions + * involving the wallet's addresses, or of re-using the same address. + * + * The HD-split wallet feature added a second keypool (commit: 02592f4c). There + * is an external keypool (for addresses to hand out) and an internal keypool + * (for change addresses). + * + * Keypool keys are stored in the wallet/keystore's keymap. The keypool data is + * stored as sets of indexes in the wallet (setInternalKeyPool, + * setExternalKeyPool and set_pre_split_keypool), and a map from the key to the + * index (m_pool_key_to_index). The CKeyPool object is used to + * serialize/deserialize the pool data to/from the database. + */ class CKeyPool { public: + //! The time at which the key was generated. Set in AddKeypoolPubKeyWithDB int64_t nTime; + //! The public key CPubKey vchPubKey; - bool fInternal; // for change outputs - bool m_pre_split; // For keys generated before keypool split upgrade + //! Whether this keypool entry is in the internal keypool (for change outputs) + bool fInternal; + //! Whether this key was generated for a keypool before the wallet was upgraded to HD-split + bool m_pre_split; CKeyPool(); CKeyPool(const CPubKey& vchPubKeyIn, bool internalIn); From f1a77b0c5176306ca9f6f30211e32d3502ed4281 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 9 Apr 2019 14:36:57 -0400 Subject: [PATCH 3/3] [docs] Add doxygen comment for CReserveKey --- src/wallet/wallet.h | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 435f2b2a2..7c9c7486c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -228,16 +228,35 @@ public: } }; -/** A key allocated from the key pool. */ +/** A wrapper to reserve a key from a wallet keypool + * + * CReserveKey is used to reserve a key from the keypool. It is passed around + * during the CreateTransaction/CommitTransaction procedure. + * + * Instantiating a CReserveKey does not reserve a keypool key. To do so, + * GetReservedKey() needs to be called on the object. Once a key has been + * reserved, call KeepKey() on the CReserveKey object to make sure it is not + * returned to the keypool. Call ReturnKey() to return the key to the keypool + * so it can be re-used (for example, if the key was used in a new transaction + * and that transaction was not completed and needed to be aborted). + * + * If a key is reserved and KeepKey() is not called, then the key will be + * returned to the keypool when the CReserveObject goes out of scope. + */ class CReserveKey { protected: + //! The wallet to reserve the keypool key from CWallet* pwallet; + //! The index of the key in the keypool int64_t nIndex{-1}; + //! The public key CPubKey vchPubKey; + //! Whether this is from the internal (change output) keypool bool fInternal{false}; public: + //! Construct a CReserveKey object. This does NOT reserve a key from the keypool yet explicit CReserveKey(CWallet* pwalletIn) { pwallet = pwalletIn; @@ -246,13 +265,17 @@ public: CReserveKey(const CReserveKey&) = delete; CReserveKey& operator=(const CReserveKey&) = delete; + //! Destructor. If a key has been reserved and not KeepKey'ed, it will be returned to the keypool ~CReserveKey() { ReturnKey(); } - void ReturnKey(); + //! Reserve a key from the keypool bool GetReservedKey(CPubKey &pubkey, bool internal = false); + //! Return a key to the keypool + void ReturnKey(); + //! Keep the key. Do not return it to the keypool when this object goes out of scope void KeepKey(); };