Merge #18787: wallet: descriptor wallet release notes and cleanups

ca2a09640fe976b1e74a33d29d9381895e71b347 Change SetType to SetInternal and remove m_address_type (Andrew Chow)
89b1ce1140535b4c902a7c5999bed335b9ddfe7c Remove unimplemented SetCrypted from DescriptorScriptPubKeyMan (Andrew Chow)
b9073c8f13fb0ba94c2ec6365666343e19fd9ddf rpc: createwallet warning that descriptor wallets are experimental (Andrew Chow)
610030d95c60ea526440d801a98ac8bd370eac48 docs: Add release notes for descriptor wallets (Andrew Chow)

Pull request description:

  Some docs and cleanup following #16528.

  * Added release notes to explain a bit of motivation for descriptor wallets, what was changed, and how users will be effected by it. Also mentions the caveats regarding multsigs and watchonly that we have discussed on IRC.
  * Adds a warning to `createwallet` that descriptor wallets are experimental.
  * Removed unused `SetCrypted` as suggestioned: https://github.com/bitcoin/bitcoin/pull/16528#discussion_r415300916
  * Removed `m_address_type` as mentioned in https://github.com/bitcoin/bitcoin/pull/18782#issuecomment-620167077

ACKs for top commit:
  Sjors:
    tACK ca2a09640fe976b1e74a33d29d9381895e71b347
  instagibbs:
    utACK ca2a09640f
  meshcollider:
    utACK ca2a09640fe976b1e74a33d29d9381895e71b347

Tree-SHA512: 987188a912c191430e5d3f89bcef54ba6773692fc2d95b16a3ec11d9007ded210466ed980a3857e8b7196beef6422f07f9c85cc157f996c02d16f4dbde2e7b2a

doc/release-notes-16528.md

@@ -0,0 +1,119 @@
+Wallet
+------
+
+### Experimental Descriptor Wallets
+
+Please note that Descriptor Wallets are still experimental and not all expected functionality
+is available. Additionally there may be some bugs and current functions may change in the future.
+Bugs and missing functionality can be reported to the [issue tracker](https://github.com/dashpay/dash/issues).
+
+v21 introduces a new type of wallet - Descriptor Wallets. Descriptor Wallets store
+scriptPubKey information using descriptors. This is in contrast to the Legacy Wallet
+structure where keys are used to generate scriptPubKeys and addresses. Because of this
+shift to being script based instead of key based, many of the confusing things that Legacy
+Wallets do are not possible with Descriptor Wallets. Descriptor Wallets use a definition
+of "mine" for scripts which is simpler and more intuitive than that used by Legacy Wallets.
+Descriptor Wallets also uses different semantics for watch-only things and imports.
+
+As Descriptor Wallets are a new type of wallet, their introduction does not affect existing wallets.
+Users who already have a Dash Core wallet can continue to use it as they did before without
+any change in behavior. Newly created Legacy Wallets (which is the default type of wallet) will
+behave as they did in previous versions of Dash Core.
+
+The differences between Descriptor Wallets and Legacy Wallets are largely limited to non user facing
+things. They are intended to behave similarly except for the import/export and watchonly functionality
+as described below.
+
+#### Creating Descriptor Wallets
+
+Descriptor Wallets are not created by default. They must be explicitly created using the
+`createwallet` RPC or via the GUI. A `descriptors` option has been added to `createwallet`.
+Setting `descriptors` to `true` will create a Descriptor Wallet instead of a Legacy Wallet.
+
+In the GUI, a checkbox has been added to the Create Wallet Dialog to indicate that a
+Descriptor Wallet should be created.
+
+Without those options being set, a Legacy Wallet will be created instead.
+
+#### `IsMine` Semantics
+
+`IsMine` refers to the function used to determine whether a script belongs to the wallet.
+This is used to determine whether an output belongs to the wallet. `IsMine` in Legacy Wallets
+returns true if the wallet would be able to sign an input that spends an output with that script.
+Since keys can be involved in a variety of different scripts, this definition for `IsMine` can
+lead to many unexpected scripts being considered part of the wallet.
+
+With Descriptor Wallets, descriptors explicitly specify the set of scripts that are owned by
+the wallet. Since descriptors are deterministic and easily enumerable, users will know exactly
+what scripts the wallet will consider to belong to it. Additionally the implementation of `IsMine`
+in Descriptor Wallets is far simpler than for Legacy Wallets. Notably, in Legacy Wallets, `IsMine`
+allowed for users to take one type of address (e.g. P2PKH), mutate it into another address type
+and the wallet would still detect outputs sending to the new address type
+even without that address being requested from the wallet. Descriptor Wallets does not
+allow for this and will only watch for the addresses that were explicitly requested from the wallet.
+
+These changes to `IsMine` will make it easier to reason about what scripts the wallet will
+actually be watching for in outputs. However for the vast majority of users, this change is
+largely transparent and will not have noticeable effect.
+
+#### Imports and Exports
+
+In Legacy Wallets, raw scripts and keys could be imported to the wallet. Those imported scripts
+and keys are treated separately from the keys generated by the wallet. This complicates the `IsMine`
+logic as it has to distinguish between spendable and watchonly.
+
+Descriptor Wallets handle importing scripts and keys differently. Only complete descriptors can be
+imported. These descriptors are then added to the wallet as if it were a descriptor generated by
+the wallet itself. This simplifies the `IsMine` logic so that it no longer has to distinguish
+between spendable and watchonly. As such, the watchonly model for Descriptor Wallets is also
+different and described in more detail in the next section.
+
+To import into a Descriptor Wallet, a new `importdescriptors` RPC has been added that uses a syntax
+similar to that of `importmulti`.
+
+As Legacy Wallets and Descriptor Wallets use different mechanisms for storing and importing scripts and keys
+the existing import RPCs have been disabled for descriptor wallets.
+New export RPCs for Descriptor Wallets have not yet been added.
+
+The following RPCs are disabled for Descriptor Wallets:
+
+* importprivkey
+* importpubkey
+* importaddress
+* importwallet
+* importelectrumwallet
+* dumpprivkey
+* dumpwallet
+* dumphdinfo
+* importmulti
+* addmultisigaddress
+
+#### Watchonly Wallets
+
+A Legacy Wallet contains both private keys and scripts that were being watched.
+Those watched scripts would not contribute to your normal balance. In order to see the watchonly
+balance and to use watchonly things in transactions, an `include_watchonly` option was added
+to many RPCs that would allow users to do that. However it is easy to forget to include this option.
+
+Descriptor Wallets move to a per-wallet watchonly model. Instead an entire wallet is considered to be
+watchonly depending on whether it was created with private keys disabled. This eliminates the need
+to distinguish between things that are watchonly and things that are not within a wallet itself.
+
+This change does have a caveat. If a Descriptor Wallet with private keys *enabled* has
+a multiple key descriptor without all of the private keys (e.g. `multi(...)` with only one private key),
+then the wallet will fail to sign and broadcast transactions. Such wallets would need to use the PSBT
+workflow but the typical GUI Send, `sendtoaddress`, etc. workflows would still be available, just
+non-functional.
+
+This issue is worsened if the wallet contains both single key (e.g. `pkh(...)`) descriptors and such
+multiple key descriptors as some transactions could be signed and broadast and others not. This is
+due to some transactions containing only single key inputs, while others would contain both single
+key and multiple key inputs, depending on which are available and how the coin selection algorithm
+selects inputs. However this is not considered to be a supported use case; multisigs
+should be in their own wallets which do not already have descriptors. Although users cannot export
+descriptors with private keys for now as explained earlier.
+
+
+## RPC changes
+ - `createwallet` has changed list of arguments: `createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )`
+`load_on_startup` used to be an argument 5 but now has a number 6.

src/wallet/rpcwallet.cpp

@@ -2935,6 +2935,7 @@ static UniValue createwallet(const JSONRPCRequest& request)
     }
     if (!request.params[5].isNull() && request.params[5].get_bool()) {
         flags |= WALLET_FLAG_DESCRIPTORS;
+        warnings.emplace_back(Untranslated("Wallet is an experimental descriptor wallet"));
     }
 
     DatabaseOptions options;

