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.
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.
achow101 force-pushed
on Oct 26, 2019
DrahtBot added the label
Build system
on Oct 26, 2019
DrahtBot added the label
GUI
on Oct 26, 2019
DrahtBot added the label
RPC/REST/ZMQ
on Oct 26, 2019
DrahtBot added the label
Tests
on Oct 26, 2019
DrahtBot added the label
Utils/log/libs
on Oct 26, 2019
DrahtBot added the label
Wallet
on Oct 26, 2019
fanquake removed the label
Build system
on Oct 26, 2019
fanquake removed the label
GUI
on Oct 26, 2019
fanquake removed the label
RPC/REST/ZMQ
on Oct 26, 2019
fanquake removed the label
Utils/log/libs
on Oct 26, 2019
fanquake added the label
Refactoring
on Oct 26, 2019
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).
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.
achow101 force-pushed
on Oct 26, 2019
fanquake added this to the "Blockers" column in a project
ryanofsky referenced this in commit
aa0e6d8ab1
on Oct 29, 2019
achow101 force-pushed
on Oct 29, 2019
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:
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.
achow101 force-pushed
on Oct 29, 2019
achow101
commented at 7:03 pm on October 29, 2019:
member
MarcoFalke
commented at 0:36 am on October 30, 2019:
member
#17260#event-2751955233 was in for this week, but fair enough
DrahtBot added the label
Needs rebase
on Oct 30, 2019
achow101 force-pushed
on Oct 30, 2019
DrahtBot removed the label
Needs rebase
on Oct 30, 2019
DrahtBot added the label
Needs rebase
on Oct 31, 2019
ryanofsky
commented at 4:51 pm on November 4, 2019:
member
In case its useful, rebased branch pr/keyman9e776722b902d8f6d095d06053b12746065baa46 -> 6ff0b88d5a98465eb79a02ceb7a46afb31d43708 (pr/keyman.9 -> pr/keyman.10) after #17300 and #17304
ryanofsky
commented at 5:59 pm on November 4, 2019:
member
Updated branch pr/keyman6ff0b88d5a98465eb79a02ceb7a46afb31d43708 -> 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.
achow101 force-pushed
on Nov 4, 2019
achow101 force-pushed
on Nov 4, 2019
achow101
commented at 6:31 pm on November 4, 2019:
member
Rebased
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):
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)
in
src/wallet/scriptpubkeyman.h:299
in
90287119f4outdated
291@@ -292,8 +292,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
292293 void MarkUnusedAddresses(const CScript& script) override;
294295- //! 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.
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.
achow101 force-pushed
on Nov 4, 2019
DrahtBot removed the label
Needs rebase
on Nov 4, 2019
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
“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.
ryanofsky added this to the "PRs" column in a project
achow101 force-pushed
on Nov 5, 2019
DrahtBot added the label
Needs rebase
on Nov 6, 2019
achow101 force-pushed
on Nov 6, 2019
DrahtBot removed the label
Needs rebase
on Nov 7, 2019
in
src/wallet/scriptpubkeyman.cpp:281
in
3499674136outdated
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.)
You mean #17373, not #17237 right? If so I’ll review the first.
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.
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.
in
src/wallet/wallet.cpp:3109
in
9020b9f0b1outdated
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
in
src/wallet/wallet.h:1126
in
9020b9f0b1outdated
1122@@ -1116,13 +1123,22 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
1123 LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...);
1124 };
11251126+ //! 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
in
src/wallet/rpcwallet.cpp:3308
in
16c36d8581outdated
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
achow101 force-pushed
on Dec 30, 2019
achow101 force-pushed
on Dec 30, 2019
instagibbs
commented at 9:29 pm on January 3, 2020:
member
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.
They were needed in a previous iteration. Removed.
in
src/outputtype.cpp:98
in
50145c8ad5outdated
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.
in
src/wallet/wallet.cpp:578
in
426ca7efd8outdated
572@@ -572,9 +573,9 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
573 Unlock(strWalletPassphrase);
574575 // 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 11:06 am on January 4, 2020:
member
Other than the above sethdseed issue: code review ACKdb4a741d30a44ebd975c905cef0f9578c816e8f2
Suggestion for followup: add tests for the PSBT related change in dec753da5045be56623ec29155dc4ce5e416e1a5, because I can drop it without breaking any wallet functional test.
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.
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.
in
src/wallet/rpcwallet.cpp:1879
in
15488dfc3eoutdated
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.
kallewoof
commented at 1:18 am on January 5, 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.
kallewoof
commented at 8:27 am on January 7, 2020:
member
Is b5afe999206ee6982fd555b490fae6dd50a1c0c3 not meant to be squashed into d8a3c7830e1292c8b7d52b5a02fdd1d2459de3ef?
in
src/wallet/wallet.cpp:585
in
b5afe99920outdated
573@@ -569,7 +574,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
574 Unlock(strWalletPassphrase);
575576 // 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?
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
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.
achow101 force-pushed
on Jan 7, 2020
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.
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.
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.
Sjors
commented at 2:20 am on January 15, 2020:
member
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.
DrahtBot added the label
Needs rebase
on Jan 17, 2020
achow101 force-pushed
on Jan 17, 2020
achow101
commented at 2:10 am on January 17, 2020:
member
Rebased
DrahtBot removed the label
Needs rebase
on Jan 17, 2020
Sjors
commented at 9:12 am on January 17, 2020:
member
re-ACKc985aea9be828c0a49eb81f5a5d54b42147ef20c; just a rebase with merged #17843 content dropped.
in
src/wallet/wallet.cpp:2996
in
6c953ae84foutdated
2992@@ -2993,10 +2993,9 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
2993 }
2994 }
29952996+ // 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.
in
src/wallet/scriptpubkeyman.cpp:1058
in
0c4cb49560outdated
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.
in
src/wallet/wallet.cpp:3913
in
b4c48fbd56outdated
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
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.
achow101 force-pushed
on Jan 17, 2020
instagibbs approved
instagibbs
commented at 5:50 pm on January 17, 2020:
member
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:
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.
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:
Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman
This commit only affects locking behavior and doesn't have other changes.
fadc08ad94
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
List output types in an array in order to be iterated over81610eddbc
Always try to sign for all pubkeys in multisig501acb5538
Store p2sh scripts in AddAndGetDestinationForScript4a7e43e846
Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan01b4511206
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
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
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
Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan
This commit does not change behavior.
e2f02aa59e
Cleanup: Drop unused GUI learnRelatedScripts method
This commit does not change behavior.
3afe53c403
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
achow101 force-pushed
on Jan 23, 2020
instagibbs
commented at 10:15 pm on January 23, 2020:
member
fanquake removed this from the "Blockers" column in a project
fanquake referenced this in commit
44c2400bcc
on Jan 30, 2020
sidhujag referenced this in commit
e55d4dec7c
on Feb 1, 2020
ryanofsky referenced this in commit
db55f65109
on Feb 4, 2020
in
src/wallet/scriptpubkeyman.cpp:387
in
415afcccd3outdated
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?
ryanofsky
commented at 6:57 pm on February 5, 2020:
member
Post-merge code review ACK3f373659d732a5b1e5fdc692a45b2b8179f66bec. 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
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: 2025-04-02 18:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
