[docs] Add doxygen comments for keypool classes #15777

jnewbery commented at 6:46 PM on April 9, 2019: member

Docs/move-only

Adds doxygen comments for the CKeyPool and CReserveKey objects. The way these work is pretty confusing and it's easy to overlook details (eg #15557 (review)).

These are on the verbose side, but I think too much commenting is better than not enough. Happy to take feedback on what's an appropriate level.

fanquake added the label Docs on Apr 9, 2019

fanquake added the label Wallet on Apr 9, 2019

DrahtBot added the label Needs rebase on Apr 9, 2019

promag commented at 11:08 PM on April 9, 2019: member

:clap: very informative! Thanks! Concept ACK.

laanwj commented at 8:53 AM on April 10, 2019: member

Thanks for adding documentation! ACK fb776446810e8f8d3cada1613a30c68d4d3a6fff

jnewbery commented at 1:22 PM on April 10, 2019: member

rebased

jnewbery force-pushed on Apr 10, 2019

DrahtBot removed the label Needs rebase on Apr 10, 2019

DrahtBot commented at 3:06 PM on April 10, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15796 (CReserveKey should not allow passive key re-use, KeepKey in destructor by instagibbs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jnewbery force-pushed on Apr 10, 2019

jnewbery commented at 4:18 PM on April 10, 2019: member

rebased

[wallet] move-only: move CReserveKey to be next to CKeyPool

reviewer tip: use git diff --color-moved=dimmed-zebra

ef2d515af3

jnewbery force-pushed on Apr 14, 2019

in src/wallet/wallet.h:185 in dc0299ef2f outdated

 181 | + * serialize/deserialize the pool data to/from the database.
 182 | + */
 183 |  class CKeyPool
 184 |  {
 185 |  public:
 186 | +    // The time at which the key was generated. Set in AddKeypoolPubKeyWithDB

jonatack commented at 10:28 AM on April 15, 2019:

For my understanding: doc/developer-notes.md mentions using //! Description before the member syntax to describe member or variable use to be picked up by Doxygen. Is the // syntax used here with the intent to not be picked up, or for another reason?

jnewbery commented at 8:45 PM on April 29, 2019:

No reason, I just got the doxygen syntax wrong. Thanks for pointing this out!

jonatack commented at 10:31 AM on April 15, 2019: member

Very helpful documentation! :100: Concept ACK

jnewbery force-pushed on Apr 29, 2019

jnewbery commented at 8:45 PM on April 29, 2019: member

Fixed @jonatack's comment.

in src/wallet/wallet.h:142 in 292f0ee148 outdated

 134 | @@ -135,14 +135,61 @@ enum WalletFlags : uint64_t {
 135 |  
 136 |  static constexpr uint64_t g_known_wallet_flags = WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET | WALLET_FLAG_KEY_ORIGIN_METADATA;
 137 |  
 138 | -/** A key pool entry */
 139 | +/** A key from a CWallet's keypool
 140 | + *
 141 | + * The wallet holds one (for pre HD-split wallets) or several keypools. These
 142 | + * are sets of keys that have not yet been used to give out addresses or
 143 | + * send change to.

jonatack commented at 8:51 AM on May 1, 2019:

Possibly: "to provide addresses or receive change."

in src/wallet/wallet.h:147 in 292f0ee148 outdated

 143 | + * send change to.
 144 | + *
 145 | + * The Bitcoin Core wallet was originally a collection of unrelated private
 146 | + * keys with their associated addresses. If a non-HD wallet generated a
 147 | + * key/address, gave that address out and then restored a backup from before
 148 | + * that key had been generated, then any funds sent to that address would be

jonatack commented at 8:55 AM on May 1, 2019:

Possibly: "that key's existence," or "that key's generation,"

in src/wallet/wallet.h:170 in 292f0ee148 outdated

 166 | + * However, if many addresses were used since the backup, then the wallet may
 167 | + * not know how far ahead in the HD chain to look for its addresses. The
 168 | + * keypool is used to implement a 'gap limit'. The keypool maintains a set of
 169 | + * keys (by default 1000) ahead of the last used key and scans for the
 170 | + * addresses of those keys.  This avoids the risk of not seeing transactions
 171 | + * involving the wallet's addresses, or re-using the same address.

jonatack commented at 9:03 AM on May 1, 2019:

/s/, or/ or of/` (missing "of")

in src/wallet/wallet.h:172 in 292f0ee148 outdated

 168 | + * keypool is used to implement a 'gap limit'. The keypool maintains a set of
 169 | + * keys (by default 1000) ahead of the last used key and scans for the
 170 | + * addresses of those keys.  This avoids the risk of not seeing transactions
 171 | + * involving the wallet's addresses, or re-using the same address.
 172 | + *
 173 | + * HD-split added a second keypool (commit: 02592f4c). There is an external

jonatack commented at 9:14 AM on May 1, 2019:

Beginner perspective: ran git show 02592f4c to see what "HD-split" meant more exactly. Perhaps "The wallet with HD chain split feature (HD-split) added..."

in src/wallet/wallet.h:177 in 292f0ee148 outdated

 173 | + * HD-split added a second keypool (commit: 02592f4c). There is an external
 174 | + * keypool (for addresses to hand out) and an internal keypool (for change
 175 | + * addresses).
 176 | + *
 177 | + * Keypool keys are stored in the wallet/keystore's keymap. The keypool data is
 178 | + * stored as a sets of indexes in the wallet (setInternalKeyPool,

jonatack commented at 9:15 AM on May 1, 2019:

Seems this ought to be either "a set of" or "sets of"

jonatack commented at 9:29 AM on May 1, 2019: member

ACK https://github.com/bitcoin/bitcoin/pull/15777/commits/292f0ee148bbe3b305aa8c1623355cee08a05867

Feel free to ignore the nits I mention. Very helpful documentation, particularly for someone new to the codebase and its history like myself. Kudos for the relevant commit hashes to dive in further.

[docs] Add doxygen comment for CKeyPool 37796b2dd4

[docs] Add doxygen comment for CReserveKey f1a77b0c51

jnewbery force-pushed on May 1, 2019

jnewbery commented at 6:54 PM on May 1, 2019: member

Thanks for the review @jonatack . I've taken all your suggestions.

jonatack commented at 12:04 PM on May 6, 2019: member

Thanks, John. Re-ACK f1a77b0c5176306ca9f6f30211e32d3502ed4281, doc-only changes with respect to previous review.

jb55 commented at 6:35 AM on May 14, 2019: member

ACK f1a77b0c5176306ca9f6f30211e32d3502ed4281

MarcoFalke merged this on May 14, 2019

MarcoFalke closed this on May 14, 2019

MarcoFalke referenced this in commit 65526fc866 on May 14, 2019

sidhujag referenced this in commit ef84484a6a on May 15, 2019

deadalnix referenced this in commit 9f4f50f228 on Jun 4, 2020

deadalnix referenced this in commit 8a2ed5f2d0 on Jun 4, 2020

Munkybooty referenced this in commit e7e1fefdf7 on Oct 16, 2021

DrahtBot locked this on Dec 16, 2021