src/wallet/scriptpubkeyman.cpp

@@ -1774,7 +1774,7 @@ bool LegacyScriptPubKeyMan::GetHDChain(CHDChain& hdChainRet) const
     return !hdChain.IsNull();
 }
 
-void LegacyScriptPubKeyMan::SetType(bool internal) {}
+void LegacyScriptPubKeyMan::SetInternal(bool internal) {}
 
 bool DescriptorScriptPubKeyMan::GetNewDestination(CTxDestination& dest, std::string& error)
 {
@@ -2336,7 +2336,7 @@ uint256 DescriptorScriptPubKeyMan::GetID() const
     return id;
 }
 
-void DescriptorScriptPubKeyMan::SetType(bool internal)
+void DescriptorScriptPubKeyMan::SetInternal(bool internal)
 {
     this->m_internal = internal;
 }

src/wallet/scriptpubkeyman.h

@@ -216,7 +216,7 @@ public:
 
     virtual uint256 GetID() const { return uint256(); }
 
-    virtual void SetType(bool internal) {}
+    virtual void SetInternal(bool internal) {}
 
     /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
     template<typename... Params>
@@ -366,7 +366,7 @@ public:
 
     uint256 GetID() const override;
 
-    void SetType(bool internal) override;
+    void SetInternal(bool internal) override;
 
     // Map from Key ID to key metadata.
     std::map<CKeyID, CKeyMetadata> mapKeyMetadata GUARDED_BY(cs_KeyStore);
@@ -524,8 +524,6 @@ private:
     KeyMap m_map_keys GUARDED_BY(cs_desc_man);
     CryptedKeyMap m_map_crypted_keys GUARDED_BY(cs_desc_man);
 
-    bool SetCrypted();
-
     //! keeps track of whether Unlock has run a thorough check before
     bool m_decryption_thoroughly_checked = false;
 
@@ -596,7 +594,7 @@ public:
 
     uint256 GetID() const override;
 
-    void SetType(bool internal) override;
+    void SetInternal(bool internal) override;
 
     void SetCache(const DescriptorCache& cache);
 

src/wallet/wallet.cpp

@@ -5559,7 +5559,7 @@ void CWallet::SetActiveScriptPubKeyMan(uint256 id, bool internal, bool memonly)
     WalletLogPrintf("Setting spkMan to active: id = %s, type = %d, internal = %d\n", id.ToString(), static_cast<int>(OutputType::LEGACY), static_cast<int>(internal));
     auto& spk_mans = internal ? m_internal_spk_managers : m_external_spk_managers;
     auto spk_man = m_spk_managers.at(id).get();
-    spk_man->SetType(internal);
+    spk_man->SetInternal(internal);
     spk_mans = spk_man;
 
     if (!memonly) {