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

achow101 · 2019-10-26T05:16:00Z

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.

achow101 · 2019-10-26T05:17:46Z

The final diff is very similar to #16341 just with some things dropped as they were deemed unnecessary.

Sjors · 2019-10-26T08:35:58Z

For comparison, my last ACK on the original was for c37be15. That code was identical to @ryanofsky's branch at 299296e, just using a different set of commits to get there. This PR rebases that on master @25d7e2e78137d07eb612c44d19b0d496050c947a, with the last two commits (952ba99 & 299296e) dropped. The result is f47ffe0 here (minus two linter changes).

DrahtBot · 2019-10-26T11:55:59Z

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.

Suggested by MarcoFalke <falke.marco@gmail.com> bitcoin#17260 (comment) The same check was also added in the followup commit "Refactor: Move SetupGeneration code out of CWallet" 9727a4d from bitcoin#17261, but it's safer to start checking now without waiting for that followup.

ryanofsky · 2019-10-29T18:39:58Z

achow101 · 2019-10-29T19:03:56Z

@ryanofsky Done as #17304

maflcko · 2019-10-29T20:15:09Z

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.

fanquake · 2019-10-29T20:24:32Z

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

…

On Tue, 29 Oct 2019 at 16:15, MarcoFalke ***@***.***> 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 <#17261?email_source=notifications&email_token=AAGS34SKBDHCRX7QCH2LWVLQRCKWDA5CNFSM4JFL4BW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECR5VXY#issuecomment-547609311>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGS34RIQ5JLGSG4BKV3SCTQRCKWDANCNFSM4JFL4BWQ> .

maflcko · 2019-10-30T00:36:21Z

#17260 (comment) was in for this week, but fair enough

Make LegacyScriptPubKeyMan::CanProvide method able to recognize p2sh scripts when the redeem script is present in the mapScripts map without the p2sh script also having to be added to the mapScripts map. This restores behavior prior to bitcoin#17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before bitcoin#17261 as solvable. The reason why tests didn't fail with the CanProvide implementation in bitcoin#17261 is because of a workaround added in 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files. This change adds a lot of comments and allows reverting commit 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", so the AddAndGetDestinationForScript() function, CanProvide() method, and mapScripts map should all be more comprehensible

ryanofsky

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

Changes since last review (72df6f1):

Dropped commit daf6d54 "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 01b4511 "Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan" out of commit 264917f "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

ryanofsky · 2020-02-05T18:08:25Z

src/wallet/scriptpubkeyman.cpp

@@ -384,7 +384,7 @@ bool LegacyScriptPubKeyMan::Upgrade(int prev_version, std::string& error)
        hd_upgrade = true;
    }
    // Upgrade to HD chain split if necessary
-    if (m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) {
+    if (m_storage.CanSupportFeature(FEATURE_HD_SPLIT) && CHDChain::VERSION_HD_CHAIN_SPLIT) {


In commit "HD Split: Avoid redundant upgrades" (415afcc):

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 · 2020-02-05T18:21:00Z

src/wallet/wallet.cpp

@@ -569,7 +574,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
        Unlock(strWalletPassphrase);

        // if we are using HD, replace the HD seed with a new one
-        if (auto spk_man = m_spk_man.get()) {
+        if (auto spk_man = GetLegacyScriptPubKeyMan()) {


re: #17261 (comment)

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

In commit "Box the wallet: Add multiple keyman maps and loops" (c729afd):

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

Make LegacyScriptPubKeyMan::CanProvide method able to recognize p2sh scripts when the redeem script is present in the mapScripts map without the p2sh script also having to be added to the mapScripts map. This restores behavior prior to bitcoin#17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before bitcoin#17261 as solvable. The reason why tests didn't fail with the CanProvide implementation in bitcoin#17261 is because of a workaround added in 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files. This change adds a lot of comments and allows reverting commit 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", so the AddAndGetDestinationForScript() function, CanProvide() method, and mapScripts map should all be more comprehensible

…t recognition a304a36 Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky) eb7d8a5 [test] check for addmultisigaddress regression (Sjors Provoost) 005f8a9 wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky) Pull request description: Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable. The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files. This change adds a lot of comments and allows reverting commit 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible ACKs for top commit: Sjors: re-ACK a304a36 (rebase, slight text changes and my test) achow101: re-ACK a304a36 meshcollider: utACK a304a36 Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e

…e script recognition a304a36 Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky) eb7d8a5 [test] check for addmultisigaddress regression (Sjors Provoost) 005f8a9 wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky) Pull request description: Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to bitcoin#17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before bitcoin#17261 as solvable. The reason why tests didn't fail with the CanProvide implementation in bitcoin#17261 is because of a workaround added in 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files. This change adds a lot of comments and allows reverting commit 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible ACKs for top commit: Sjors: re-ACK a304a36 (rebase, slight text changes and my test) achow101: re-ACK a304a36 meshcollider: utACK a304a36 Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e

