Split some CWallet functions into new LegacyScriptPubKeyMan #17260

pull achow101 wants to merge 3 commits into bitcoin:master from achow101:wallet-box-pr-1 changing 24 files +2152 −1763
  1. achow101 commented at 9:11 pm on October 25, 2019: member

    Moves key management functions into a new class LegacyScriptPubKeyMan. First two commits are move-only commits which move stuff out of wallet.{h/cpp} and into newly created scriptpubkeyman.{h/cpp}. Third commit changes several things in CWallet to use LegacyScriptPubKeyMan.

    First step in the wallet boxes refactor. Note that LegacyScriptPubKeyMan and ScriptPubKeyMan cannot be used standalone yet and are still very much tied into CWallet with both accessing functions within each other. This PR is to help reduce review burden.

  2. fanquake added the label Wallet on Oct 25, 2019
  3. fanquake added the label Refactoring on Oct 25, 2019
  4. achow101 force-pushed on Oct 25, 2019
  5. achow101 force-pushed on Oct 25, 2019
  6. achow101 force-pushed on Oct 25, 2019
  7. ryanofsky approved
  8. ryanofsky commented at 9:47 pm on October 25, 2019: member

    ACK af8780a65a530c48a0abc7f3ef6915f33d30943f confirming no changes other than rebase from my pr/keyman branch.

    Linter fixes in 626eed831918ac4f492b94ad0131f65d67fd1785

  9. in src/wallet/scriptpubkeyman.cpp:433 in 194227b117 outdated 
    431      * these. Do not add them to the wallet and warn. */
