Make ScriptPubKeyMan an actual interface and the wallet to have multiple #17261

pull achow101 wants to merge 13 commits into bitcoin:master from achow101:wallet-box-pr-2 changing 30 files +493 −263
  1. achow101 commented at 5:16 am on October 26, 2019: member

    Continuation of wallet boxes project.

    Actually makes ScriptPubKeyMan an interface which LegacyScriptPubkeyMan. Moves around functions and things from CWallet into LegacyScriptPubKeyMan so that they are actually separate things without circular dependencies.


    Introducing the ScriptPubKeyMan (short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys. This functionality is moved over from CWallet. Instead, CWallet will have a pointer to a ScriptPubKeyMan for every possible address type, internal and external. It will fetch the correct ScriptPubKeyMan as necessary. When fetching new addresses, it chooses the ScriptPubKeyMan based on address type and whether it is change. For signing, it takes the script and asks each ScriptPubKeyMan for whether that ScriptPubKeyMan considers that script IsMine, whether it has that script, or whether it is able to produce a signature for it. If so, the ScriptPubKeyMan will provide a SigningProvider to the caller which will use that in order to sign.

    There is currently one ScriptPubKeyMan - the LegacyScriptPubKeyMan. Each CWallet will have only one LegacyScriptPubKeyMan with the pointers for all of the address types and change pointing to this LegacyScriptPubKeyMan. It is created when the wallet is loaded and all keys and metadata are loaded into it instead of CWallet. The LegacyScriptPubKeyMan is primarily made up of all of the key and script management that used to be in CWallet. For convenience, CWallet has a GetLegacyScriptPubKeyMan which will return the LegacyScriptPubKeyMan or a nullptr if it does not have one (not yet implemented, but callers will check for the nullptr). For purposes of signing, LegacyScriptPubKeyMan’s GetSigningProvider will return itself rather than a separate SigningProvider. This will be different for future ScriptPubKeyMans.

    The LegacyScriptPubKeyMan will also handle the importing and exporting of keys and scripts instead of CWallet. As such, a number of RPCs have been limited to work only if a LegacyScriptPubKeyMan can be retrieved from the wallet. These RPCs are sethdseed, addmultisigaddress, importaddress, importprivkey, importpubkey, importmulti, dumpprivkey, and dumpwallet. Other RPCs which relied on the wallet for scripts and keys have been modified in order to take the SigningProvider retrieved from the ScriptPubKeyMan for a given script.

    Overall, these changes should not effect how everything actually works and the user should experience no difference between having this change and not having it. As such, no functional tests were changed, and the only unit tests changed were those that were directly accessing CWallet functions that have been removed.

    This PR is the last step in the Wallet Structure Changes.

  2. achow101 commented at 5:17 am on October 26, 2019: member
    The final diff is very similar to #16341 just with some things dropped as they were deemed unnecessary.
  3. achow101 force-pushed on Oct 26, 2019
  4. DrahtBot added the label Build system on Oct 26, 2019
  5. DrahtBot added the label GUI on Oct 26, 2019
  6. DrahtBot added the label RPC/REST/ZMQ on Oct 26, 2019
  7. DrahtBot added the label Tests on Oct 26, 2019
  8. DrahtBot added the label Utils/log/libs on Oct 26, 2019
  9. DrahtBot added the label Wallet on Oct 26, 2019
  10. fanquake removed the label Build system on Oct 26, 2019
  11. fanquake removed the label GUI on Oct 26, 2019
  12. fanquake removed the label RPC/REST/ZMQ on Oct 26, 2019
  13. fanquake removed the label Utils/log/libs on Oct 26, 2019
  14. fanquake added the label Refactoring on Oct 26, 2019
  15. Sjors commented at 8:35 am on October 26, 2019: member
    For comparison, my last ACK on the original was for c37be15. That code was identical to @ryanofsky’s branch at 299296e51f6ea0e416aa84d7fc139195be96a58d, just using a different set of commits to get there. This PR rebases that on master @25d7e2e78137d07eb612c44d19b0d496050c947a, with the last two commits (952ba99a447b619e353816cac429f825cb3bcaff & 299296e51f6ea0e416aa84d7fc139195be96a58d) dropped. The result is f47ffe0a88111a63e1d7d98c25fbe2184215dbbe here (minus two linter changes).
  16. DrahtBot commented at 11:55 am on October 26, 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:

    • #17954 (wallet: Remove calls to Chain::Lock methods by ryanofsky)
    • #17878 (wip: zmq: Support -zmqpubwallettx by promag)
    • #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
    • #17681 (wallet: Keep inactive seeds after sethdseed and derive keys from them as needed by achow101)
    • #17484 (wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload by ariard)
    • #17443 (Drop checkFinalTx and use Median Time Past to check finality of wallet transactions by ariard)
    • #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)
    • #16698 (Mempool: rework rebroadcast logic to improve privacy by amitiuttarwar)
    • #16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
    • #16440 (BIP-322: Generic signed message format by kallewoof)
    • #16224 (gui: Bilingual GUI error messages by hebasto)
    • #15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
    • #15719 (Drop Chain::requestMempoolTransactions method by ryanofsky)
    • #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

    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.

  17. achow101 force-pushed on Oct 26, 2019
  18. fanquake added this to the "Blockers" column in a project

  19. ryanofsky referenced this in commit aa0e6d8ab1 on Oct 29, 2019
  20. achow101 force-pushed on Oct 29, 2019
  21. ryanofsky commented at 6:39 pm on October 29, 2019: member

    Suggestion: there’s a cluster of fairly trivial refactoring commits in this PR that I think could be pulled out into a separate PR and merged pretty quickly. Doing this would reduce the number of commits here by almost half:

    • b318219c51e316cfb75639f0820cc1b8017bfdfd MOVEONLY: Reorder LegacyScriptPubKeyMan methods (4/36)
    • 8c7eac4de3e94adcf3ec8a0358c410a198b8b844 Refactor: Declare LegacyScriptPubKeyMan methods as virtual (5/36)
    • f0339ba0b3a9d54f04b0e5f0ec80a9f791567caa Refactor: Add new ScriptPubKeyMan virtual methods (6/36)
    • 5321cf7eb565834a7e69331d690543375890d516 Refactor: Move SetAddressBook call out of LegacyScriptPubKeyMan::GetNewDestination (7/36)
    • 44f524e6c2116a4c57e9e47c4b130efe751cf5a7 Refactor: Move SetWalletFlag out of LegacyScriptPubKeyMan::UpgradeKeyMetadata (8/36)
    • 2b92e87104868e11be9a0e2129d1340677ebc81c Refactor: Remove UnsetWalletFlag call from LegacyScriptPubKeyMan::SetHDSeed (9/36)
    • 802dfb95abf9465efe34d16ece4f7e6dc8349177 Refactor: Move SetAddressBookWithDB call out of LegacyScriptPubKeyMan::ImportScriptPubKeys (10/36)
    • d8d2d31e55a53bfeb3860f278e47692970bdcd84 Refactor: Move LoadKey LegacyScriptPubKeyMan method definition (11/36)
    • 9f20667b5bb4f7939efa2cf16e3f12abf34ebbe5 Refactor: Move GetMetadata code out of getaddressinfo (12/36)
    • 309ee44cc663c8d753729d94100bbbe694c2e091 Refactor: Move MarkUnusedAddresses code out of CWallet::AddToWalletIfInvolvingMe (13/36)
    • 1ffce090c62ca340e09ace89d859843b995b0ee9 Refactor: Move Upgrade code out of CWallet::CreateWalletFromFile (14/36)
    • f09be4ff1cc00390fc50147c63260983a2d98d71 Refactor: Move HavePrivateKeys code out of CWallet::CreateWalletFromFile (15/36)
    • 25d904ce890f2989cbd3f677972be59d1381e98c Refactor: Move SetupGeneration code out of CWallet (16/36)
    • a207dbc464a647342c55cfd3c81e9866c063cdce Refactor: Move RewriteDB code out of CWallet (17/36)
    • 1cca908e4d1f855e961eb513ff87deda6a615323 Refactor: Move GetKeypoolSize code out of CWallet (18/36)
    • 23008f4c36d3bd982dcdb70698b6dc05b673c4e0 Refactor: Move nTimeFirstKey accesses out of CWallet (19/36) @achow101, would you want to pull out the commits above into a separate PR? I’d also be happy to make the PR if it would be more convenient.
  22. achow101 force-pushed on Oct 29, 2019
  23. achow101 commented at 7:03 pm on October 29, 2019: member
    @ryanofsky Done as #17304
  24. MarcoFalke commented at 8:15 pm on October 29, 2019: member

    fanquake added this to Blockers in High-priority for review 5 hours ago

    Why? I thought that high-prio could only be used once per week per contributor.

  25. fanquake commented at 8:24 pm on October 29, 2019: member

    Achow requested it via IRC. He didn’t have anything in high-prio.

    On Tue, 29 Oct 2019 at 16:15, MarcoFalke notifications@github.com wrote:

    fanquake added this to Blockers in High-priority for review 5 hours ago

    Why?

    — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/17261?email_source=notifications&email_token=AAGS34SKBDHCRX7QCH2LWVLQRCKWDA5CNFSM4JFL4BW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECR5VXY#issuecomment-547609311, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGS34RIQ5JLGSG4BKV3SCTQRCKWDANCNFSM4JFL4BWQ .

  26. MarcoFalke commented at 0:36 am on October 30, 2019: member
    #17260#event-2751955233 was in for this week, but fair enough
  27. DrahtBot added the label Needs rebase on Oct 30, 2019
  28. achow101 force-pushed on Oct 30, 2019
  29. DrahtBot removed the label Needs rebase on Oct 30, 2019
  30. DrahtBot added the label Needs rebase on Oct 31, 2019
  31. ryanofsky commented at 4:51 pm on November 4, 2019: member
    In case its useful, rebased branch pr/keyman 9e776722b902d8f6d095d06053b12746065baa46 -> 6ff0b88d5a98465eb79a02ceb7a46afb31d43708 (pr/keyman.9 -> pr/keyman.10) after #17300 and #17304
  32. ryanofsky commented at 5:59 pm on November 4, 2019: member
    Updated branch pr/keyman 6ff0b88d5a98465eb79a02ceb7a46afb31d43708 -> 72df6f1050d27a13435fab77f20c1a0e08fdbf51 (pr/keyman.10 -> pr/keyman.11, compare) to fix exception in CWallet::GetLegacyScriptPubKeyMan() caused by being called from CWallet::UpgradeKeyMetadata() early in initialization. This makes tests pass.
  33. achow101 force-pushed on Nov 4, 2019
  34. achow101 force-pushed on Nov 4, 2019
  35. achow101 commented at 6:31 pm on November 4, 2019: member
    Rebased
  36. ryanofsky commented at 6:33 pm on November 4, 2019: member

    Now that most of code movement is done, to make more progress on this, I’d say the following individual commits are complicated enough to be their own PRs which could all be reviewed and merged in parallel:

    • b5f456eb91e687ed39bc38e5b9b4e5aca0434515 Always try to sign for all pubkeys in multisig (2/20)
    • a1297615cce13bba3c16412418e0522e3f7f9eff Store p2sh scripts in AddAndGetDestinationForScript (3/20)
    • 09d99fc720998c892b60c26ff02e7dc11f69fee4 Refactor: Move encryption code between KeyMan and Wallet (4/20)
    • 11157cffc8925fe129d045d0238106db3bfb444a Refactor: Allow LegacyScriptPubKeyMan to be null (6/20)
    • a0b524c10f6944195f8fffa2a6558e002647fe23 Refactor: Require scriptPubKey to get wallet SigningProvider (7/20)
    • ac8cbde93df8cf365a08f472d2018f9667fae7c9 IsMine: Set state to WATCH_ONLY if we can get the pubkey (14/20)

    Then there is a set of keypool tweaks and fixes, which could go in a single PR (reviewed in parallel with PRs above):

    • 71f1a7ff10408211b7f86ee3c932a3d264441643 Key pool: Move CanGetAddresses call (8/20)
    • 5733cc0303673d58e887313965e5fb045f08e472 Key pool: Move LearnRelated and GetDestination calls (9/20)
    • 922252b6ff74a9d0da60c7bd87217eb5b938e220 Key pool: Move TopUp call (10/20)
    • 4fb82dcb7fac2f3684b9c4345946956488066d7a Key pool: Change ReturnDestination interface to take address instead of key (11/20)
    • 0c823e4d5c687fe116d2de550888558a3005b184 Key pool: Make TopUp fail if unexpected wallet flags are set (12/20)
    • 68a35f25bf3036d3390243c1a06887681d144586 Key pool: Fix omitted pre-split count in GetKeyPoolSize (13/20)

    After the encryption change above is in (09d99fc720998c892b60c26ff02e7dc11f69fee4), the cs_wallet/cs_keystore locking change could be a quick followup PR:

    • 558e0bb42cc0336bde941fb3a6e871e98a96962f Locking: Lock cs_KeyStore instead of cs_main in legacy keyman (5/20)

    After that, the remaining commits in this PR would add the new key manager maps and lookup code, and do final cleanups and tweaks:

    • fbcf6ad0ee6a87ac718c3ee2d9fa7663e18495a2 List output types in an array in order to be iterated over (1/20)
    • 85a1bb8e3a363eeca8f15505b716d6c04733cd11 HD Split: Avoid redundant upgrades (15/20)
    • 2fa9e854da0641d289b5d975ab3d15449fe59deb Box the wallet: Add multiple keyman maps and loops (16/20)
    • dd447f855324d5c6cebc2054db03d2fb5fdc4149 Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan (17/20)
    • ddf78847a8abc62b3339fe98aea467a2fce894c6 Cleanup: Update message strings and comments (18/20)
    • 32ec05d7aec6574928f5ca019c61ce24dccb1c25 Cleanup: Drop unused GUI learnRelatedScripts method (19/20)
    • 72df6f1050d27a13435fab77f20c1a0e08fdbf51 Refactor: Replace SigningProvider pointers with unique_ptrs (20/20)
  37. in src/wallet/scriptpubkeyman.h:299 in 90287119f4 outdated
    291@@ -292,8 +292,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
    292 
    293     void MarkUnusedAddresses(const CScript& script) override;
    294 
    295-    //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo
    


    ryanofsky commented at 6:50 pm on November 4, 2019:

    In commit “Locking: Lock cs_KeyStore instead of cs_main in legacy keyman” (90287119f4bab55e3ffb8e5b9139d19f11eebd61):

    Looks like comment this line was unintentionally deleted (mistake during rebase)


    achow101 commented at 7:19 pm on November 4, 2019:
    Added it back in.
  38. ryanofsky commented at 6:58 pm on November 4, 2019: member
    Current rebase d03af791a8b7a290c0d883ecf6642ead3ffe6a6a looks basically same as the rebase I did, but I think it avoids a bug in CWallet::UpgradeKeyMetadata where my rebase would incorrectly set the WALLET_FLAG_KEY_ORIGIN_METADATA if legacy keyman was null (tests passed anyway so I guess coverage is not great here). The cleanup change to group together all wallet flag set/unset methods in wallet.h was also dropped.
  39. achow101 force-pushed on Nov 4, 2019
  40. DrahtBot removed the label Needs rebase on Nov 4, 2019
  41. achow101 commented at 11:46 pm on November 4, 2019: member

    “Refactor: Move encryption code between KeyMan and Wallet” was split into #17369

    “Refactor: Require scriptPubKey to get wallet SigningProvider” was split into #17371

    The Keypool commits were split into #17373

    “IsMine: Set state to WATCH_ONLY if we can get the pubkey” was split into #17374

    I had some issues with making “Refactor: Allow LegacyScriptPubKeyMan to be null” standalone, there were some segfaults and other conflicting changes since it comes after the locking and encryption commits. So I don’t think I will make this standalone.

    Both “Always try to sign for all pubkeys in multisig” and “Store p2sh scripts in AddAndGetDestinationForScript” aren’t needed until “Box the wallet: Add multiple keyman maps and loops”. I don’t think they make sense by themselves, and they aren’t really complicated. So I would prefer to bundle them with “Box the wallet: Add multiple keyman maps and loops”.

    I’ll PR the locking commit after the encryption commit is merged.

  42. ryanofsky added this to the "PRs" column in a project

  43. achow101 force-pushed on Nov 5, 2019
  44. DrahtBot added the label Needs rebase on Nov 6, 2019
  45. achow101 force-pushed on Nov 6, 2019
  46. DrahtBot removed the label Needs rebase on Nov 7, 2019
  47. in src/wallet/scriptpubkeyman.cpp:281 in 3499674136 outdated
    277@@ -278,6 +278,8 @@ bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool i
    278     if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
    279         return false;
    280     }
    281+    LearnRelatedScripts(keypool.vchPubKey, type);
    


    promag commented at 12:40 pm on November 7, 2019:

    3499674136167f662e5b0ef0b599c02f5a959558

    You gave ACK in #17237 so I wonder why is this here?


    ryanofsky commented at 1:21 pm on November 7, 2019:

    3499674

    You gave ACK in #17237 so I wonder why is this here?

    Better to ask this question in #17373 where this commit was moved than this PR. Specifically 17373/commits/35c37b5

    Probably this code change just precedes your PR


    promag commented at 2:06 pm on November 7, 2019:
    Thanks for pointing to the right PR, I think I’ll just wait for this get merged.

    ryanofsky commented at 2:14 pm on November 7, 2019:
    It would be nice to get more review on #17373, even if this question won’t be raised. The changes there are confusing to me, and might be more straightforward to you. (The changes there are minor and seem safe but I don’t understand why they are made.)

    promag commented at 2:29 pm on November 7, 2019:
    You mean #17373, not #17237 right? If so I’ll review the first.
  48. ryanofsky commented at 1:27 pm on November 7, 2019: member

    Maybe say at the top of the PR description that this builds on #17369, #17371, #17373, and those PRs should be reviewed first to avoid misplaced review comments here.

    Can also link to the descriptor wallet project url which lists all PRs https://github.com/bitcoin/bitcoin/projects/12

  49. DrahtBot added the label Needs rebase on Nov 8, 2019
  50. achow101 force-pushed on Nov 8, 2019
  51. DrahtBot removed the label Needs rebase on Nov 8, 2019
  52. DrahtBot added the label Needs rebase on Nov 20, 2019
  53. meshcollider referenced this in commit 8aac85d71e on Nov 22, 2019
  54. meshcollider referenced this in commit 4effd67bf4 on Nov 22, 2019
  55. sidhujag referenced this in commit fa0c6c83dd on Nov 22, 2019
  56. sidhujag referenced this in commit b609c9ed8a on Nov 22, 2019
  57. achow101 force-pushed on Nov 23, 2019
  58. DrahtBot removed the label Needs rebase on Nov 23, 2019
  59. achow101 force-pushed on Nov 25, 2019
  60. DrahtBot added the label Needs rebase on Nov 26, 2019
  61. achow101 force-pushed on Nov 26, 2019
  62. DrahtBot removed the label Needs rebase on Nov 26, 2019
  63. Sjors commented at 1:35 pm on November 27, 2019: member
    For those interested in helping out, please review #17369, #17537 and #17261.
  64. ryanofsky commented at 1:50 pm on November 27, 2019: member

    For those interested in helping out, please review #17369, #17537 and #17261.

    Would add #17373. You can also check https://github.com/bitcoin/bitcoin/projects/12 for the latest list of descriptor wallet pull requests.

  65. fanquake referenced this in commit 4ee8a58ce7 on Dec 6, 2019
  66. DrahtBot added the label Needs rebase on Dec 6, 2019
  67. achow101 force-pushed on Dec 6, 2019
  68. DrahtBot removed the label Needs rebase on Dec 6, 2019
  69. sidhujag referenced this in commit 18e4eb8dc8 on Dec 6, 2019
  70. laanwj referenced this in commit 0192bd0652 on Dec 12, 2019
  71. achow101 force-pushed on Dec 12, 2019
  72. sidhujag referenced this in commit baae132c23 on Dec 12, 2019
  73. DrahtBot added the label Needs rebase on Dec 16, 2019
  74. achow101 force-pushed on Dec 17, 2019
  75. achow101 force-pushed on Dec 17, 2019
  76. DrahtBot removed the label Needs rebase on Dec 17, 2019
  77. in src/qt/test/wallettests.cpp:148 in a5e0d06d4b outdated
    140@@ -141,8 +141,7 @@ void TestGUI(interfaces::Node& node)
    141     {
    142         auto spk_man = wallet->GetLegacyScriptPubKeyMan();
    143         auto locked_chain = wallet->chain().lock();
    144-        LOCK(wallet->cs_wallet);
    145-        AssertLockHeld(spk_man->cs_wallet);
    146+        LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore);
    


    instagibbs commented at 8:21 pm on December 19, 2019:
    little confused; the commit says cs_main and I see cs_wallet being deleted. Fix commit message?

    achow101 commented at 7:56 pm on December 30, 2019:
    Fixed.
  78. in src/wallet/scriptpubkeyman.h:204 in 9020b9f0b1 outdated
    200     virtual const CKeyMetadata* GetMetadata(const CTxDestination& dest) const { return nullptr; }
    201+
    202+    virtual const SigningProvider* GetSigningProvider(const CScript& script) const { return nullptr; }
    203+
    204+    /** Whether this ScriptPubKeyMan can provide a SigningProvider (via GetSigningProvider) that, combined with
    205+      * sigdata, can produce a valid signature.
    


    instagibbs commented at 5:03 pm on December 30, 2019:
    s/can/is sufficient/ since the wallet itself may actually be unable to sign(watchonly)

    achow101 commented at 6:59 pm on December 30, 2019:
    Is that not stated by the “combined with sigdata” part?

    instagibbs commented at 9:29 pm on January 3, 2020:
    If I think of better wording I’ll let you know.
  79. in src/wallet/scriptpubkeyman.cpp:524 in 9020b9f0b1 outdated
    518@@ -491,6 +519,11 @@ const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(const CTxDestination& des
    519     return nullptr;
    520 }
    521 
    522+uint256 LegacyScriptPubKeyMan::GetID() const
    523+{
    524+    return uint256S("0000000000000000000000000000000000000000000000000000000000000001");
    


    instagibbs commented at 5:12 pm on December 30, 2019:
    Let’s make a constant like CWalletTx::ABANDON_HASH, or rename that one since it’s the same constant…

    achow101 commented at 7:57 pm on December 30, 2019:
    Since this value of one is used in a couple of other places, I’ve added a commit refactor those uses and adding a constant UINT256_ONE to uint256.h. So we use that constant and so do the other places that use that value of one.
  80. in src/wallet/wallet.cpp:3109 in 9020b9f0b1 outdated
    3075@@ -3073,7 +3076,7 @@ size_t CWallet::KeypoolCountExternalKeys()
    3076     AssertLockHeld(cs_wallet);
    


    instagibbs commented at 5:20 pm on December 30, 2019:
    For keypool checks do we want to assert that these are only being used for LegacySPKM?

    achow101 commented at 7:44 pm on December 30, 2019:
    No, they still apply to other SPKMs.
  81. in src/wallet/wallet.cpp:4080 in 9020b9f0b1 outdated
    4075+    return spk_mans;
    4076+}
    4077+
    4078+ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const OutputType& type, bool internal) const
    4079+{
    4080+    if (internal) {
    


    instagibbs commented at 5:35 pm on December 30, 2019:
    could make this function roughly half the code by making a temporary reference spk_managers which points to internal(or not) based on the argument, which avoids code copying/pasting.

    achow101 commented at 7:57 pm on December 30, 2019:
    Done
  82. in src/wallet/wallet.h:1126 in 9020b9f0b1 outdated
    1122@@ -1116,13 +1123,22 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
    1123         LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...);
    1124     };
    1125 
    1126+    //! De-duplicates and returns all ScriptPubKeyMans in m_internal_spk_managers and m_external_spk_managers
    


    instagibbs commented at 5:40 pm on December 30, 2019:
    s/De-duplicates and returns all ScriptPubKeyMans/Returns all unique ScriptPubKeyMans/

    achow101 commented at 7:57 pm on December 30, 2019:
    Done
  83. in src/wallet/rpcwallet.cpp:3308 in 16c36d8581 outdated
    3305         if (provider) {
    3306             providers.insert(std::move(provider));
    3307         }
    3308     }
    3309     if (providers.size() == 0) {
    3310         // When there are no available providers, use DUMMY_SIGNING_PROVIDER so we can check if the tx is complete
    


    instagibbs commented at 6:12 pm on December 30, 2019:
    comment is now out of date

    achow101 commented at 7:57 pm on December 30, 2019:
    Updated
  84. achow101 force-pushed on Dec 30, 2019
  85. achow101 force-pushed on Dec 30, 2019
  86. instagibbs commented at 9:29 pm on January 3, 2020: member
  87. Sjors commented at 6:24 am on January 4, 2020: member

    85901a994ea00a6fa9af44eeaed418b968829f26 Refactor: Allow LegacyScriptPubKeyMan to be null breaks sethdseed for blank wallets, because of the EnsureLegacyScriptPubKeyMan check. None of the tests catch this.

    I’m also seeing "keypoololdest": 1578118989 (instead of 0) for blank wallets, not when created, but after a restart. That may be a pre-existing issue.

  88. in src/outputtype.h:50 in a92b77e82f outdated
    49@@ -47,4 +50,3 @@ std::vector<CTxDestination> GetAllDestinationsForKey(const CPubKey& key);
    50 CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, const CScript& script, OutputType);
    51 
    52 #endif // BITCOIN_OUTPUTTYPE_H
    53-
    


    kallewoof commented at 7:35 am on January 4, 2020:

    In commit a92b77e82f4905e8a0568b3de6b53345356dbf58 (“List output types in an array in order to be iterated over”):

    Nit: leave newline at end of file (or am I misreading this diff?)


    achow101 commented at 6:42 pm on January 6, 2020:

    There’s an extra newline that’s being removed.

    Technically an unnecessary change, but oh well.


    kallewoof commented at 1:50 am on January 7, 2020:
    Oh, I misread then. +1 removing extra NL.
  89. in src/outputtype.cpp:89 in 50145c8ad5 outdated
    86-    case OutputType::LEGACY:
    87-        return ScriptHash(script);
    88+    case OutputType::LEGACY: {
    89+        keystore.AddCScript(GetScriptForDestination(sh));
    90+        return sh;
    91+    }
    


    kallewoof commented at 7:40 am on January 4, 2020:

    In commit 50145c8ad53c604713689ae587badbb0a473471a (“Store p2sh scripts in AddAndGetDestinationForScript”):

    Why add brackets?


    achow101 commented at 7:06 pm on January 6, 2020:
    They were needed in a previous iteration. Removed.
  90. in src/outputtype.cpp:98 in 50145c8ad5 outdated
    94         CTxDestination witdest = WitnessV0ScriptHash(script);
    95         CScript witprog = GetScriptForDestination(witdest);
    96         // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
    97-        if (!IsSolvable(keystore, witprog)) return ScriptHash(script);
    98+        if (!IsSolvable(keystore, witprog)) {
    99+            keystore.AddCScript(GetScriptForDestination(sh));
    


    kallewoof commented at 7:43 am on January 4, 2020:

    In commit 50145c8ad53c604713689ae587badbb0a473471a (“Store p2sh scripts in AddAndGetDestinationForScript”):

    I think this could use a comment explaining why the sh dest is added to keystore when witprog is not solvable.


    achow101 commented at 7:06 pm on January 6, 2020:
    Done
  91. in src/wallet/wallet.cpp:578 in 426ca7efd8 outdated
    572@@ -572,9 +573,9 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
    573         Unlock(strWalletPassphrase);
    574 
    575         // if we are using HD, replace the HD seed with a new one
    576-        if (auto spk_man = m_spk_man.get()) {
    577-            if (spk_man->IsHDEnabled()) {
    578-                if (!spk_man->SetupGeneration(true)) {
    579+        for (const auto& spk_man_pair : m_spk_managers) {
    580+            if (spk_man_pair.second->IsHDEnabled()) {
    581+                if (!spk_man_pair.second->SetupGeneration(true)) {
    


    Sjors commented at 10:22 am on January 4, 2020:

    This is fine while there’s only one (legacy) scriptPubKeyManager, but with descriptor wallets this would cause the seed to be reset three times. In the current descriptor wallet PR there’s a separate code branch for descriptor wallets: https://github.com/bitcoin/bitcoin/pull/16528/files#diff-b2bb174788c7409b671c46ccc86034bdR580

    Instead of a loop, maybe just call the LegacyScriptPubkeyMan?


    achow101 commented at 7:06 pm on January 6, 2020:
    Done
  92. Sjors commented at 11:06 am on January 4, 2020: member

    Other than the above sethdseed issue: code review ACK db4a741d30a44ebd975c905cef0f9578c816e8f2

    Suggestion for followup: add tests for the PSBT related change in dec753da5045be56623ec29155dc4ce5e416e1a5, because I can drop it without breaking any wallet functional test.

  93. instagibbs commented at 7:06 pm on January 4, 2020: member

    I’m also seeing “keypoololdest”: 1578118989 (instead of 0) for blank wallets, not when created, but after a restart. That may be a pre-existing issue.

    From my reading of the code that’s what it’s “supposed” to do when the keypool is empty on master; it will just use current clock time. Help should probably detail that.

  94. in src/uint256.h:147 in 20a5c56d2f outdated
    143@@ -144,4 +144,6 @@ inline uint256 uint256S(const std::string& str)
    144     return rv;
    145 }
    146 
    147+uint256& UINT256_ONE();
    


    kallewoof commented at 10:31 pm on January 4, 2020:
    Why not simply make this extern uint256 UINT256_ONE; instead of a function?

    achow101 commented at 6:52 pm on January 6, 2020:
    Clang didn’t like it. Something about static initialization order resulting in a potential uninitialized object access. There’s a failed travis build you can check for the error.

    kallewoof commented at 1:52 am on January 7, 2020:
    Weird. :/ I’ve seen that once before too. Sounds like a compiler bug tbh.
  95. in src/wallet/rpcwallet.cpp:1879 in 15488dfc3e outdated
    1875@@ -1876,7 +1876,7 @@ static UniValue keypoolrefill(const JSONRPCRequest& request)
    1876     pwallet->TopUpKeyPool(kpSize);
    1877 
    1878     if (pwallet->GetKeyPoolSize() < kpSize) {
    1879-        throw JSONRPCError(RPC_WALLET_ERROR, "Error refreshing keypool.");
    1880+        throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Error refreshing keypool. %d", pwallet->GetKeyPoolSize()));
    


    kallewoof commented at 11:06 pm on January 4, 2020:
    This looks like debug leftovers.

    achow101 commented at 7:06 pm on January 6, 2020:
    Removed
  96. in src/wallet/scriptpubkeyman.h:451 in ea610db77d outdated
    446+class LegacySigningProvider : public SigningProvider
    447+{
    448+private:
    449+    const LegacyScriptPubKeyMan& spk_man;
    450+public:
    451+    LegacySigningProvider(const LegacyScriptPubKeyMan& spk_man) : spk_man(spk_man) {}
    


    kallewoof commented at 11:22 pm on January 4, 2020:
    Call it m_spk_man and avoid a(a)m_spk_man(spk_man) ?

    achow101 commented at 7:06 pm on January 6, 2020:
    Done
  97. kallewoof commented at 11:24 pm on January 4, 2020: member
    Code review ACK
  98. kallewoof commented at 0:34 am on January 5, 2020: member

    @instagibbs

    From my reading of the code that’s what it’s “supposed” to do when the keypool is empty on master; it will just use current clock time. Help should probably detail that.

    I think the problem here is that it is 0 before restart, not that it uses current clock time.

  99. kallewoof commented at 1:18 am on January 5, 2020: member

    Tested ACK db4a741d30a44ebd975c905cef0f9578c816e8f2

    Tested on top of signet patch. Received and sent funds. All seems to work a-OK. (Did not test GUI.)

  100. MarcoFalke removed the label Refactoring on Jan 6, 2020
  101. MarcoFalke removed the label Tests on Jan 6, 2020
  102. achow101 force-pushed on Jan 6, 2020
  103. achow101 commented at 7:07 pm on January 6, 2020: member

    Fixed the sethdseed problem.

    Edit: I changed the fix so that it also has the import* RPCs work with blank wallets too.

  104. achow101 force-pushed on Jan 6, 2020
  105. achow101 force-pushed on Jan 6, 2020
  106. Sjors commented at 7:29 am on January 7, 2020: member

    Code review re-ACK b5afe999206ee6982fd555b490fae6dd50a1c0c3

    I assume we don’t want to add a LegacyScriptPubKeyMan to a (future) descriptor based wallet. That’s working correctly I believe, because EnsureLegacyScriptPubKeyMan(*wallet, true) calls SetupLegacyScriptPubKeyMan which fails if any other ScriptPubKeyMan types exist. But I would prefer if EnsureLegacyScriptPubKeyMan checked this directly.

  107. kallewoof commented at 8:27 am on January 7, 2020: member
    Is b5afe999206ee6982fd555b490fae6dd50a1c0c3 not meant to be squashed into d8a3c7830e1292c8b7d52b5a02fdd1d2459de3ef?
  108. in src/wallet/wallet.cpp:585 in b5afe99920 outdated
    573@@ -569,7 +574,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
    574         Unlock(strWalletPassphrase);
    575 
    576         // if we are using HD, replace the HD seed with a new one
    577-        if (auto spk_man = m_spk_man.get()) {
    578+        if (auto spk_man = GetLegacyScriptPubKeyMan()) {
    


    instagibbs commented at 3:08 pm on January 7, 2020:
    Are we not going to replace the seed for descriptor wallets?

    achow101 commented at 5:05 pm on January 7, 2020:
    No, descriptor wallets have a different way of doing it.

    ryanofsky commented at 6:21 pm on February 5, 2020:

    re: #17261 (review)

    No, descriptor wallets have a different way of doing it.

    In commit “Box the wallet: Add multiple keyman maps and loops” (c729afd0a3b74a3943e4c359270beaf3e6ff8a7b):

    It seems bad to do nothing at all and keep using unencrypted seeds for non-legacy wallets, and the comment here doesn’t here doesn’t explain any legacy/nonlegacy distinction. It would seem safer to trigger some kind of error for non-legacy wallets if planned behavior hasn’t been implemented yet, than to just do nothing.

    If there is supposed to be legacy / non-legacy distinction, it would seem better to move legacy code out of CWallet into LegacyScriptPubKeyMan in a PostEncrypt or similar ScriptPubKeyMan virtual method

  109. instagibbs commented at 3:18 pm on January 7, 2020: member

    EnsureLegacyScriptPubKeyMan(*wallet, true) calls SetupLegacyScriptPubKeyMan which fails if any other ScriptPubKeyMan types exist

    It’ll hit asserts, right? Yeah I’d rather things be explicitly checked and handled instead if possible.

  110. achow101 force-pushed on Jan 7, 2020
  111. achow101 commented at 5:11 pm on January 7, 2020: member

    I assume we don’t want to add a LegacyScriptPubKeyMan to a (future) descriptor based wallet. That’s working correctly I believe, because EnsureLegacyScriptPubKeyMan(*wallet, true) calls SetupLegacyScriptPubKeyMan which fails if any other ScriptPubKeyMan types exist. But I would prefer if EnsureLegacyScriptPubKeyMan checked this directly.

    What do you mean by “checked this directly”? Have the same wallet flag check or something else?

    It’ll hit asserts, right? Yeah I’d rather things be explicitly checked and handled instead if possible.

    The descriptor wallet PR checks the wallet flag and will return early if the descriptor wallet flag is set. No asserts.

    Is b5afe99 not meant to be squashed into d8a3c78?

    I guess it can be. Hadn’t really noticed that.

  112. Sjors commented at 6:26 am on January 8, 2020: member

    The descriptor wallet PR checks the wallet flag and will return early if the descriptor wallet flag is set. No asserts.

    It’s good that EnsureLegacyScriptPubKeyMan never gets called for a descriptor wallet, but it would be better if it explicitly checked that there is no other ScriptPubKeyMan out there. Currently it only checks that there’s no LegacyScriptPubKeyMan out there, but then hit an assert later on if it wasn’t the only ScriptPubKeyMan.

  113. achow101 commented at 5:34 pm on January 10, 2020: member

    The descriptor wallet PR checks the wallet flag and will return early if the descriptor wallet flag is set. No asserts.

    It’s good that EnsureLegacyScriptPubKeyMan never gets called for a descriptor wallet, but it would be better if it explicitly checked that there is no other ScriptPubKeyMan out there. Currently it only checks that there’s no LegacyScriptPubKeyMan out there, but then hit an assert later on if it wasn’t the only ScriptPubKeyMan.

    I don’t quite understand what you want me to do here. We don’t have any other ScriptPubKeyMans yet, so we can’t check if it is something else. And if there are other ScriptPubKeyMans in a wallet that has a LegacyScriptPubKeyMan, then it definitely should assert and crash because that’s not a safe or consistent state for the wallet to be in.

  114. Sjors commented at 2:20 am on January 15, 2020: member

    Instead of:

    0 LegacyScriptPubKeyMan* spk_man = nullptr;
    1 if (also_create) {
    2     spk_man = wallet.GetOrCreateLegacyScriptPubKeyMan();
    3 } else {
    4     spk_man = wallet.GetLegacyScriptPubKeyMan();
    5 }
    

    This seems more clear:

    0 LegacyScriptPubKeyMan* spk_man = wallet.GetLegacyScriptPubKeyMan();
    1 if (!spk_man && also_create) {
    2     spk_man = wallet.GetOrCreateLegacyScriptPubKeyMan();
    3 }
    

    And instead of:

    0void CWallet::SetupLegacyScriptPubKeyMan()
    1{
    2    if (m_internal_spk_managers.empty() && m_external_spk_managers.empty() && m_spk_managers.empty()) {
    3        auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new LegacyScriptPubKeyMan(*this));
    

    Try:

    0void CWallet::SetupLegacyScriptPubKeyMan()
    1{
    2    if(m_internal_spk_managers.count(OutputType::LEGACY) > 0) {
    3        return;
    4    }
    5    assert(m_internal_spk_managers.empty() && m_external_spk_managers.empty() && m_spk_managers.empty());
    6    auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new LegacyScriptPubKeyMan(*this));
    

    (this top if statement might be avoidable by having a separate CreateLegacyScriptPubKeyMan())

  115. DrahtBot added the label Needs rebase on Jan 15, 2020
  116. achow101 force-pushed on Jan 15, 2020
  117. achow101 commented at 5:40 pm on January 15, 2020: member
    Rebased. @Sjors Done. I changed your suggestion for SetupLegacyScriptPubKeyMan to check whether any of the *spk_managers maps has a ScriptPubKeyMan.
  118. DrahtBot removed the label Needs rebase on Jan 15, 2020
  119. in src/wallet/rpcwallet.cpp:128 in cb90d1b6f9 outdated
    123@@ -124,9 +124,12 @@ void EnsureWalletIsUnlocked(const CWallet* pwallet)
    124     }
    125 }
    126 
    127-LegacyScriptPubKeyMan& EnsureLegacyScriptPubKeyMan(CWallet& wallet)
    128+LegacyScriptPubKeyMan& EnsureLegacyScriptPubKeyMan(CWallet& wallet, bool also_create)
    


    instagibbs commented at 8:30 pm on January 15, 2020:
    What’s the general usage for this new boolean? I see it’s set to true for most importing, but not for addmultisigaddress for instance. A guiding comment on usage would be helpful.

    achow101 commented at 11:44 pm on January 15, 2020:

    Added a comment.

    It’s to be used for RPCs that make a blank wallet no longer blank. addmultisigaddress wasn’t doing this before, although I suppose the expected behavior of it with a blank wallet isn’t really known (i.e. what do we want it to do?).


    instagibbs commented at 11:49 pm on January 15, 2020:
    Hm, yeah it would be a weird non-blank wallet. I’m fine as-is for now.
  120. in src/outputtype.cpp:104 in 28cea92c22 outdated
    102         keystore.AddCScript(witprog);
    103         if (type == OutputType::BECH32) {
    104             return witdest;
    105         } else {
    106-            return ScriptHash(witprog);
    107+            ScriptHash wsh = ScriptHash(witprog);
    


    instagibbs commented at 8:36 pm on January 15, 2020:
    micro-nit: wsh is a bit of a minsnomer, it’s sh_w, or something

    achow101 commented at 11:44 pm on January 15, 2020:
    Done
  121. in src/wallet/scriptpubkeyman.cpp:387 in c5842c171b outdated
    383@@ -384,7 +384,7 @@ bool LegacyScriptPubKeyMan::Upgrade(int prev_version, std::string& error)
    384         hd_upgrade = true;
    385     }
    386     // Upgrade to HD chain split if necessary
    387-    if (m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) {
    388+    if (m_storage.CanSupportFeature(FEATURE_HD_SPLIT) && hdChain.nVersion < 2 /* VERSION_HD_CHAIN_SPLIT */) {
    


    instagibbs commented at 8:39 pm on January 15, 2020:
    any way to actually use the constant rather than referring to it and hoping the value never changes?

    achow101 commented at 11:45 pm on January 15, 2020:

    Yes. the proper header is being imported now, so no need for this hack. Changed it.

    Also, the value should never change because its persisted to wallet files and changing it would break backwards compatibility.

  122. achow101 force-pushed on Jan 15, 2020
  123. Sjors commented at 12:38 pm on January 16, 2020: member
    Code review ACK bbf50a006410bd3e56908983664c5a1a0b475c2c
  124. DrahtBot added the label Needs rebase on Jan 17, 2020
  125. achow101 force-pushed on Jan 17, 2020
  126. achow101 commented at 2:10 am on January 17, 2020: member
    Rebased
  127. DrahtBot removed the label Needs rebase on Jan 17, 2020
  128. Sjors commented at 9:12 am on January 17, 2020: member
    re-ACK c985aea9be828c0a49eb81f5a5d54b42147ef20c; just a rebase with merged #17843 content dropped.
  129. in src/wallet/wallet.cpp:2996 in 6c953ae84f outdated
    2992@@ -2993,10 +2993,9 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
    2993         }
    2994     }
    2995 
    2996+    // This wallet is in its first run if all of these are empty and this isn't blank or no privkeys
    


    meshcollider commented at 9:23 am on January 17, 2020:

    Comment doesn’t make sense referring to “all of these are empty” now.

    EDIT: it makes slightly more sense after later commit, but it would be better to explicitly say “all the SPK managers are empty”


    achow101 commented at 1:48 pm on January 17, 2020:
    Edited the comment.
  130. in src/wallet/scriptpubkeyman.cpp:1058 in 0c4cb49560 outdated
    1054@@ -1056,7 +1055,7 @@ bool LegacyScriptPubKeyMan::NewKeyPool()
    1055         if (!TopUp()) {
    1056             return false;
    1057         }
    1058-        WalletLogPrintf("LegacyScriptPubKeyMan::NewKeyPool rewrote keypool\n");
    1059+        WalletLogPrintf("CWallet::NewKeyPool rewrote keypool\n");
    


    meshcollider commented at 9:29 am on January 17, 2020:
    I don’t understand the purpose of this change, more explanation pls?

    achow101 commented at 1:49 pm on January 17, 2020:
    On further examination, that entire commit doesn’t make sense, so I’ve dropped it.
  131. in src/wallet/wallet.cpp:4195 in b4c48fbd56 outdated
    4187@@ -4135,7 +4188,26 @@ LegacyScriptPubKeyMan* CWallet::GetOrCreateLegacyScriptPubKeyMan()
    4188 
    4189 void CWallet::SetupLegacyScriptPubKeyMan()
    4190 {
    4191-    if (!m_spk_man) m_spk_man = MakeUnique<LegacyScriptPubKeyMan>(*this);
    4192+    if (!m_internal_spk_managers.empty() || !m_external_spk_managers.empty() || !m_spk_managers.empty()) {
    4193+        return;
    4194+    }
    4195+
    4196+    assert(m_internal_spk_managers.empty() && m_external_spk_managers.empty() && m_spk_managers.empty());
    


    meshcollider commented at 10:02 am on January 17, 2020:
    This assert seems a little unnecessary considering the if-statement above

    achow101 commented at 1:49 pm on January 17, 2020:
    Removed
  132. in src/wallet/wallet.cpp:4210 in b4c48fbd56 outdated
    4206+    ScriptPubKeyMan* spk_man = m_internal_spk_managers.at(OutputType::LEGACY);
    4207+    for (const auto& type : OUTPUT_TYPES) {
    4208+        assert(m_internal_spk_managers.at(type) == spk_man);
    4209+        assert(m_external_spk_managers.at(type) == spk_man);
    4210+    }
    4211+    assert(m_spk_managers.size() == 1);
    


    meshcollider commented at 10:10 am on January 17, 2020:
    In what case would these asserts ever be hit, directly following the setup code? Is there a scenario you can think of?

    achow101 commented at 1:49 pm on January 17, 2020:
    I don’t think so. I’ve removed these asserts.

    achow101 commented at 5:25 pm on January 17, 2020:
    I think a better solution would be to have this return a bool, but that’s an improvement we can make in the future as it’s really a belt-and-suspenders check. And also have this check sameness check somewhere.
  133. in src/wallet/wallet.cpp:3913 in b4c48fbd56 outdated
    3909@@ -3906,7 +3910,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
    3910         // No need to read and scan block if block was created before
    3911         // our wallet birthday (as adjusted for block time variability)
    3912         Optional<int64_t> time_first_key;
    3913-        if (auto spk_man = walletInstance->m_spk_man.get()) {
    3914+        for (auto spk_man : walletInstance->GetActiveScriptPubKeyMans()) {
    


    meshcollider commented at 10:15 am on January 17, 2020:
    Why only check the “active” SPKMans here?

    achow101 commented at 1:49 pm on January 17, 2020:
    Changed to check all.

    instagibbs commented at 5:48 pm on January 17, 2020:
    not sure this was needed, but it’s a safe change
  134. meshcollider commented at 10:18 am on January 17, 2020: contributor

    Strong/obvious concept ACK. I’m excited to get this merged soon. I’ve done an initial review:

    • (didn’t review in depth, just read through it) 0bceafb874dbbc2994de1fe10d37782ffdcb0e55 Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman
    • 6c953ae84f4272d036602d5c7cf19456d9f39863 Refactor: Allow LegacyScriptPubKeyMan to be null
    • 14f7284eb52dafa9fa36955f39296e31bf534cdd List output types in an array in order to be iterated over
    • 97eac193ed167ba48c2b57fd16a1b52170fc87dc Always try to sign for all pubkeys in multisig
    • ec0d430be8888c60e763c7c12a545674eb876c40 Store p2sh scripts in AddAndGetDestinationForScript
    • 9941be111049cd9463ad69ba5a4f1e9fbaa97d48 Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan
    • d27e13f5050b1ed24a49e351a2fa607297104794 HD Split: Avoid redundant upgrades
    • de37afd27b5407e742797cdb35f935f9fabfd03a refactor: define a UINT256_ONE global constant
    • b4c48fbd5602f57e9bc59de83c6c256fdd0f2d71 Box the wallet: Add multiple keyman maps and loops
    • f62eaa952f24c54dcc63353fb06a61b979b56577 Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan
    • 0c4cb4956003c83706c812dfd699f5b22ab58d44 Cleanup: Update message strings and comments
    • 8b08ca79f19c65681a3acbcc236c6fb516169a45 Cleanup: Drop unused GUI learnRelatedScripts method
    • c985aea9be828c0a49eb81f5a5d54b42147ef20c Refactor: Replace SigningProvider pointers with unique_ptrs

    A couple of comments inline. I’ll test this out soon too.

  135. achow101 force-pushed on Jan 17, 2020
  136. instagibbs approved
  137. Sjors commented at 11:23 am on January 19, 2020: member
    re-utACK 126d6945e38cdc47b1227cdacfaa456714efcdf5
  138. in src/outputtype.h:31 in 126d6945e3 outdated
    27@@ -27,6 +28,8 @@ enum class OutputType {
    28     CHANGE_AUTO,
    29 };
    30 
    31+const std::array<OutputType, 3> OUTPUT_TYPES = {OutputType::LEGACY, OutputType::P2SH_SEGWIT, OutputType::BECH32};
    


    laanwj commented at 7:03 pm on January 20, 2020:
    Doesn’t defining the value of non-primitive constants in the header file potentially cause linker issues in C++ (or duplication, as this literal data will be included in multiple files)?. I think it’s better to only declare the variable here then initialize the value in the cpp.

    achow101 commented at 10:01 pm on January 20, 2020:
    I don’t think this would cause any issues since it’s const. I also haven’t seen any linker warnings or errors about this. I think there would only be some duplication, but isn’t that the same for any constant global variable that is defined in a header?

    kallewoof commented at 0:28 am on January 21, 2020:

    If you are fine trusting the word of Microsoft,

    In C, constant values default to external linkage, so they can appear only in source files. In C++, constant values default to internal linkage, which allows them to appear in header files.

    Another source said C++ does not normally create storage for consts, which I assume means they work like #defines, in which case there shouldn’t be wasted duplicate data entries either.


    sipa commented at 7:39 pm on January 23, 2020:
    If you replace the {…} initializer with a function call, and then have the constant in multiple translation units, the function will get called once for each.

    achow101 commented at 9:58 pm on January 23, 2020:
    I moved the initialization to the cpp file
  139. wallet: Improve CWallet:MarkDestinationsDirty f5be479694
  140. Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman
    This commit only affects locking behavior and doesn't have other changes.
    fadc08ad94
  141. Refactor: Allow LegacyScriptPubKeyMan to be null
    In CWallet::LoadWallet, use this to detect and empty wallet with no keys
    
    This commit does not change behavior.
    eb81fc3ee5
  142. List output types in an array in order to be iterated over 81610eddbc
  143. Always try to sign for all pubkeys in multisig 501acb5538
  144. Store p2sh scripts in AddAndGetDestinationForScript 4a7e43e846
  145. Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan 01b4511206
  146. HD Split: Avoid redundant upgrades
    This avoids repeaded upgrades when support for more multiple keyman references
    is added in the next commit:
    https://github.com/bitcoin/bitcoin/pull/16341#discussion_r322370108
    415afcccd3
  147. refactor: define a UINT256_ONE global constant
    Instead of having a uint256 representations of one scattered throughout
    where it is used, define it globally in uint256.h
    4977c30d59
  148. Box the wallet: Add multiple keyman maps and loops
    Add wallet logic for dealing with multiple ScriptPubKeyMan instances. This
    doesn't change current behavior because there is still only a single
    LegacyScriptPubKeyMan. But in the future the new logic will be used to support
    descriptor wallets.
    c729afd0a3
  149. Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan
    This commit does not change behavior.
    e2f02aa59e
  150. Cleanup: Drop unused GUI learnRelatedScripts method
    This commit does not change behavior.
    3afe53c403
  151. Refactor: Replace SigningProvider pointers with unique_ptrs
    Needed for future ScriptPubKeyMans which may need to create
    SigningProviders dynamically and thus a normal pointer is not enough
    
    This commit does not change behavior.
    3f373659d7
  152. achow101 force-pushed on Jan 23, 2020
  153. Sjors commented at 8:38 am on January 24, 2020: member
    re-utACK 3f373659d732a5b1e5fdc692a45b2b8179f66bec (it still compiles on macOS after #17261 (review))
  154. fanquake requested review from meshcollider on Jan 29, 2020
  155. meshcollider commented at 4:20 am on January 30, 2020: contributor

    Tested re-ACK 3f373659d732a5b1e5fdc692a45b2b8179f66bec

    I think this is ready to go in

  156. meshcollider merged this on Jan 30, 2020
  157. meshcollider closed this on Jan 30, 2020

  158. fanquake removed this from the "Blockers" column in a project

  159. fanquake referenced this in commit 44c2400bcc on Jan 30, 2020
  160. sidhujag referenced this in commit e55d4dec7c on Feb 1, 2020
  161. ryanofsky referenced this in commit db55f65109 on Feb 4, 2020
  162. in src/wallet/scriptpubkeyman.cpp:387 in 415afcccd3 outdated
    383@@ -384,7 +384,7 @@ bool LegacyScriptPubKeyMan::Upgrade(int prev_version, std::string& error)
    384         hd_upgrade = true;
    385     }
    386     // Upgrade to HD chain split if necessary
    387-    if (m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) {
    388+    if (m_storage.CanSupportFeature(FEATURE_HD_SPLIT) && CHDChain::VERSION_HD_CHAIN_SPLIT) {
    


    ryanofsky commented at 6:08 pm on February 5, 2020:

    In commit “HD Split: Avoid redundant upgrades” (415afcccd3e5583defdb76e3a280f48e98983301):

    This seems buggy and the change looks like it has no effect. Is this supposed to say && hdChain.nVersion < CHDChain::VERSION_HD_CHAIN_SPLIT?

  163. ryanofsky commented at 6:57 pm on February 5, 2020: member

    Post-merge code review ACK 3f373659d732a5b1e5fdc692a45b2b8179f66bec. I did post two questions in review comments below that I think would be good to resolve.

    Changes since last review (72df6f1050d27a13435fab77f20c1a0e08fdbf51):

    • Dropped commit daf6d548de3efdfdb974bb52948f6086fb0e2ecc “Set state to WATCH_ONLY if we can get the pubkey”
    • Added EnsureLegacyScriptPubKeyMan create argument to make RPCs like sethdseed work on empty wallets
    • coinselector_tests setup fix
    • OUTPUT_TYPES definition move
    • AddAndGetDestinationForScript formatting tweaks
    • Split commit 01b4511206e399981a77976deb15785d18db46ae “Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan” out of commit 264917ff4343b678007d89373aba6e99d521001c “Add multiple keyman maps and loops”
    • LegacyScriptPubKeyMan::Upgrade hd split version check no longer added
    • Replacement hd seed generation skipped in CWallet::EncryptWallet for nonlegacy wallets
    • CWallet::GetAllScriptPubKeyMans method added and used by CreateWalletFromFile to not skip inactive keyman instances when figuring out first key time
    • CWallet::GetScriptPubKeyMan internal true/false code deduped
    • CWallet::SetupLegacyScriptPubKeyMan asserts removed
    • LegacySigningProvider m_spk_man member rename and comment tweak
  164. ryanofsky referenced this in commit 005f8a92cc on Feb 12, 2020
  165. ryanofsky referenced this in commit 267e4c967b on Feb 12, 2020
  166. meshcollider referenced this in commit 68e841e0af on Feb 19, 2020
  167. sidhujag referenced this in commit 3a89bee259 on Feb 19, 2020
  168. fanquake moved this from the "PRs" to the "Done" column in a project

  169. meshcollider referenced this in commit dcf2ccbfde on Mar 9, 2020
  170. sidhujag referenced this in commit 98c8a84b4c on Mar 10, 2020
  171. HashUnlimited referenced this in commit d44cd4f051 on Apr 17, 2020
  172. deadalnix referenced this in commit 85f266319f on Oct 2, 2020
  173. deadalnix referenced this in commit b820c50ea9 on Oct 9, 2020
  174. deadalnix referenced this in commit e238f8c4ba on Oct 10, 2020
  175. jasonbcox referenced this in commit 1bdcd8044d on Oct 10, 2020
  176. jasonbcox referenced this in commit 82c314ca28 on Oct 10, 2020
  177. deadalnix referenced this in commit 132148cc92 on Oct 10, 2020
  178. deadalnix referenced this in commit fa5ca6b092 on Oct 10, 2020
  179. jasonbcox referenced this in commit 343d199ad1 on Oct 10, 2020
  180. jasonbcox referenced this in commit dfe2ebd571 on Oct 10, 2020
  181. jasonbcox referenced this in commit e37acaefee on Oct 10, 2020
  182. deadalnix referenced this in commit 0345bf8805 on Oct 10, 2020
  183. deadalnix referenced this in commit f6a97bd620 on Oct 10, 2020
  184. deadalnix referenced this in commit e6d44e7f11 on Oct 10, 2020
  185. fanquake locked this on Oct 10, 2020

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-12-18 00:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me