…nstead of exporting the private keys d2774c0 Clear any input_errors for an input after it is signed (Andrew Chow) dc17488 Replace GetSigningProvider with GetSolvingProvider (Andrew Chow) 6a9c429 Move direct calls to MessageSign into new SignMessage functions in CWallet and ScriptPubKeyMan (Andrew Chow) 82a30fa Move key and script filling and signing from CWallet::FillPSBT to ScriptPubKeyMan::FillPSBT (Andrew Chow) 3d70dd9 Move FillPSBT to be a member of CWallet (Andrew Chow) a4af324 Use CWallet::SignTransaction in CreateTransaction and signrawtransactionwithwallet (Andrew Chow) f37de92 Implement CWallet::SignTransaction using ScriptPubKeyMan::SignTransaction (Andrew Chow) d999dd5 Add SignTransaction function to ScriptPubKeyMan and LegacyScriptPubKeyMan (Andrew Chow) 2c52b59 Refactor rawtransaction's SignTransaction into generic SignTransaction function (Andrew Chow) Pull request description: Following #17261, the way to sign transactions, PSBTs, and messages was to use `GetSigningProvider()` and get a `SigningProvider` containing the private keys. However this may not be feasible for future `ScriptPubKeyMan`s, such as for hardware wallets. Instead of exporting a `SigningProvider` containing private keys, we need to pass these things into the `ScriptPubKeyMan` (via `CWallet`) so that they can do whatever is needed internally to sign them. This is largely a refactor as the logic of processing transactions, PSBTs, and messages for is moved into `LegacyScriptPubKeyMan` and `CWallet` instead of being handled by the caller (e.g. `signrawtransaction`). To help with this, I've refactored the 3(!) implementations of a `SignTransaction()` function into one generic one. This function will be called by `signrawtransactionwithkey` and `LegacyScriptPubKeyMan::SignTransaction()`. `CWallet::CreateTransaction()` is changed to call `CWallet::SignTransaction()` which in turn, calls `LegacyScriptPubKeyMan::SignTransaction()`. Other `ScriptPubKeyMan`s may implement `SignTransaction()` differently. `FillPSBT()` is moved to be a member function of `CWallet` and the `psbtwallet.cpp/h` files removed. It is further split so that `CWallet` handles filling the UTXOs while the `ScriptPubKeyMan` handles adding keys, derivation paths, scripts, and signatures. In the end `LegacyScriptPubKeyMan::FillPSBT` still calls `SignPSBTInput`, but the `SigningProvider` is internal to `LegacyScriptPubKeyMan`. Other `ScriptPubKeyMan`s may do something different. A new `SignMessage()` function is added to both `CWallet` and `ScriptPubKeyMan`. Instead of having the caller (i.e. `signmessage` or the sign message dialog) get the private key, hash the message, and sign, `ScriptPubKeyMan` will now handle that (`CWallet` passes through to the `ScriptPubKeyMan`s as it does for many functions). This signing code is thus consolidated into `LegacyScriptPubKeyMan::SignMessage()`, though other `ScriptPubKeyMan`s may implement it differently. Additionally, a `SigningError` enum is introduced for the different errors that we expect to see from `SignMessage()`. Lastly, `GetSigningProvider()` is renamed to `GetPublicSigningProvider()`. It will now only provide pubkeys, key origins, and scripts. `LegacySigningProvider` has it's `GetKey` and `HaveKey` functions changed to only return false. Future implementations should return `HidingSigningProvider`s where private keys are hidden. Other things like `dumpprivkey` and `dumpwallet` are not changed because they directly need and access the `LegacyScriptPubKeyMan` so are not relevant to future changes. ACKs for top commit: instagibbs: reACK d2774c0 Sjors: re-utACK d2774c0 meshcollider: re-utACK d2774c0 Tree-SHA512: 89c83e7e7e9315e283fae145a2264648a9d7f7ace8f3281cb3f44f0b013c988d67ba4fa9726e50c643c0ed921bdd269adaec984840d11acf4a681f3e8a582cc1

