wallet: descriptor wallet release notes and cleanups

doc/release-notes-16528.md

@@ -0,0 +1,120 @@
+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/bitcoin/bitcoin/issues).
+
+0.21 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 Bitcoin 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 Bitcoin 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. Additionally the
+Default Wallet created upon first startup of Bitcoin Core will be a Legacy Wallet.
+
+#### `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
+(e.g. P2WPKH), 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
+* dumpprivkey
+* dumpwallet
+* importmulti
+* addmultisigaddress
+* sethdseed
+
+#### 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. `wpkh(...)`) 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.
+
+#### BIP 44/49/84 Support
+
+The change to using descriptors changes the default derivation paths used by Bitcoin Core
+to adhere to BIP 44/49/84. Descriptors with different derivation paths can be imported without
+issue.

src/wallet/rpcwallet.cpp

@@ -2720,6 +2720,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"));
     }
 
     bilingual_str error;

src/wallet/scriptpubkeyman.cpp

@@ -1497,7 +1497,7 @@ std::set<CKeyID> LegacyScriptPubKeyMan::GetKeys() const
     return set_address;
 }
 
-void LegacyScriptPubKeyMan::SetType(OutputType type, bool internal) {}
+void LegacyScriptPubKeyMan::SetInternal(bool internal) {}
 
 bool DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
 {
@@ -1509,7 +1509,9 @@ bool DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDest
     {
         LOCK(cs_desc_man);
         assert(m_wallet_descriptor.descriptor->IsSingleType()); // This is a combo descriptor which should not be an active descriptor
-        if (type != m_address_type) {
+        Optional<OutputType> desc_addr_type = m_wallet_descriptor.descriptor->GetOutputType();
+        assert(desc_addr_type);
+        if (type != *desc_addr_type) {
             throw std::runtime_error(std::string(__func__) + ": Types are inconsistent");
         }
 
@@ -1777,7 +1779,7 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const
     }
 }
 
-bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key)
+bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type)
 {
     LOCK(cs_desc_man);
     assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
@@ -1794,7 +1796,7 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_
     // Build descriptor string
     std::string desc_prefix;
     std::string desc_suffix = "/*)";
-    switch (m_address_type) {
+    switch (addr_type) {
     case OutputType::LEGACY: {
         desc_prefix = "pkh(" + xpub + "/44'";
         break;
@@ -2076,9 +2078,8 @@ uint256 DescriptorScriptPubKeyMan::GetID() const
     return id;
 }
 
-void DescriptorScriptPubKeyMan::SetType(OutputType type, bool internal)
+void DescriptorScriptPubKeyMan::SetInternal(bool internal)
 {
-    this->m_address_type = type;
     this->m_internal = internal;
 }
 

src/wallet/scriptpubkeyman.h

@@ -224,7 +224,7 @@ class ScriptPubKeyMan
 
     virtual uint256 GetID() const { return uint256(); }
 
-    virtual void SetType(OutputType type, 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>
@@ -370,7 +370,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
 
     uint256 GetID() const override;
 
-    void SetType(OutputType type, bool internal) override;
+    void SetInternal(bool internal) override;
 
     // Map from Key ID to key metadata.
     std::map<CKeyID, CKeyMetadata> mapKeyMetadata GUARDED_BY(cs_KeyStore);
@@ -497,14 +497,11 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
     PubKeyMap m_map_pubkeys GUARDED_BY(cs_desc_man);
     int32_t m_max_cached_index = -1;
 
-    OutputType m_address_type;
     bool m_internal = false;
 
     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;
 
@@ -524,9 +521,9 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
         :   ScriptPubKeyMan(storage),
             m_wallet_descriptor(descriptor)
         {}
-    DescriptorScriptPubKeyMan(WalletStorage& storage, OutputType address_type, bool internal)
+    DescriptorScriptPubKeyMan(WalletStorage& storage, bool internal)
         :   ScriptPubKeyMan(storage),
-            m_address_type(address_type), m_internal(internal)
+            m_internal(internal)
         {}
 
     mutable RecursiveMutex cs_desc_man;
@@ -551,7 +548,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
     bool IsHDEnabled() const override;
 
     //! Setup descriptors based on the given CExtkey
-    bool SetupDescriptorGeneration(const CExtKey& master_key);
+    bool SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type);
 
     bool HavePrivateKeys() const override;
 
@@ -575,7 +572,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
 
     uint256 GetID() const override;
 
-    void SetType(OutputType type, bool internal) override;
+    void SetInternal(bool internal) override;
 
     void SetCache(const DescriptorCache& cache);
 

src/wallet/wallet.cpp

@@ -4369,7 +4369,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
 
     for (bool internal : {false, true}) {
         for (OutputType t : OUTPUT_TYPES) {
-            auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, t, internal));
+            auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, internal));
             if (IsCrypted()) {
                 if (IsLocked()) {
                     throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors");
@@ -4378,7 +4378,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
                     throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors");
                 }
             }
-            spk_manager->SetupDescriptorGeneration(master_key);
+            spk_manager->SetupDescriptorGeneration(master_key, t);
             uint256 id = spk_manager->GetID();
             m_spk_managers[id] = std::move(spk_manager);
             SetActiveScriptPubKeyMan(id, t, internal);
@@ -4391,7 +4391,7 @@ void CWallet::SetActiveScriptPubKeyMan(uint256 id, OutputType type, bool interna
     WalletLogPrintf("Setting spkMan to active: id = %s, type = %d, internal = %d\n", id.ToString(), static_cast<int>(type), 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(type, internal);
+    spk_man->SetInternal(internal);
     spk_mans[type] = spk_man;
 
     if (!memonly) {