Merge #7099: refactor(rpc): migrate composite RPCs to client conversion table, drop ParseInt{32,64}V(), lay foundation to drop ParseBoolV()

2d5817e doc: add release notes (Kittywhiskers Van Gogh)
11bf976 refactor: add composite RPC bools to conversion table, deprecate old fn (Kittywhiskers Van Gogh)
53a66ca refactor: drop unused `ParseInt{32,64}V()`, unresolvable `ParseDoubleV()` (Kittywhiskers Van Gogh)
ac537fc fix: adapt `ParseBlockIndex` to work with stringy integers, update docs (Kittywhiskers Van Gogh)
e2ee1f7 refactor: drop array parsing logic in `quorum rotationinfo` (Kittywhiskers Van Gogh)
635ecd9 refactor: remove `ParseInt{32,64}V()` usage in Dash-specific code (Kittywhiskers Van Gogh)
fd6ccaf fix: don't string quote numerical arguments (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on #7098

  ## Breaking Changes

  See release notes.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 2d5817e
  UdjinM6:
    The issue with a swallowed exception exists on develop actually, can be fixed in a follow-up PR.

utACK 2d5817e

Tree-SHA512: 2432af51fca2e2ac5b1272d244c96588a9122a82428bdb1a6a9c926eecfa3292f0767749b6d1c67632cc9b8ba7942c266cdc5e0a9ddd06a27b167fe0df28808a

Author: PastaPastaPasta

doc/release-notes-7099.md

@@ -0,0 +1,16 @@
+Updated RPCs
+------------
+
+* Dash RPCs will no longer permit submitting strings to boolean input fields in line with validation
+  enforced on upstream RPCs, where this is already the case. Requests must now use unquoted `true`
+  or `false`.
+
+  * The following RPCs are affected, `bls generate`, `bls fromsecret`, `coinjoinsalt generate`,
+    `coinjoinsalt set`, `masternode connect`, `protx diff`, `protx list`, `protx register`,
+    `protx register_legacy`, `protx register_evo`, `protx register_fund`, `protx register_fund_legacy`,
+    `protx register_fund_evo`, `protx revoke`, `protx update_registrar`, `protx update_registrar_legacy`,
+    `protx update_service`, `protx update_service_evo`, `quorum info`,  `quorum platformsign`,
+    `quorum rotationinfo`, `quorum sign`.
+
+  * This restriction can be relaxed by setting `-deprecatedrpc=permissive_bool` at runtime
+    but is liable to be removed in future versions of Dash Core.

src/rpc/client.cpp

@@ -256,25 +256,81 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "verifyislock", 3, "maxHeight" },
     { "submitchainlock", 2, "blockHeight" },
     { "mnauth", 0, "nodeId" },
+    // Compound RPCs (note: index position is offset by one to account for subcommand)
+    { "bls generate", 1, "legacy" },
+    { "bls fromsecret", 2, "legacy" },
+    { "coinjoinsalt generate", 1, "overwrite" },
+    { "coinjoinsalt set", 2, "overwrite" },
+    { "gobject list-prepared", 1, "count" },
+    { "gobject prepare", 2, "revision" },
+    { "gobject prepare", 3, "time" },
+    { "gobject prepare", 7, "outputIndex" },
+    { "gobject submit", 2, "revision" },
+    { "gobject submit", 3, "time" },
+    { "masternode connect", 2, "v2transport" },
+    { "masternode payments", 2, "count" },
+    { "masternode winners", 1, "count" },
+    { "protx diff", 3, "extended" },
+    { "protx list", 2, "detailed" },
+    { "protx list", 3, "height" },
+    { "protx register", 2, "collateralIndex" },
     { "protx register", 3, "coreP2PAddrs", true },
+    { "protx register", 10, "submit" },
+    { "protx register_legacy", 2, "collateralIndex" },
     { "protx register_legacy", 3, "coreP2PAddrs", true },
+    { "protx register_legacy", 10, "submit" },
+    { "protx register_evo", 2, "collateralIndex" },
     { "protx register_evo", 3, "coreP2PAddrs", true },
     { "protx register_evo", 10, "platformP2PAddrs", true },
     { "protx register_evo", 11, "platformHTTPSAddrs", true },
+    { "protx register_evo", 13, "submit" },
     { "protx register_fund", 2, "coreP2PAddrs", true },
+    { "protx register_fund", 9, "submit" },
     { "protx register_fund_legacy", 2, "coreP2PAddrs", true },