…gning instead of exporting the private keys d2774c0 Clear any input_errors for an input after it is signed (Andrew Chow) dc17488 Replace GetSigningProvider with GetSolvingProvider (Andrew Chow) 6a9c429 Move direct calls to MessageSign into new SignMessage functions in CWallet and ScriptPubKeyMan (Andrew Chow) 82a30fa Move key and script filling and signing from CWallet::FillPSBT to ScriptPubKeyMan::FillPSBT (Andrew Chow) 3d70dd9 Move FillPSBT to be a member of CWallet (Andrew Chow) a4af324 Use CWallet::SignTransaction in CreateTransaction and signrawtransactionwithwallet (Andrew Chow) f37de92 Implement CWallet::SignTransaction using ScriptPubKeyMan::SignTransaction (Andrew Chow) d999dd5 Add SignTransaction function to ScriptPubKeyMan and LegacyScriptPubKeyMan (Andrew Chow) 2c52b59 Refactor rawtransaction's SignTransaction into generic SignTransaction function (Andrew Chow) Pull request description: Following bitcoin#17261, the way to sign transactions, PSBTs, and messages was to use `GetSigningProvider()` and get a `SigningProvider` containing the private keys. However this may not be feasible for future `ScriptPubKeyMan`s, such as for hardware wallets. Instead of exporting a `SigningProvider` containing private keys, we need to pass these things into the `ScriptPubKeyMan` (via `CWallet`) so that they can do whatever is needed internally to sign them. This is largely a refactor as the logic of processing transactions, PSBTs, and messages for is moved into `LegacyScriptPubKeyMan` and `CWallet` instead of being handled by the caller (e.g. `signrawtransaction`). To help with this, I've refactored the 3(!) implementations of a `SignTransaction()` function into one generic one. This function will be called by `signrawtransactionwithkey` and `LegacyScriptPubKeyMan::SignTransaction()`. `CWallet::CreateTransaction()` is changed to call `CWallet::SignTransaction()` which in turn, calls `LegacyScriptPubKeyMan::SignTransaction()`. Other `ScriptPubKeyMan`s may implement `SignTransaction()` differently. `FillPSBT()` is moved to be a member function of `CWallet` and the `psbtwallet.cpp/h` files removed. It is further split so that `CWallet` handles filling the UTXOs while the `ScriptPubKeyMan` handles adding keys, derivation paths, scripts, and signatures. In the end `LegacyScriptPubKeyMan::FillPSBT` still calls `SignPSBTInput`, but the `SigningProvider` is internal to `LegacyScriptPubKeyMan`. Other `ScriptPubKeyMan`s may do something different. A new `SignMessage()` function is added to both `CWallet` and `ScriptPubKeyMan`. Instead of having the caller (i.e. `signmessage` or the sign message dialog) get the private key, hash the message, and sign, `ScriptPubKeyMan` will now handle that (`CWallet` passes through to the `ScriptPubKeyMan`s as it does for many functions). This signing code is thus consolidated into `LegacyScriptPubKeyMan::SignMessage()`, though other `ScriptPubKeyMan`s may implement it differently. Additionally, a `SigningError` enum is introduced for the different errors that we expect to see from `SignMessage()`. Lastly, `GetSigningProvider()` is renamed to `GetPublicSigningProvider()`. It will now only provide pubkeys, key origins, and scripts. `LegacySigningProvider` has it's `GetKey` and `HaveKey` functions changed to only return false. Future implementations should return `HidingSigningProvider`s where private keys are hidden. Other things like `dumpprivkey` and `dumpwallet` are not changed because they directly need and access the `LegacyScriptPubKeyMan` so are not relevant to future changes. ACKs for top commit: instagibbs: reACK bitcoin@d2774c0 Sjors: re-utACK d2774c0 meshcollider: re-utACK d2774c0 Tree-SHA512: 89c83e7e7e9315e283fae145a2264648a9d7f7ace8f3281cb3f44f0b013c988d67ba4fa9726e50c643c0ed921bdd269adaec984840d11acf4a681f3e8a582cc1

