…s involving PubKeys

a57a1d4 test: add unit test for wallet watch-only methods involving PubKeys (Sebastian Falbesoner)

Pull request description:

  The motivation for this addition was to unit test the function `wallet.cpp:ExtractPubKey()` (see recent change in commit 798a589) which is however static and only indirectly available via the public methods `AddWatchOnly()`, `LoadWatchOnly()` and `RemoveWatchOnly()`. Since the first of those methods also stores the addresses to the disk, the second, simpler one was chosen which only operates in memory.

ACKs for top commit:
  Sjors:
    ACK a57a1d4
  instagibbs:
    reACK bitcoin@a57a1d4
  Sjors:
    re-ACK a57a1d4

Tree-SHA512: 92a242204ab533022cd848662997372c41815b1265d07b3d96305697f801db29a5ba5668337faf4bea702bec1451972529afd6665927fb142aaf91700a338b26

…some CWallet functions into new LegacyScriptPubKeyMan

Moved:
- GetLastBlockHeight
- SetLastBlockProcessed

It is needed as a preparation step before bitcoin#17261

Changes in this commit are required as a preparation to bitcoin#17261
Method GenerateNewHDChainEncrypted moved back from LegacyScriptManager to CWallet
This methods should not be moved before in bitcoin#17260.

Also added 2 new methods in interface WalletStorage: NewKeyPoolCallback and KeepDestinationCallback

… and fix signing bug

e13fea9 Add regression test for PSBT signing bug bitcoin#14473 (Glenn Willen)
5655005 Refactor PSBTInput signing to enforce invariant (Glenn Willen)
0f5bda2 Simplify arguments to SignPSBTInput (Glenn Willen)
53e6fff Add bool PSBTInputSigned (Glenn Willen)
65166d4 New PartiallySignedTransaction constructor from CTransction (Glenn Willen)
4f3f5cb Remove redundant txConst parameter to FillPSBT (Glenn Willen)
fe5d22b More concise conversion of CDataStream to string (Glenn Willen)

Pull request description:

  As discussed in the comments on bitcoin#14473, I think that bug was caused primarily by failure to adhere to the invariant that a PSBTInput always has exactly one of the two utxo fields present -- an invariant that is already enforced by PSBTInput::IsSane, but which we were temporarily suspending during signing.

  This refactor repairs the invariant, also fixing the bug. It also simplifies some other code, and removes redundant parameters from some related functions.

  fixes bitcoin#14473

Tree-SHA512: cbad3428175e30f9b7bac3f600668dd1a8f9acde16b915d27a940a2fa6d5149d4fbe236d5808fd590fb20a032274c99e8cac34bef17f79a53fdf69a5948c0fd0

… wallet to have multiple

3f37365 Refactor: Replace SigningProvider pointers with unique_ptrs (Andrew Chow)
3afe53c Cleanup: Drop unused GUI learnRelatedScripts method (Andrew Chow)
e2f02aa Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan (Andrew Chow)
c729afd Box the wallet: Add multiple keyman maps and loops (Andrew Chow)
4977c30 refactor: define a UINT256_ONE global constant (Andrew Chow)
415afcc HD Split: Avoid redundant upgrades (Andrew Chow)
01b4511 Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan (Andrew Chow)
4a7e43e Store p2sh scripts in AddAndGetDestinationForScript (Andrew Chow)
501acb5 Always try to sign for all pubkeys in multisig (Andrew Chow)
81610ed List output types in an array in order to be iterated over (Andrew Chow)
eb81fc3 Refactor: Allow LegacyScriptPubKeyMan to be null (Andrew Chow)
fadc08a Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman (Andrew Chow)
f5be479 wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)

Pull request description:

  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 `ScriptPubKeyMan`s.

  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](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes).

