0b75a7f wallet: Reuse existing batch in CWallet::SetUsedDestinationState (João Barbosa)
01f45dd wallet: Avoid recursive lock in CWallet::SetUsedDestinationState (João Barbosa)

Pull request description:

  This PR makes 2 distinct changes around `CWallet::SetUsedDestinationState`:
   - 1st the recursive lock is removed and now it requires the lock to be held;
   - 2nd change is to support, in the best case, just a wallet database flush when transaction is added to the wallet.

ACKs for top commit:
  achow101:
    ACK 0b75a7f
  MarcoFalke:
    ACK 0b75a7f
  ryanofsky:
    Code review ACK 0b75a7f. Code changes looks fine but PR description should be updated to say what benefits of the change are. I might have missed something, but I didn't see a place where multiple batches were used previously and a single batch was used now. So the main benefit of this change appears to be removing a recursive lock? And maybe moving toward a consistent convention for passing batch instances?

Tree-SHA512: abcf23a5850d29990668db20d6f624cca3e89629cc9ed003e0d05cde1b58ab2ff365034f156684ad13e55764b54c6c0c2bc7d5f96b8af7dc5e45a3be955d6b15

…ningProvider

d0dab89 Refactor: Require scriptPubKey to get wallet SigningProvider (Andrew Chow)
4b0c718 Accumulate result UniValue in SignTransaction (Andrew Chow)

Pull request description:

  Easier to review ignoring whitespace:

      git log -p -n1 -w

  This commit does not change behavior. It passes new CScript arguments to
  signing functions, but the arguments aren't currently used.

  Split from bitcoin#17261

ACKs for top commit:
  instagibbs:
    utACK bitcoin@d0dab89
  ryanofsky:
    Code review ACK d0dab89. Thanks for the SignTransaction update. No other changes since last review
  Sjors:
    Code review ACK d0dab89
  promag:
    Code review ACK d0dab89.
  meshcollider:
    Code review ACK d0dab89

Tree-SHA512: c3f52df20fd9d6b3b5aa65562cf5f7dce7b7f44c148b0f988f8b578fce2a28e9b7bf010f5f04bb5bf60f5272b2899f1dbbfb8aee81579c21c9cba559d1d2bb70

3958295 wallet: LearnRelatedScripts only if KeepDestination (João Barbosa)
55295fb wallet: Lock address type in ReserveDestination (João Barbosa)

Pull request description:

  Only mutates the wallet if the reserved key is kept.

  First commit is a refactor that makes the address type a class member.

  The second commit moves `LearnRelatedScripts` from `GetReservedDestination` to `KeepDestination` to avoid an unnecessary call to `AddCScript` - which in turn prevents multiple entries of the same script in the wallet DB.