Make LegacyScriptPubKeyMan::CanProvide method able to recognize p2sh scripts when the redeem script is present in the mapScripts map without the p2sh script also having to be added to the mapScripts map. This restores behavior prior to bitcoin#17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before bitcoin#17261 as solvable. The reason why tests didn't fail with the CanProvide implementation in bitcoin#17261 is because of a workaround added in 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files. This change adds a lot of comments and allows reverting commit 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", so the AddAndGetDestinationForScript() function, CanProvide() method, and mapScripts map should all be more comprehensible

Summary: Revert D7674 and D7691 (since it built on top of the prior). D7674 is failing all sanitizer builds and appears to be adding flakiness to `ninja check` in some configurations. There's hints that backporting up to [[bitcoin/bitcoin#17261 | PR17261]] may fix one or more of these problems. Until then, we should revert as it's making diff tests unreliable and incredibly noisy, as well as zero reliability on sanitizer builds. I opted to revert them together as one patch in order to give us the option of un-reverting them both as a single change (by reverting this change). This reverts commit 78322df and 28efd9a Test Plan: ``` ninja all check check-functional ``` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D7729

…in legacy keyman Summary: This commit only affects locking behavior and doesn't have other changes. bitcoin/bitcoin@fadc08a --- Partial backport of Core [[bitcoin/bitcoin#17261 | PR17261]] Test Plan: cmake -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS=-Werror ninja all check check-functional Reviewers: #bitcoin_abc, Fabien, deadalnix Reviewed By: #bitcoin_abc, Fabien, deadalnix Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D7838