ACKs for top commit:
  instagibbs:
    re-utACK bitcoin@3f37365
  Sjors:
    re-utACK 3f37365 (it still compiles on macOS after bitcoin#17261 (comment))
  meshcollider:
    Tested re-ACK 3f37365

Tree-SHA512: f8e2b8d9efa750b617691e8702d217ec4c33569ec2554a060141d9eb9b9a3a5323e4216938e2485c44625d7a6e0925d40dea1362b3af9857cf08860c2f344716

…Provider

1115ba6 psbt_wallet_tests: use unique_ptr for GetSigningProvider (Anthony Towns)

Pull request description:

  bitcoin#17261 changed GetSigningProvider to return a unique_ptr, but bitcoin#17156 made psbt_wallet_tests use it as well, and wasn't correspondingly updated.

ACKs for top commit:
  fanquake:
    ACK 1115ba6
  meshcollider:
    Thanks! utACK 1115ba6

Tree-SHA512: f0191c9b00780e6d1445fa4ec531456758b468b5bca8660474d22b1edb5f48a636a940656c9bdbe466b8bffad7af1e57e0756239906e901d60c69c3124d3bff4

…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

… possible

79facb1 wallet: use constant CWallets in rpcwallet.cpp (Karl-Johan Alm)
d9b0ebc wallet: make ReserveDestination pwallet ivar const (Karl-Johan Alm)
57c569e wallet: make BackupWallet() const (Karl-Johan Alm)
df3a818 wallet: make getters const (Karl-Johan Alm)
227b9dd wallet/spkm: make GetOldestKeyPoolTime() const (Karl-Johan Alm)
22d329a wallet: use constant CWallets in rpcdump.cpp (Karl-Johan Alm)
7b3587b wallet/db: make IsDummy() const (Karl-Johan Alm)
d366795 wallet/db: make Backup() const (Karl-Johan Alm)
8cd0b86 wallet: make CanGetAddresses() const (Karl-Johan Alm)
037fa77 wallet: make KeypoolCountExternalKeys() const (Karl-Johan Alm)
ddc9355 wallet: make CanGenerateKeys() const (Karl-Johan Alm)
dc2d065 make BlockUntilSyncedToCurrentChain() const (Karl-Johan Alm)

Pull request description:

  A lot of places refer to `CWallet*`'s as `CWallet * const`, which translates to *"an immutable pointer to a mutable `CWallet` instance"*; this is

  1. often not what the author meant, especially as a lot of these places do not at all modify the wallet object, and
  2. confusing, as it tends to suggest that this is a proper way to refer to a constant `CWallet` instance.

  This PR changes references to wallets to `const CWallet* const` whenever immutability is expected. This should result in no behavioral changes at all, and improved compile-time error checking.

  Note from irc:

  > <sipa> sounds good to me; this is the sort of change that as long as it compiles, the behavior shouldn't change
  > <sipa> though in general it may lead to introducing automatic copying of objects sometimes (e.g. trying to std::move a const object will work, but generally result in a copy rather than an efficient move)
  > <sipa> CWallet objects aren't copied or moved though

ACKs for top commit:
  laanwj:
    ACK 79facb1
  Empact:
    ACK bitcoin@79facb1
  promag:
    ACK 79facb1.
  fjahr:
    ACK 79facb1

Tree-SHA512: 80a80c1a52f0f788d0ccb268b53bc0f46c796643a3c5a22b55bbbde4ffa6c7e347784e5e53b1e488a3b4e14399e31d5be9417ad5b6319c74a462609e9b1a98e8

…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

it doesn't work cause LegacySigningProvider doesn't provide privekeys

…s/CreateWallet

e1e68b6 test: Fix inconsistent lock order in wallet_tests/CreateWallet (Hennadii Stepanov)
cb23fe0 sync: Check precondition in LEAVE_CRITICAL_SECTION() macro (Hennadii Stepanov)
c5e3e74 sync: Improve CheckLastCritical() (Hennadii Stepanov)

Pull request description:

  This PR:
  - fixes bitcoin#19049 that was caused by bitcoin#16426
  - removes `wallet_tests::CreateWallet` suppression from the `test/sanitizer_suppressions/tsan`

  The example of the improved `CheckLastCritical()`/`LEAVE_CRITICAL_SECTION()` log (could be got when compiled without the last commit):
  ```
  2020-09-20T08:34:28.429485Z [test] INCONSISTENT LOCK ORDER DETECTED
  2020-09-20T08:34:28.429493Z [test] Current lock order (least recent first) is:
  2020-09-20T08:34:28.429501Z [test]  'walletInstance->cs_wallet' in wallet/wallet.cpp:4007 (in thread 'test')
  2020-09-20T08:34:28.429508Z [test]  'cs_wallets' in wallet/wallet.cpp:4089 (in thread 'test')
  ```

  Currently, there are other "naked" `LEAVE_CRITICAL_SECTION()` in the code base:
  https://github.com/bitcoin/bitcoin/blob/b99a1633b270e0e89479b2bb2ae19a8a8dc0fa05/src/rpc/mining.cpp#L698

  https://github.com/bitcoin/bitcoin/blob/b99a1633b270e0e89479b2bb2ae19a8a8dc0fa05/src/checkqueue.h#L208

ACKs for top commit:
  MarcoFalke:
    review ACK e1e68b6 💂
  ryanofsky:
    Code review ACK e1e68b6. Just trivial rebase and suggested switch to BOOST_CHECK_EXCEPTION since last review
  vasild:
    ACK e1e68b6

Tree-SHA512: a627680eac3af4b4c02772473d68322ce8d3811bf6b035d3485ccc97d35755bef933cffabd3f20b126f89e3301eccecec3f769df34415fb7c426c967b6ce36e6

…DDRv2

Fix compilation error for C++20 for new code. Added missing changes from this commit:
 - fe42411 test: move HasReason so it can be reused

…expr/ and remove template (followup to bitcoin#19845)

    added missing changes after 19845 was re-done

89836a8 style: minor improvements as a followup to bitcoin#19845 (Vasil Dimov)

Pull request description:

  Address suggestions:
  bitcoin#19845 (comment)
  bitcoin#19845 (comment)
  bitcoin#19845 (comment)

ACKs for top commit:
  jonatack:
    re-ACK 89836a8 change since previous review is replacing std::runtime_error with std::exception, built/ran unit tests with gcc debian 10.2.0-15, then broke a few v3 net_tests involving `BOOST_CHECK_EXCEPTION`, rebuilt, ran `src/test/test_bitcoin -t net_tests -l all` and checked the error reporting.
  hebasto:
    re-ACK 89836a8
  theStack:
    ACK 89836a8

Tree-SHA512: 36477fdccabe5a8ad91fbabb4655cc363a3a7ca237a98ae6dd4a9fae4a4113762040f864d4ca13a47d081f7d16e5bd487edbfb61ab50a37e4a0424e9bec30b24

ConnectScriptPubKeyManNotifiers is a no-op otherwise

partially revert 1a5a1ca, I believe this part is not needed anymore