+    { "protx register_fund_legacy", 9, "submit" },
     { "protx register_fund_evo", 2, "coreP2PAddrs", true },
     { "protx register_fund_evo", 9, "platformP2PAddrs", true },
     { "protx register_fund_evo", 10, "platformHTTPSAddrs", true },
+    { "protx register_fund_evo", 12, "submit" },
+    { "protx register_prepare", 2, "collateralIndex" },
     { "protx register_prepare", 3, "coreP2PAddrs", true },
+    { "protx register_prepare_legacy", 2, "collateralIndex" },
     { "protx register_prepare_legacy", 3, "coreP2PAddrs", true },
+    { "protx register_prepare_evo", 2, "collateralIndex" },
     { "protx register_prepare_evo", 3, "coreP2PAddrs", true },
     { "protx register_prepare_evo", 10, "platformP2PAddrs", true },
     { "protx register_prepare_evo", 11, "platformHTTPSAddrs", true },
+    { "protx revoke", 3, "reason" },
+    { "protx revoke", 5, "submit" },
+    { "protx update_registrar", 6, "submit" },
+    { "protx update_registrar_legacy", 6, "submit" },
     { "protx update_service", 2, "coreP2PAddrs", true },
+    { "protx update_service", 6, "submit" },
     { "protx update_service_evo", 2, "coreP2PAddrs", true },
     { "protx update_service_evo", 5, "platformP2PAddrs", true },
     { "protx update_service_evo", 6, "platformHTTPSAddrs", true },
+    { "protx update_service_evo", 9, "submit" },
+    { "quorum dkgsimerror", 2, "rate" },
+    { "quorum dkgstatus", 1, "detail_level" },
+    { "quorum getdata", 1, "nodeId" },
+    { "quorum getdata", 2, "llmqType" },
+    { "quorum getdata", 4, "dataMask" },
+    { "quorum getrecsig", 1, "llmqType" },
+    { "quorum hasrecsig", 1, "llmqType" },
+    { "quorum info", 1, "llmqType" },
+    { "quorum info", 3, "includeSkShare" },
+    { "quorum isconflicting", 1, "llmqType" },
+    { "quorum listextended", 1, "height" },
+    { "quorum list", 1, "count" },
+    { "quorum memberof", 2, "scanQuorumsCount" },
+    { "quorum platformsign", 4, "submit" },
+    { "quorum rotationinfo", 2, "extraShare" },
+    { "quorum rotationinfo", 3, "baseBlockHashes" },
+    { "quorum selectquorum", 1, "llmqType" },
+    { "quorum sign", 1, "llmqType" },
+    { "quorum sign", 5, "submit" },
+    { "quorum verify", 1, "llmqType" },
+    { "quorum verify", 6, "signHeight" },
 };
 // clang-format on
 

src/rpc/evo.cpp

@@ -713,7 +713,7 @@ static UniValue protx_register_common_wrapper(const JSONRPCRequest& request,
         paramIdx++;
     } else {
         uint256 collateralHash(ParseHashV(request.params[paramIdx], "collateralHash"));
-        int32_t collateralIndex = ParseInt32V(request.params[paramIdx + 1], "collateralIndex");
+        int32_t collateralIndex = request.params[paramIdx + 1].getInt<int>();
         if (collateralHash.IsNull() || collateralIndex < 0) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("invalid hash or index: %s-%d", collateralHash.ToString(), collateralIndex));
         }
@@ -1277,7 +1277,7 @@ static RPCHelpMan protx_revoke()
     CBLSSecretKey keyOperator = ParseBLSSecretKey(request.params[1].get_str(), "operatorKey");
 
     if (!request.params[2].isNull()) {
-        int32_t nReason = ParseInt32V(request.params[2], "reason");
+        int32_t nReason = request.params[2].getInt<int>();
         if (nReason < 0 || nReason > CProUpRevTx::REASON_LAST) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("invalid reason %d, must be between 0 and %d", nReason, CProUpRevTx::REASON_LAST));
         }
@@ -1462,7 +1462,7 @@ static RPCHelpMan protx_list()
         bool detailed = !request.params[1].isNull() ? ParseBoolV(request.params[1], "detailed") : false;
 
         LOCK2(wallet->cs_wallet, ::cs_main);
-        int height = !request.params[2].isNull() ? ParseInt32V(request.params[2], "height") : chainman.ActiveChain().Height();
+        int height = !request.params[2].isNull() ? request.params[2].getInt<int>() : chainman.ActiveChain().Height();
         if (height < 1 || height > chainman.ActiveChain().Height()) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid height specified");
         }