Summary: In CWallet::LoadWallet, use this to detect and empty wallet with no keys This commit does not change behavior. --- bitcoin/bitcoin@eb81fc3 Depends on D7838 Partial backport of Core [[bitcoin/bitcoin#17261 | PR17261]] Test Plan: export CC=clang CXX=clang++; cmake -GNinja -DCMAKE_BUILD_TYPE=Debug ninja all check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D7848

…erated over Summary: bitcoin/bitcoin@81610ed Depends on D7848 Partial backport of Core [[bitcoin/bitcoin#17261 | PR17261]] Test Plan: ninja Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7849

Summary: bitcoin/bitcoin@501acb5 Depends on D7849 Partial backport of Core [[bitcoin/bitcoin#17261 | PR17261]] --- Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7850

…ript Summary: bitcoin/bitcoin@4a7e43e Depends on D7850 Partial backport of Core [[bitcoin/bitcoin#17261 | PR17261]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7851

…ptPubKeyMan Summary: bitcoin/bitcoin@01b4511 Depends on D7851 Partial backport of Core [[bitcoin/bitcoin#17261 | PR17261]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7854

Summary: This avoids repeaded upgrades when support for more multiple keyman references is added in the next commit: bitcoin/bitcoin#16341 (comment) bitcoin/bitcoin@415afcc Depends on D7854 Partial backport of Core [[bitcoin/bitcoin#17261 | PR17261]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7855

Summary: Instead of having a uint256 representations of one scattered throughout where it is used, define it globally in uint256.h bitcoin/bitcoin@4977c30 Depends on D7855 Partial backport of Core [[bitcoin/bitcoin#17261 | PR17261]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7858

…oops Summary: 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. --- bitcoin/bitcoin@c729afd Note: Had to bring in [[bitcoin/bitcoin#18052 | PR18052]] to fix an instance of -Wmaybe-uninitialized Depends on D7858 Partial backport of Core [[bitcoin/bitcoin#17261 | PR17261]] and [[bitcoin/bitcoin#18052 | PR18052]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7859

…ion to LegacyScriptPubKeyMan Summary: This commit does not change behavior. --- bitcoin/bitcoin@e2f02aa Depends on D7859 Partial backport of Core [[bitcoin/bitcoin#17261 | PR17261]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7860

…ethod Summary: This commit does not change behavior. bitcoin/bitcoin@3afe53c Depends on D7860 Partial backport of Core [[bitcoin/bitcoin#17261 | PR17261]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7861

…h unique_ptrs Summary: 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. bitcoin/bitcoin@3f37365 Depends on D7861 Concludes backport of Core [[bitcoin/bitcoin#17261 | PR17261]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7863

achow101 force-pushed the wallet-box-pr-2 branch from 1b62c69 to c81a129 Compare October 26, 2019 05:52

DrahtBot added Build system GUI RPC/REST/ZMQ Tests Utils/log/libs Wallet labels Oct 26, 2019

fanquake added Refactoring and removed Build system GUI RPC/REST/ZMQ Utils/log/libs labels Oct 26, 2019

Sjors mentioned this pull request Oct 26, 2019

Split some CWallet functions into new LegacyScriptPubKeyMan #17260

Merged

achow101 force-pushed the wallet-box-pr-2 branch from f47ffe0 to 4f559f7 Compare October 26, 2019 16:24

ryanofsky mentioned this pull request Oct 29, 2019

LegacyScriptPubKeyMan code cleanups #17300

Merged

achow101 force-pushed the wallet-box-pr-2 branch from 4f559f7 to 7250fab Compare October 29, 2019 18:22

Sjors mentioned this pull request Oct 29, 2019

External signer support - Wallet Box edition #16546

Merged

2 tasks

achow101 force-pushed the wallet-box-pr-2 branch from 7250fab to f3c3e9e Compare October 29, 2019 19:03

ryanofsky mentioned this pull request Oct 29, 2019

refactor: Move many functions into LegacyScriptPubKeyMan and further separate it from CWallet #17304

Merged

DrahtBot added the Needs rebase label Oct 30, 2019

achow101 force-pushed the wallet-box-pr-2 branch from f3c3e9e to 35b0f7b Compare October 30, 2019 16:12

This was referenced Feb 4, 2020

wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition #18067

Merged

wallet: #17261 regression: addmultisigaddress addresses not treated as solvable #18075

Closed

ryanofsky reviewed Feb 5, 2020

View reviewed changes

achow101 mentioned this pull request Feb 11, 2020

wallet: Pass in transactions and messages for signing instead of exporting the private keys #18115

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

ryanofsky mentioned this pull request Apr 27, 2020

Add fee_est tool for debugging fee estimation code #10443

Closed

achow101 mentioned this pull request Jun 12, 2020

[RFC] Wallet Class Structure Changes #16165

Closed

ajtowns mentioned this pull request Sep 25, 2020

uint256: 1 is a constant #20016

Merged

bitcoin locked and limited conversation to collaborators Oct 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

achow101 commented Oct 26, 2019 •

edited

Loading

Uh oh!

achow101 commented Oct 26, 2019

Uh oh!

Sjors commented Oct 26, 2019 •

edited

Loading

Uh oh!

DrahtBot commented Oct 26, 2019 •

edited

Loading

Uh oh!

ryanofsky commented Oct 29, 2019 •

edited

Loading

Uh oh!

achow101 commented Oct 29, 2019

Uh oh!

maflcko commented Oct 29, 2019 •

edited

Loading

Uh oh!

fanquake commented Oct 29, 2019 via email

Uh oh!

maflcko commented Oct 30, 2019

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Feb 5, 2020

Uh oh!

ryanofsky Feb 5, 2020

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

achow101 commented Oct 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Oct 26, 2019

Uh oh!

Sjors commented Oct 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Oct 29, 2019

Uh oh!

maflcko commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Oct 29, 2019 via email

Uh oh!

maflcko commented Oct 30, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Oct 26, 2019 •

edited

Loading

Sjors commented Oct 26, 2019 •

edited

Loading

DrahtBot commented Oct 26, 2019 •

edited

Loading

ryanofsky commented Oct 29, 2019 •

edited

Loading

maflcko commented Oct 29, 2019 •

edited

Loading