ACKs for top commit:
  achow101:
    Re-ACK 3958295
  Sjors:
    ACK 3958295
  ryanofsky:
    Code review ACK 3958295. I like this change. The new behavior makes more sense, and the change makes the code clearer, since the current LearnRelatedScripts call is hard to understand and explain. (Personally, I'd like it if this PR were merged before bitcoin#17373 or that PR was rebased on top of this one so it would be less confusing.)
  meshcollider:
    utACK 3958295

Tree-SHA512: 49a5f4b022b28042ad37ea309b28378a3983cb904e234a25795b5a360356652e0f8e60f15e3e64d85094ea63af9be01812d90ccfc08ca4f1dd927fdd8566e33f

…ndency

3ed5e68 refactor: Nuke coincontrol circular dependency (Hennadii Stepanov)

Pull request description:

  This PR gets rid of `wallet/coincontrol` -> `wallet/wallet` -> `wallet/coincontrol` circular dependency.

ACKs for top commit:
  Sjors:
    re-ACK 3ed5e68
  meshcollider:
    utACK 3ed5e68

Tree-SHA512: 7fbceb74a9cd04157170df158d2deb979cf397df818376b478224d2423f1d8504a8688e3a9b8fc527da73e4a34ab6bc4a98be0cc2937e102a063ab2ac553e86d

… in AddrToPubKey

1a3a256 wallet: replace raw pointer with const reference in AddrToPubKey (Harris)

Pull request description:

  This PR replaces a redundant reference-to-pointer conversion in **addmultisigaddress** from *wallet/rpcwallet.cpp*. It also makes the API from *rpc/util.h* look more straightforward as **AddrToPubKey** now uses const references like other functions from there.

  I am not sure why there is a ref-to-ptr conversion in addmultisignatures, so I can only speculate that this is because of "historical reasons".

  The ref-to-ptr conversion happens here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L1001

  There, the address of LegacyScriptPubKeyMan& is given to AddrToPubKey.

  Later, in AddrToPubKey, it gets converted back to a reference, because GetKeyForDestination in rpc/util.cpp expects a const ref: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/util.cpp#L140

  Regards,

ACKs for top commit:
  achow101:
    ACK 1a3a256
  meshcollider:
    utACK 1a3a256
  promag:
    Code review ACK 1a3a256.
  hebasto:
    ACK 1a3a256, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

Tree-SHA512: 1a2b8ddab5694ef4c65fac69f011e38dd03a634e84a35857e13bd05ad99fe42af22ee0af6230865e3d2c725693512f3336acb055ede19c958424283e7a3856c4

…dling in LegacyScriptPubKeyMan and CWallet

886f173 Key pool: Fix omitted pre-split count in GetKeyPoolSize (Andrew Chow)
386a994 Key pool: Change ReturnDestination interface to take address instead of key (Andrew Chow)
ba41aa4 Key pool: Move LearnRelated and GetDestination calls (Andrew Chow)
65833a7 Add OutputType and CPubKey parameters to KeepDestination (Andrew Chow)
9fcf8ce Rename Keep/ReturnKey to Keep/ReturnDestination and remove the wrapper (Andrew Chow)
596f646 Key pool: Move CanGetAddresses call (Andrew Chow)

Pull request description:

  * The `pwallet->CanGetAddresses()` call in `ReserveDestination::GetReservedDestination` to `LegacyScriptPubKeyMan::GetReservedDestination` so that the sanity check results in a failure when a `ScriptPubKeyMan` individually cannot get a destination, not when any of the `ScriptPubKeyMan`s can't.
  * `ScriptPubKeyMan::GetReservedDestination` is changed to return the destination so that future `ScriptPubKeyMan`s can return destinations constructed in other ways. This is implemented for `LegacyScriptPubKeyMan` by moving key-to-destination code from `CWallet` to `LegacyScriptPubKeyMan`
  * In order for `ScriptPubKeyMan` to be generic and work with future `ScriptPubKeyMan`s, `ScriptPubKeyMan::ReturnDestination` is changed to take a `CTxDestination` instead of a `CPubKey`. Since `LegacyScriptPubKeyMan` still deals with keys internally, a new map `m_reserved_key_to_index` is added in order to track the keypool indexes that have been reserved.
  * A bug is fixed in how the total keypool size is calculated as it was omitting `set_pre_split_keypool` which is a bug.

  Split from bitcoin#17261

ACKs for top commit:
  ryanofsky:
    Code review ACK 886f173. Only change is moving earlier fix to a better commit (same end result).
  promag:
    Code review ACK 886f173.
  instagibbs:
    code review re-ACK bitcoin@886f173
  Sjors:
    Code review re-ACK 886f173

Tree-SHA512: f4be290759f63fdc920d5c02bd0d09acc4b06a5f053787d4afcd3c921b2e35d2bd97617fadae015da853dc189f559fb8d2c6e58d53e4cabfac9af151cd97ad19

Current implementation works until bitcoin#17369 backported
because it changes logic of related IsCrypted() function not fully ompatible way

…d Wallet

    7cecf10 Replace LegacyScriptPubKeyMan::IsCrypted with LegacyScriptPubKeyMan::HasEncryptionKeys (Andrew Chow)
    bf64171 Remove SetCrypted() and fUseCrypto; Change IsCrypted()'s implementation (Andrew Chow)
    77a7771 Rename EncryptKeys to Encrypt and pass in the encrypted batch to use (Andrew Chow)
    35f962f Clear mapKeys before encrypting (Andrew Chow)
    14b5efd Move fDecryptionThoroughlyChecked from CWallet to LegacyScriptPubKeyMan (Andrew Chow)
    97c0374 Move Unlock implementation to LegacyScriptPubKeyMan (Andrew Chow)
    e576b13 Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey() (Andrew Chow)
    fd9d6ee Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage (Andrew Chow)

Pull request description:

  Let wallet class handle locked/unlocked status and master key, and let keyman
  handle encrypting its data and determining whether there is encrypted data.

  There should be no change in behavior, but state is tracked differently. The
  fUseCrypto atomic bool is eliminated and replaced with equivalent
  HasEncryptionKeys checks.

  Split from bitcoin#17261

ACKs for top commit:
  laanwj:
    ACK 7cecf10

Tree-SHA512: 95a997c366ca539abba0c0a7a0015f39d27b55220683d8d86344ff2d926db4724da67700d2c8ec2d82ed75d07404318c6cb81544af8aadeefab312167257e673

…fluous TopUp()s

6e77a7b keypool: Add comment about TopUp and when to use it (Andrew Chow)
ea50e34 keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow)
bb2c8ce keypool: Remove  superfluous topup from CWallet::GetNewChangeDestination (Andrew Chow)

Pull request description:

  * The `TopUp()` in `CWallet::GetNewChangeDestination` is unnecessary as currently m_spk_man calls TopUp further down the call stack inside LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination). This also lets us prepare for future changes with multiple ScriptPubKeyMans in the wallet.
  * An opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::GetNewDestination` to `CWallet::GetNewDestination`.
  * Another opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::ReserveKeyFromKeyPool`

  Moving opportunistic TopUps ensures that ScriptPubKeyMans will always be topped up before requesting Destinations from them as we cannot  always rely on future ScriptPubKeyMan implementaions topping up internally.

  See also: bitcoin#17373 (comment)

ACKs for top commit:
  instagibbs:
    utACK bitcoin@6e77a7b only change is slight elaboration on comment
  ryanofsky:
    Code review ACK 6e77a7b. Only the comment changed since my previous review.

Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f

…key address

0950245 IsUsedDestination should count any known single-key address (Gregory Sanders)

Pull request description:

  This plugs the privacy leak detailed at bitcoin#17605, at least for the single-key case.

ACKs for top commit:
  meshcollider:
    Code Review ACK 0950245

Tree-SHA512: e1d68281675f05072b3087171cba1df9416a69c9ccf70c72e8555e55eadda2d0fd339e5a894e3a3438ff94b9e3827fb19b8b701faade70c08756b19ff157ee0c

…n's implementation

Changed relationship between a functino `DecryptHDChain` and `vMasterKey`
Removed unused GetEncryptionKeyMutable()