@@ -1495,7 +1495,7 @@ static RPCHelpMan protx_list()
 #else
         LOCK(::cs_main);
 #endif
-        int height = !request.params[2].isNull() ? ParseInt32V(request.params[2], "height") : chainman.ActiveChain().Height();
+        int height = !request.params[2].isNull() ? request.params[2].getInt<int>() : chainman.ActiveChain().Height();
         if (height < 1 || height > chainman.ActiveChain().Height()) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid height specified");
         }
@@ -1587,20 +1587,43 @@ static uint256 ParseBlock(const UniValue& v, const ChainstateManager& chainman,
     try {
         return ParseHashV(v, strName);
     } catch (...) {
-        int h = ParseInt32V(v, strName);
-        if (h < 1 || h > chainman.ActiveChain().Height())
+        bool fail{false}; int32_t h{0};
+        if (v.isNum()) {
+            h = v.getInt<int>();
+        } else if (!ParseInt32(v.get_str(), &h)) {
+            fail = true;
+        }
+        if (fail || h < 1 || h > chainman.ActiveChain().Height()) {
             throw std::runtime_error(strprintf("%s must be a block hash or chain height and not %s", strName, v.getValStr()));
+        }
         return *chainman.ActiveChain()[h]->phashBlock;
     }
 }
 
+static const CBlockIndex* ParseBlockIndex(const UniValue& v, const ChainstateManager& chainman, const std::string& strName) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
+{
+    AssertLockHeld(::cs_main);
+
+    try {
+        const auto hash{ParseBlock(v, chainman, strName)};
+        const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash);
+        if (!pindex) {
+            throw std::runtime_error(strprintf("Block %s with hash %s not found", strName, v.getValStr()));
+        }
+        return pindex;
+    } catch (...) {
+        // Same phrasing as ParseBlock() as it can parse heights
+        throw std::runtime_error(strprintf("%s must be a block hash or chain height and not %s", strName, v.getValStr()));
+    }
+}
+
 static RPCHelpMan protx_diff()
 {
     return RPCHelpMan{"protx diff",
         "\nCalculates a diff between two deterministic masternode lists. The result also contains proof data.\n",
         {
-            {"baseBlock", RPCArg::Type::NUM, RPCArg::Optional::NO, "The starting block height."},
-            {"block", RPCArg::Type::NUM, RPCArg::Optional::NO, "The ending block height."},
+            {"baseBlock", RPCArg::Type::STR, RPCArg::Optional::NO, "The starting block hash or height."},
+            {"block", RPCArg::Type::STR, RPCArg::Optional::NO, "The ending block hash or height."},
             {"extended", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED, "Show additional fields."},
         },
         CSimplifiedMNListDiff::GetJsonHelp(/*key=*/"", /*optional=*/false),
@@ -1635,31 +1658,13 @@ static RPCHelpMan protx_diff()
     };
 }
 