432     if (redeemScript.size() > MAX_SCRIPT_ELEMENT_SIZE)
433     {
434         std::string strAddr = EncodeDestination(ScriptHash(redeemScript));
435-        WalletLogPrintf("%s: Warning: This wallet contains a redeemScript of size %i which exceeds maximum size %i thus can never be redeemed. Do not use address %s.\n", __func__, redeemScript.size(), MAX_SCRIPT_ELEMENT_SIZE, strAddr);
436+        m_wallet.WalletLogPrintf("%s: Warning: This wallet contains a redeemScript of size %i which exceeds maximum size %i thus can never be redeemed. Do not use address %s.\n", __func__, redeemScript.size(), MAX_SCRIPT_ELEMENT_SIZE, strAddr);
    ryanofsky commented at 10:04 pm on October 25, 2019:

    In commit “Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes” (194227b1173b0dd9a51d24e245ddaa601d36a109)

    Would be better not to reference m_wallet here. m_wallet is a temporary alias so any references added now will have to be removed later, causing unnecessary churn. Would suggest reverting the commit back to your previous af8780a65a530c48a0abc7f3ef6915f33d30943f push, and just adding the linter fixes in 626eed831918ac4f492b94ad0131f65d67fd1785

    achow101 commented at 10:56 pm on October 25, 2019:
    Reverted and squashed linter changes.
  10. in src/wallet/test/wallet_tests.cpp:355 in 194227b117 outdated 
    353 {
354     CScript p2pk = GetScriptForRawPubKey(add_pubkey);
355     CKeyID add_address = add_pubkey.GetID();
356     CPubKey found_pubkey;
357-    LOCK(wallet.cs_wallet);
358+    LOCK2(spk_man->cs_wallet, spk_man->cs_KeyStore);
    ryanofsky commented at 10:06 pm on October 25, 2019:

    In commit “Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes” (194227b1173b0dd9a51d24e245ddaa601d36a109)

    Extra lock here seems not ideal. This commit is not supposed to change behavior in any way. Other places in this commit I just added AssertLockHelds where needed to avoid clang lock warnings, but here I’m not even seeing that is necessary.

    achow101 commented at 10:25 pm on October 25, 2019:
    It was failing the tests without it.
    ryanofsky commented at 1:45 pm on October 26, 2019:

    It was failing the tests without it.

    So this is definitely a bug. There can’t be any changes in behavior in this PR. All it is supposed do is add wrapper functions and variable aliases. If any side effects are present from this, it means something has gone wrong.

    Any case I reproduced the problem and found it was limited to tests and caused by me pulling in some test-only LOCK changes that didn’t belong here in a bad rebase. This PR can be fixed by just updating the tip from 1c49a20cb10f2d48cf23bb364d586a9c1e86d5ec to f201ba59ffd2e071a36a688b80d2cff9a9c44bb2 (compare) from my updated pr/keyman branch. Getting rid of the unintended test locking changes also makes this PR slightly smaller.

    (You can also reset #17261 to the updated pr/keyman branch with no loss since I pulled in your updated SigningProvider commit there.)

    achow101 commented at 4:25 pm on October 26, 2019:
    Done
  11. DrahtBot commented at 10:45 pm on October 25, 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:

    • #17283 (rpc: improve getaddressinfo test coverage, help, code docs by jonatack)
    • #17246 (wallet: avoid knapsack when there’s no change by Sjors)
    • #17237 (wallet: LearnRelatedScripts only if KeepDestination by promag)
    • #17219 (wallet: allow transaction without change if keypool is empty by Sjors)
    • #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
    • #16946 (wallet: include a checksum of encrypted private keys by achow101)
    • #16910 (wallet: reduce loading time by using unordered maps by achow101)
    • #16546 ([WIP] External signer support - Wallet Box edition by Sjors)
    • #16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
    • #16224 (gui: Bilingual GUI error messages by hebasto)
    • #15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
    • #15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
    • #15294 ([moveonly] wallet: Extract RipeMd160 by Empact)
    • #14942 (wallet: Make scan / abort status private to CWallet by Empact)
    • #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
    • #14384 (Fire TransactionRemovedFromMempool callbacks from mempool by l2a5b1)
    • #14144 (Refactoring: Clarify code using encrypted_batch in CWallet by domob1812)
    • #10785 (Serialization improvements by sipa)

    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.

  12. achow101 force-pushed on Oct 25, 2019
  13. achow101 force-pushed on Oct 25, 2019
  14. Move wallet enums to walletutil.h ab053ec6d1
  15. MOVEONLY: Move key handling code out of wallet to keyman file 
    Start moving wallet and ismine code to scriptpubkeyman.h, scriptpubkeyman.cpp

The easiest way to review this commit is to run:

   git log -p -n1 --color-moved=dimmed_zebra

And check that everything is a move (other than includes and copyrights comments).

This commit is move-only and doesn't change code or affect behavior.
    6702048f91
  16. Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes 
    This moves CWallet members and methods dealing with keys to a new
LegacyScriptPubKeyMan class, and updates calling code to reference the new
class instead of CWallet.

Most of the changes are simple text replacements and variable substitutions
easily verified with:

    git log -p -n1 -U0 --word-diff-regex=.

The only nontrivial chunk of code added is the new LegacyScriptPubKeyMan class
declaration, but this code isn't new and is just selectively copied and moved
from the previous CWallet class declaration. This can be verified with:

    git log -p -n1 --color-moved=dimmed_zebra src/wallet/scriptpubkeyman.h src/wallet/wallet.h

or

    git diff HEAD~1:src/wallet/wallet.h HEAD:src/wallet/scriptpubkeyman.h

This commit does not change behavior.
    f201ba59ff
  17. achow101 force-pushed on Oct 25, 2019
  18. Sjors commented at 8:44 am on October 26, 2019: member

    The code at 1c49a20cb10f2d48cf23bb364d586a9c1e86d5ec matches the first three commits in #17261 modulo the linter changes and extra lock discussed above. I in turn verified that #17261 matches what I previously reviewed.

    I’ll review this PR on its own merits later.

  19. achow101 force-pushed on Oct 26, 2019
  20. in src/wallet/rpcdump.cpp:143 in f201ba59ff 
    137@@ -138,6 +138,11 @@ UniValue importprivkey(const JSONRPCRequest& request)
138         throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled");
139     }
140 
141+    LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan();
142+    if (!spk_man) {
143+        throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
    Sjors commented at 5:20 pm on October 26, 2019:
    FWIW: I’m no longer in favor of splitting this out. For example this RPC method relies on ImportPrivKeys which is no longer a method on CWallet, but onLegacyScriptPubKeyMan (and not of its parent class ScriptPubKeyMan). This is in contrast to methods like setwalletflag that still call CWallet methods.
    ryanofsky commented at 6:44 pm on October 26, 2019:

    re: #17260 (review)

    FWIW: I’m no longer in favor of splitting this out. For example this RPC method relies on ImportPrivKeys which is no longer a method on CWallet, but onLegacyScriptPubKeyMan (and not of its parent class ScriptPubKeyMan). This is in contrast to methods like setwalletflag that still call CWallet methods.

    :thumbsup:

    For reference, Sjors is referring back to his suggestion from #16341 (comment) to move this code, and saying it doesn’t necessarily apply anymore, so the PR should be good here.

    promag commented at 11:01 am on October 28, 2019:
    nit, could add some helper to do this repeated check (currently 9 times), like GetLegacyScriptPubKeyMan(pwallet)?
    ryanofsky commented at 8:27 pm on October 28, 2019:

    re: #17260 (review)

    nit, could add some helper to do this repeated check (currently 9 times), like GetLegacyScriptPubKeyMan(pwallet)?

    Would be a nice followup. My preference would be to do it in a later PR though to avoid delay on this one, since this PR is pretty big and already has some acks.

    achow101 commented at 9:19 pm on October 28, 2019:
    Agreed for follow up.
  21. Sjors approved
  22. Sjors commented at 5:22 pm on October 26, 2019: member

    Code review ACK f201ba5.

    You could add a seperate commit that moves isminetype IsMine(const CWallet& keystore, const CTxDestination& dest) from IsMine.cpp to wallet.cpp, because that exception to the move from IsMine to scriptpubkeyman is a bit hard to spot now.

  23. in src/wallet/wallet.cpp:239 in f201ba59ff 
    245-    for (const auto& entry : out.pubkeys) {
246-        ret.push_back(entry.first);
247-    }
248-    return ret;
249-}
250+std::vector<CKeyID> GetAffectedKeys(const CScript& spk, const SigningProvider& provider);
    Sjors commented at 5:27 pm on October 26, 2019:
    It’s a bit weird to have a definition in a cpp file. On a local branch I was able to move this to scriptpubkeyman.h in the second commit, courtesy of circular includes.
    ryanofsky commented at 6:45 pm on October 26, 2019:

    In commit “MOVEONLY: Move key handling code out of wallet to keyman file” (6702048f91089d7a565e5ca5f7c8dcd2ca405a85):

    re: #17260 (review)

    It’s a bit weird to have a definition in a cpp file. On a local branch I was able to move this to scriptpubkeyman.h in the second commit, courtesy of circular includes.

    There’s a later commit that moves this declaration at the same time as moving the call: 8e743b90f5cec16383d2bf862b625c286db83661, so no need to necessarily move it here, even though it is a reasonable suggestion.

  24. ryanofsky commented at 7:01 pm on October 26, 2019: member
    Thanks for reviewing!
  25. laanwj added this to the "Blockers" column in a project
  26. promag commented at 11:01 am on October 28, 2019: member
    Code review ACK f201ba59ffd2e071a36a688b80d2cff9a9c44bb2.
  27. ryanofsky approved
  28. ryanofsky commented at 8:40 pm on October 28, 2019: member
    Code review ACK f201ba59ffd2e071a36a688b80d2cff9a9c44bb2
  29. in src/wallet/scriptpubkeyman.cpp:390 in f201ba59ff 
    388 }
389 
390-bool CWallet::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& secret, const CPubKey& pubkey)
391+bool LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& secret, const CPubKey& pubkey)
392 {
393-    AssertLockHeld(cs_wallet);
    MarcoFalke commented at 11:50 am on October 29, 2019:
    Why is this removed?
  30. in src/wallet/scriptpubkeyman.h:23 in f201ba59ff 
    15@@ -16,6 +16,25 @@
16 
17 enum class OutputType;
18 
19+// Wallet storage things that ScriptPubKeyMans need in order to be able to store things to the wallet database.
20+// It provides access to things that are part of the entire wallet and not specific to a ScriptPubKeyMan such as
21+// wallet flags, wallet version, encryption keys, encryption status, and the database itself. This allows a
22+// ScriptPubKeyMan to have callbacks into CWallet without causing a circular dependency.
23+// WalletStorage should be the same for all ScriptPubKeyMans.
    MarcoFalke commented at 11:55 am on October 29, 2019:

    in commit f201ba59ffd2e071a36a688b80d2cff9a9c44bb2:

    Could clarify that this is “for all ScriptPubKeyMans of a wallet” and not (“for all ScriptPubKeyMans of all wallets”)

  31. in src/wallet/wallet.cpp:253 in f201ba59ff 
    246@@ -248,6 +247,14 @@ const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const
247     return &(it->second);
248 }
249 
250+void CWallet::UpgradeKeyMetadata()
251+{
252+    AssertLockHeld(m_spk_man->cs_wallet);
253+    if (m_spk_man) {
    MarcoFalke commented at 11:59 am on October 29, 2019:
    How is this supposed to work? The pointer was already dereferenced in the line above
  32. in src/wallet/wallet.cpp:565 in f201ba59ff 
    561@@ -554,11 +562,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
562         Unlock(strWalletPassphrase);
563 
564         // if we are using HD, replace the HD seed with a new one
565-        if (IsHDEnabled()) {
566-            SetHDSeed(GenerateNewSeed());
567+        if (m_spk_man->IsHDEnabled()) {
    MarcoFalke commented at 12:02 pm on October 29, 2019:
    Why is a nullptr check required above but not here?
  33. in src/wallet/wallet.cpp:2787 in f201ba59ff 
    2781@@ -2693,7 +2782,12 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
2782                 const CScript& scriptPubKey = coin.txout.scriptPubKey;
2783                 SignatureData sigdata;
2784 
2785-                if (!ProduceSignature(*this, MutableTransactionSignatureCreator(&txNew, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata))
2786+                const SigningProvider* provider = GetSigningProvider();
2787+                if (!provider) {
2788+                    return false;
    MarcoFalke commented at 12:07 pm on October 29, 2019:
    Why is this not setting the error string?
  34. MarcoFalke commented at 12:18 pm on October 29, 2019: member

    Looks good. Just some questions about segmentation faults, which shouldn’t be an issue because the pointers are never null in the current code.

    ACK f201ba59ffd2e071a36a688b80d2cff9a9c44bb2

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK f201ba59ffd2e071a36a688b80d2cff9a9c44bb2
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiAFQv/e/bCI5hF3Iym4me/xYc/3wwA7zmN883yoQkIBieu5fM4Z5M0g4izzGGJ
 85saGqdCaiJZ05YeTNKXqC14byvKn1XhGqnrcBadNMiRgoa+LC8zbZU7ZezsI/gJ/
 91g722me3tqkoihSLo5znpSLEtlnbhdO/Hx3pC7P5vtPHqupnmL3DPe47NscWm+Gl
10+UqjZXX2R1MhT3cLQdOGLaK6G1CcZ/n0DYiJrsk1aCvstiAzmixdTNTaG6Kxp7oQ
11JlcMD+4FSWnwF4d3bjr+KjUl5n8lqPykUgjnp8bD1UaBXsMG+kQITujiEopRWWHN
12lJfitkIrEuKdAMQlB9ESFG2zaAX2Ucfi2uDkAhfJDREjEB0VIfJrOTwRQMQB33hL
133mYqN9e7FQ+m7nJrX4J3K7NXHOKZqwH5NF9jG38X2/E5MHhMtH+GMR2ENqyVYJHa
14P3R7sRGbiV3FtkL8UhvMQMM6OuZ9cQCqw16w5lEWI5djpzlSt3TYqedd3Lhfymuk
15CFjCzOsY
16=wJ5M
17-----END PGP SIGNATURE-----

    Timestamp of file with hash dc73b9d8ac5a486833fc09ba84ab60c9eb65828df3f5ec1320ebd5ff54f522da -

  35. MarcoFalke referenced this in commit 6a97e8a060 on Oct 29, 2019
  36. MarcoFalke merged this on Oct 29, 2019
  37. MarcoFalke closed this on Oct 29, 2019
  38. fanquake removed this from the "Blockers" column in a project
  39. ryanofsky referenced this in commit 52cf68f7ff on Oct 29, 2019
  40. ryanofsky referenced this in commit 628d11b2ba on Oct 29, 2019
  41. ryanofsky referenced this in commit 2632b1f124 on Oct 29, 2019
  42. ryanofsky referenced this in commit 4b28a05f08 on Oct 29, 2019
  43. ryanofsky referenced this in commit 53fe0b70ad on Oct 29, 2019
  44. ryanofsky referenced this in commit aa0e6d8ab1 on Oct 29, 2019
  45. MarcoFalke referenced this in commit 100fa0a62a on Oct 31, 2019
  46. sidhujag referenced this in commit 5273f889bb on Oct 31, 2019
  48. ryanofsky added this to the "PRs" column in a project
  49. fanquake moved this from the "PRs" to the "Done" column in a project
  50. JeremyRubin commented at 5:12 am on November 24, 2019: contributor

    So I might just be being lazy…

    But can you do a bit more documentation on this one? This breaks some of my outstanding work (not PR’d yet) and I’d like to better understand where I should be making this functionality live…

  51. JeremyRubin commented at 5:19 am on November 24, 2019: contributor
    I guess mostly it’s just stuff moving into scriptpubkeyman? But do I need to replicate the logic elsewhere now?
  52. achow101 commented at 7:04 am on November 24, 2019: member

    @JeremyRubin Have you read https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes

    Basically a bunch of things are moving into LegacyScriptPubKeyMan, and some of those that were moved become generic interfaces in CWallet which call the appropriate ScriptPubKeyMan.

  53. jasonbcox referenced this in commit b41b32f60c on Aug 6, 2020
  54. jasonbcox referenced this in commit 4c4b9a6521 on Aug 6, 2020
  55. jasonbcox referenced this in commit f3bc8a23fb on Aug 6, 2020
  56. jasonbcox referenced this in commit 089fb67955 on Aug 6, 2020
  57. sidhujag referenced this in commit 97e495b1a0 on Nov 10, 2020
  58. silence48 referenced this in commit 1bdcc4bdd7 on Nov 14, 2020
  59. silence48 referenced this in commit 874227c286 on Nov 14, 2020
  60. silence48 referenced this in commit afe605b7af on Nov 14, 2020
  61. silence48 referenced this in commit 8fc969282d on Nov 14, 2020
  62. silence48 referenced this in commit dac19f30ca on Nov 15, 2020
  63. silence48 referenced this in commit 6fb9cd14dd on Nov 15, 2020
  64. silence48 referenced this in commit fd8e538fa2 on Nov 15, 2020
  65. silence48 referenced this in commit c4276b3237 on Nov 15, 2020
  66. DrahtBot locked this on Dec 16, 2021


achow101 ryanofsky DrahtBot Sjors promag MarcoFalke JeremyRubin

Labels
Refactoring Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 13:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me