Merge bitcoin#28597: wallet: No BDB creation, unless -deprecatedrpc=create_bdb

fa071ae wallet: No BDB creation, unless -deprecatedrpc=create_bdb (MarcoFalke)

Pull request description:

  With BDB being removed soon, it seems confusing and harmful to allow users to create fresh BDB wallets going forward, as it would load them with an additional burden of having to migrate them soon after.

  Also, it would be good to allow for one release for test (and external) scripts to adapt.

  Fix all issues by introducing the `-deprecatedrpc=create_bdb` setting.

ACKs for top commit:
  Sjors:
    tACK fa071ae
  achow101:
    ACK fa071ae
  furszy:
    utACK fa071ae

Tree-SHA512: 37a4c3e4ba659e0ebe2382e71d9c80e42a895d9ad743f5dda7c110fbbb7d2a36f46769982552a9ac0c3a57203379ef164be97aa8033eb7674d6b4da030ba8f9b

Author: achow101

src/rpc/util.cpp

@@ -573,6 +573,7 @@ TMPL_INST(nullptr, std::optional<bool>, maybe_arg ? std::optional{maybe_arg->get
 TMPL_INST(nullptr, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;);
 
 // Required arg or optional arg with default value.
+TMPL_INST(CheckRequiredOrDefault, bool, CHECK_NONFATAL(maybe_arg)->get_bool(););
 TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>(););
 TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>(););
 TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str(););
@@ -678,42 +679,50 @@ UniValue RPCHelpMan::GetArgMap() const
     return arr;
 }
 
-void RPCArg::MatchesType(const UniValue& request) const
+static std::optional<UniValue::VType> ExpectedType(RPCArg::Type type)
 {
-    if (m_opts.skip_type_check) return;
-    if (IsOptional() && request.isNull()) return;
-    switch (m_type) {
+    using Type = RPCArg::Type;
+    switch (type) {
     case Type::STR_HEX:
     case Type::STR: {
-        RPCTypeCheckArgument(request, UniValue::VSTR);
-        return;
+        return UniValue::VSTR;
     }
     case Type::NUM: {
-        RPCTypeCheckArgument(request, UniValue::VNUM);
-        return;
+        return UniValue::VNUM;
     }
     case Type::AMOUNT: {
         // VNUM or VSTR, checked inside AmountFromValue()
-        return;
+        return std::nullopt;
     }
     case Type::RANGE: {
         // VNUM or VARR, checked inside ParseRange()
-        return;
+        return std::nullopt;
     }
     case Type::BOOL: {
-        RPCTypeCheckArgument(request, UniValue::VBOOL);
-        return;
+        return UniValue::VBOOL;
     }
     case Type::OBJ:
     case Type::OBJ_USER_KEYS: {
-        RPCTypeCheckArgument(request, UniValue::VOBJ);
-        return;
+        return UniValue::VOBJ;
     }
     case Type::ARR: {
-        RPCTypeCheckArgument(request, UniValue::VARR);
-        return;
+        return UniValue::VARR;
     }
     } // no default case, so the compiler can warn about missing cases
+    NONFATAL_UNREACHABLE();
+}
+
+UniValue RPCArg::MatchesType(const UniValue& request) const
+{
+    if (m_opts.skip_type_check) return true;
+    if (IsOptional() && request.isNull()) return true;
+    const auto exp_type{ExpectedType(m_type)};
+    if (!exp_type) return true; // nothing to check
+
+    if (*exp_type != request.getType()) {
+        return strprintf("JSON value of type %s is not of expected type %s", uvTypeName(request.getType()), uvTypeName(*exp_type));
+    }
+    return true;
 }
 
 std::string RPCArg::GetFirstName() const

src/rpc/util.h

@@ -227,8 +227,11 @@ struct RPCArg {
 
     bool IsOptional() const;
 
-    /** Check whether the request JSON type matches. */
-    void MatchesType(const UniValue& request) const;
+    /**
+     * Check whether the request JSON type matches.
+     * Returns true if type matches, or object describing error(s) if not.
+     */
+    UniValue MatchesType(const UniValue& request) const;
 
     /** Return the first of all aliases */
     std::string GetFirstName() const;

src/wallet/rpc/wallet.cpp

@@ -635,7 +635,7 @@ static RPCHelpMan createwallet()
             {"passphrase", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Encrypt the wallet with this passphrase."},
             {"avoid_reuse", RPCArg::Type::BOOL, RPCArg::Default{false}, "Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind."},
             {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation."
-                                                                       " Setting to \"false\" will create a legacy wallet; however, the legacy wallet type is being deprecated and"
+                                                                       " Setting to \"false\" will create a legacy wallet; This is only possible with the -deprecatedrpc=create_bdb setting because, the legacy wallet type is being deprecated and"
                                                                        " support for creating and opening legacy wallets will be removed in the future."},
             {"load_on_startup", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED_NAMED_ARG, "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."},
             {"external_signer", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use an external signer such as a hardware wallet. Requires -signer to be configured. Wallet creation will fail if keys cannot be fetched. Requires disable_private_keys and descriptors set to true."},
@@ -681,11 +681,16 @@ static RPCHelpMan createwallet()
     if (!request.params[5].isNull() && request.params[6].isNull()) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "The createwallet RPC requires specifying the 'load_on_startup' flag when param 'descriptors' is specified. Dash Core v21 introduced this requirement due to breaking changes in the createwallet RPC.");
     }
-    if (request.params[5].isNull() || request.params[5].get_bool()) {
+    if (self.Arg<bool>("descriptors")) {
 #ifndef USE_SQLITE
         throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without sqlite support (required for descriptor wallets)");
 #endif
         flags |= WALLET_FLAG_DESCRIPTORS;
+    } else {
+        if (!context.chain->rpcEnableDeprecated("create_bdb")) {
+            throw JSONRPCError(RPC_WALLET_ERROR, "BDB wallet creation is deprecated and will be removed in a future release."
+                                                 " In this release it can be re-enabled temporarily with the -deprecatedrpc=create_bdb setting.");
+        }
     }
     if (!request.params[7].isNull() && request.params[7].get_bool()) {
 #ifdef ENABLE_EXTERNAL_SIGNER

test/functional/test_framework/util.py

@@ -421,6 +421,7 @@ def write_config(config_path, *, n, chain, extra_config="", disable_autoconnect=
         f.write("shrinkdebugfile=0\n")
         # To reduce IO and consumed disk storage use tiny size for allocated blk and rev files
         f.write("tinyblk=1\n")
+        f.write("deprecatedrpc=create_bdb\n")  # Required to run the tests
         # To improve SQLite wallet performance so that the tests don't timeout, use -unsafesqlitesync
         f.write("unsafesqlitesync=1\n")
         if disable_autoconnect:

test/functional/wallet_createwallet.py

@@ -175,7 +175,7 @@ def run_test(self):
 
         if self.is_bdb_compiled():
             self.log.info("Test legacy wallet deprecation")
-            res = self.nodes[0].createwallet(wallet_name="legacy_w0", descriptors=False, load_on_startup=True)
+            res = self.nodes[0].createwallet(wallet_name="legacy_w0", passphrase=None, descriptors=False, load_on_startup=True)
             assert_equal(res["warning"], "Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future.")
 
 if __name__ == '__main__':