-static const CBlockIndex* ParseBlockIndex(const UniValue& v, const ChainstateManager& chainman, const std::string& strName) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
-{
-    AssertLockHeld(::cs_main);
-
-    try {
-        uint256 hash = ParseHashV(v, strName);
-        const CBlockIndex* pblockindex = chainman.m_blockman.LookupBlockIndex(hash);
-        if (!pblockindex)
-            throw std::runtime_error(strprintf("Block %s with hash %s not found", strName, v.getValStr()));
-        return pblockindex;
-    } catch (...) {
-        int h = ParseInt32V(v, strName);
-        if (h < 1 || h > chainman.ActiveChain().Height())
-            throw std::runtime_error(strprintf("%s must be a chain height and not %s", strName, v.getValStr()));
-        return chainman.ActiveChain()[h];
-    }
-}
-
 static RPCHelpMan protx_listdiff()
 {
     return RPCHelpMan{"protx listdiff",
                "\nCalculate a full MN list diff between two masternode lists.\n",
                {
-                       {"baseBlock", RPCArg::Type::NUM, RPCArg::Optional::NO, "The starting block height."},
-                       {"block", RPCArg::Type::NUM, RPCArg::Optional::NO, "The ending block height."},
+                       {"baseBlock", RPCArg::Type::STR, RPCArg::Optional::NO, "The starting block hash or height."},
+                       {"block", RPCArg::Type::STR, RPCArg::Optional::NO, "The ending block hash or height."},
                },
                 RPCResult {
                     RPCResult::Type::OBJ, "", "",
@@ -1834,8 +1839,8 @@ static RPCHelpMan evodb_verify()
         "This is a read-only operation that does not modify the database.\n"
         "If no heights are specified, defaults to the full range from DIP0003 activation to chain tip.\n",
         {
-            {"startBlock", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The starting block height (defaults to DIP0003 activation height)."},
-            {"stopBlock", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The ending block height (defaults to current chain tip)."},
+            {"startBlock", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "The starting block hash or height (defaults to DIP0003 activation height)."},
+            {"stopBlock", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "The ending block hash or height (defaults to current chain tip)."},
         },
         RPCResult{
             RPCResult::Type::OBJ, "", "",
@@ -1872,8 +1877,8 @@ static RPCHelpMan evodb_repair()
         "If verification fails, recalculates diffs from blockchain data and replaces corrupted records.\n"
         "If no heights are specified, defaults to the full range from DIP0003 activation to chain tip.\n",
         {
-            {"startBlock", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The starting block height (defaults to DIP0003 activation height)."},
-            {"stopBlock", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The ending block height (defaults to current chain tip)."},
+            {"startBlock", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "The starting block hash or height (defaults to DIP0003 activation height)."},
+            {"stopBlock", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "The ending block hash or height (defaults to current chain tip)."},
         },
         RPCResult{
             RPCResult::Type::OBJ, "", "",

src/rpc/governance.cpp

@@ -176,8 +176,8 @@ static RPCHelpMan gobject_prepare()
         hashParent = ParseHashV(request.params[0], "parent-hash");
     }
 
-    int nRevision = ParseInt32V(request.params[1], "revision");
-    int64_t nTime = ParseInt64V(request.params[2], "time");
+    int nRevision = request.params[1].getInt<int>();
+    int64_t nTime = request.params[2].getInt<int64_t>();
     std::string strDataHex = request.params[3].get_str();
 
     // CREATE A NEW COLLATERAL TRANSACTION FOR THIS SPECIFIC OBJECT
@@ -225,7 +225,7 @@ static RPCHelpMan gobject_prepare()
     outpoint.SetNull();
     if (!request.params[5].isNull() && !request.params[6].isNull()) {
         uint256 collateralHash(ParseHashV(request.params[5], "outputHash"));
-        int32_t collateralIndex = ParseInt32V(request.params[6], "outputIndex");
+        int32_t collateralIndex = request.params[6].getInt<int>();
         if (collateralHash.IsNull() || collateralIndex < 0) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("invalid hash or index: %s-%d", collateralHash.ToString(), collateralIndex));
         }
@@ -278,7 +278,7 @@ static RPCHelpMan gobject_list_prepared()
     if (!wallet) return UniValue::VNULL;
     EnsureWalletIsUnlocked(*wallet);
 
-    int64_t nCount = request.params.empty() ? 10 : ParseInt64V(request.params[0], "count");
+    int64_t nCount = request.params.empty() ? 10 : request.params[0].getInt<int64_t>();
     if (nCount < 0) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Negative count");
     }
@@ -361,8 +361,8 @@ static RPCHelpMan gobject_submit()
 
     // GET THE PARAMETERS FROM USER
 
-    int nRevision = ParseInt32V(request.params[1], "revision");
-    int64_t nTime = ParseInt64V(request.params[2], "time");
+    int nRevision = request.params[1].getInt<int>();
+    int64_t nTime = request.params[2].getInt<int64_t>();
     std::string strDataHex = request.params[3].get_str();
 
     CGovernanceObject govobj(hashParent, nRevision, nTime, txidFee, strDataHex);

src/rpc/masternode.cpp

@@ -288,7 +288,7 @@ static RPCHelpMan masternode_winners()
     std::string strFilter;
 
     if (!request.params[0].isNull()) {
-        nCount = LocaleIndependentAtoi<int>(request.params[0].get_str());
+        nCount = request.params[0].getInt<int>();
     }
 
     if (!request.params[1].isNull()) {
@@ -378,7 +378,7 @@ static RPCHelpMan masternode_payments()
         }
     }
 
-    int64_t nCount = request.params.size() > 1 ? ParseInt64V(request.params[1], "count") : 1;
+    int64_t nCount = request.params.size() > 1 ? request.params[1].getInt<int64_t>() : 1;
 
     // A temporary vector which is used to sort results properly (there is no "reverse" in/for UniValue)
     std::vector<UniValue> vecPayments;