diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 952dbc77a0d..81eeac88f56 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -1397,6 +1397,12 @@ A few guidelines for introducing and reviewing new RPC interfaces: to a multi-value, or due to other historical reasons. **Always** have false map to 0 and true to 1 in this case. +- For new RPC methods, if implementing a `verbosity` argument, use integer verbosity rather than boolean. + Disallow usage of boolean verbosity (see `ParseVerbosity()` in [util.h](/src/rpc/util.h)). + + - *Rationale*: Integer verbosity allows for multiple values. Undocumented boolean verbosity is deprecated + and new RPC methods should prevent its use. + - Don't forget to fill in the argument names correctly in the RPC command table. - *Rationale*: If not, the call cannot be used with name-based arguments. diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 4894cecfbda..d288ff0ef86 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -766,7 +766,7 @@ static RPCHelpMan getblock() { uint256 hash(ParseHashV(request.params[0], "blockhash")); - int verbosity{ParseVerbosity(request.params[1], /*default_verbosity=*/1)}; + int verbosity{ParseVerbosity(request.params[1], /*default_verbosity=*/1, /*allow_bool=*/true)}; const CBlockIndex* pblockindex; const CBlockIndex* tip; diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 601e4fa7bf5..43a7a218f66 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -255,7 +255,6 @@ static const CRPCConvertParam vRPCConvertParams[] = { "getrawmempool", 0, "verbose" }, { "getrawmempool", 1, "mempool_sequence" }, { "getorphantxs", 0, "verbosity" }, - { "getorphantxs", 0, "verbose" }, { "estimatesmartfee", 0, "conf_target" }, { "estimaterawfee", 0, "conf_target" }, { "estimaterawfee", 1, "threshold" }, diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 27a00c5d917..b26d94b0825 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -823,6 +823,7 @@ static std::vector OrphanDescription() RPCResult{RPCResult::Type::NUM, "bytes", "The serialized transaction size in bytes"}, RPCResult{RPCResult::Type::NUM, "vsize", "The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted."}, RPCResult{RPCResult::Type::NUM, "weight", "The transaction weight as defined in BIP 141."}, + RPCResult{RPCResult::Type::NUM_TIME, "entry", "The entry time into the orphanage expressed in " + UNIX_EPOCH_TIME}, RPCResult{RPCResult::Type::NUM_TIME, "expiration", "The orphan expiration time expressed in " + UNIX_EPOCH_TIME}, RPCResult{RPCResult::Type::ARR, "from", "", { @@ -839,6 +840,7 @@ static UniValue OrphanToJSON(const TxOrphanage::OrphanTxBase& orphan) o.pushKV("bytes", orphan.tx->GetTotalSize()); o.pushKV("vsize", GetVirtualTransactionSize(*orphan.tx)); o.pushKV("weight", GetTransactionWeight(*orphan.tx)); + o.pushKV("entry", int64_t{TicksSinceEpoch(orphan.nTimeExpire - ORPHAN_TX_EXPIRE_TIME)}); o.pushKV("expiration", int64_t{TicksSinceEpoch(orphan.nTimeExpire)}); UniValue from(UniValue::VARR); from.push_back(orphan.fromPeer); // only one fromPeer for now @@ -852,7 +854,7 @@ static RPCHelpMan getorphantxs() "\nShows transactions in the tx orphanage.\n" "\nEXPERIMENTAL warning: this call may be changed in future releases.\n", { - {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::Default{0}, "0 for an array of txids (may contain duplicates), 1 for an array of objects with tx details, and 2 for details from (1) and tx hex", + {"verbosity", RPCArg::Type::NUM, RPCArg::Default{0}, "0 for an array of txids (may contain duplicates), 1 for an array of objects with tx details, and 2 for details from (1) and tx hex", RPCArgOptions{.skip_type_check = true}}, }, { @@ -887,11 +889,11 @@ static RPCHelpMan getorphantxs() PeerManager& peerman = EnsurePeerman(node); std::vector orphanage = peerman.GetOrphanTransactions(); - int verbosity{ParseVerbosity(request.params[0], /*default_verbosity=*/0)}; + int verbosity{ParseVerbosity(request.params[0], /*default_verbosity=*/0, /*allow_bool*/false)}; UniValue ret(UniValue::VARR); - if (verbosity <= 0) { + if (verbosity == 0) { for (auto const& orphan : orphanage) { ret.push_back(orphan.tx->GetHash().ToString()); } @@ -899,13 +901,14 @@ static RPCHelpMan getorphantxs() for (auto const& orphan : orphanage) { ret.push_back(OrphanToJSON(orphan)); } - } else { - // >= 2 + } else if (verbosity == 2) { for (auto const& orphan : orphanage) { UniValue o{OrphanToJSON(orphan)}; o.pushKV("hex", EncodeHexTx(*orphan.tx)); ret.push_back(o); } + } else { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid verbosity value " + ToString(verbosity)); } return ret; diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 65e6e40b0dc..db35421688e 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -338,7 +338,7 @@ static RPCHelpMan getrawtransaction() throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "The genesis block coinbase is not considered an ordinary transaction and cannot be retrieved"); } - int verbosity{ParseVerbosity(request.params[1], /*default_verbosity=*/0)}; + int verbosity{ParseVerbosity(request.params[1], /*default_verbosity=*/0, /*allow_bool=*/true)}; if (!request.params[2].isNull()) { LOCK(cs_main); diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 00072296cc8..f7690f067ed 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -81,10 +81,13 @@ void RPCTypeCheckObj(const UniValue& o, } } -int ParseVerbosity(const UniValue& arg, int default_verbosity) +int ParseVerbosity(const UniValue& arg, int default_verbosity, bool allow_bool) { if (!arg.isNull()) { if (arg.isBool()) { + if (!allow_bool) { + throw JSONRPCError(RPC_TYPE_ERROR, "Verbosity was boolean but only integer allowed"); + } return arg.get_bool(); // true = 1 } else { return arg.getInt(); diff --git a/src/rpc/util.h b/src/rpc/util.h index b8e67597688..f7268f00012 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -103,11 +103,13 @@ std::vector ParseHexO(const UniValue& o, std::string_view strKey) /** * Parses verbosity from provided UniValue. * - * @param[in] arg The verbosity argument as a bool (true) or int (0, 1, 2,...) + * @param[in] arg The verbosity argument as an int (0, 1, 2,...) or bool if allow_bool is set to true * @param[in] default_verbosity The value to return if verbosity argument is null + * @param[in] allow_bool If true, allows arg to be a bool and parses it * @returns An integer describing the verbosity level (e.g. 0, 1, 2, etc.) + * @throws JSONRPCError if allow_bool is false but arg provided is boolean */ -int ParseVerbosity(const UniValue& arg, int default_verbosity); +int ParseVerbosity(const UniValue& arg, int default_verbosity, bool allow_bool); /** * Validate and return a CAmount from a UniValue number or string. diff --git a/test/functional/rpc_getorphantxs.py b/test/functional/rpc_orphans.py similarity index 79% rename from test/functional/rpc_getorphantxs.py rename to test/functional/rpc_orphans.py index dfa1168b2ec..4871166a390 100755 --- a/test/functional/rpc_getorphantxs.py +++ b/test/functional/rpc_orphans.py @@ -2,17 +2,25 @@ # Copyright (c) 2014-2024 The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. -"""Test the getorphantxs RPC.""" +"""Tests for orphan related RPCs.""" -from test_framework.mempool_util import tx_in_orphanage +import time + +from test_framework.mempool_util import ( + ORPHAN_TX_EXPIRE_TIME, + tx_in_orphanage, +) from test_framework.messages import msg_tx from test_framework.p2p import P2PInterface -from test_framework.util import assert_equal +from test_framework.util import ( + assert_equal, + assert_raises_rpc_error, +) from test_framework.test_framework import BitcoinTestFramework from test_framework.wallet import MiniWallet -class GetOrphanTxsTest(BitcoinTestFramework): +class OrphanRPCsTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 @@ -20,6 +28,7 @@ class GetOrphanTxsTest(BitcoinTestFramework): self.wallet = MiniWallet(self.nodes[0]) self.test_orphan_activity() self.test_orphan_details() + self.test_misc() def test_orphan_activity(self): self.log.info("Check that orphaned transactions are returned with getorphantxs") @@ -37,13 +46,13 @@ class GetOrphanTxsTest(BitcoinTestFramework): self.log.info("Check that neither parent is in the mempool") assert_equal(node.getmempoolinfo()["size"], 0) - self.log.info("Check that both children are in the orphanage") - orphanage = node.getorphantxs(verbosity=0) self.log.info("Check the size of the orphanage") assert_equal(len(orphanage), 2) - self.log.info("Check that negative verbosity is treated as 0") - assert_equal(orphanage, node.getorphantxs(verbosity=-1)) + self.log.info("Check that undefined verbosity is disallowed") + assert_raises_rpc_error(-8, "Invalid verbosity value -1", node.getorphantxs, verbosity=-1) + assert_raises_rpc_error(-8, "Invalid verbosity value 3", node.getorphantxs, verbosity=3) + self.log.info("Check that both children are in the orphanage") assert tx_in_orphanage(node, tx_child_1["tx"]) assert tx_in_orphanage(node, tx_child_2["tx"]) @@ -86,6 +95,8 @@ class GetOrphanTxsTest(BitcoinTestFramework): tx_child_2 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_2["new_utxo"]) peer_1 = node.add_p2p_connection(P2PInterface()) peer_2 = node.add_p2p_connection(P2PInterface()) + entry_time = int(time.time()) + node.setmocktime(entry_time) peer_1.send_and_ping(msg_tx(tx_child_1["tx"])) peer_2.send_and_ping(msg_tx(tx_child_2["tx"])) @@ -105,6 +116,9 @@ class GetOrphanTxsTest(BitcoinTestFramework): assert_equal(len(node.getorphantxs()), 1) orphan_1 = orphanage[0] self.orphan_details_match(orphan_1, tx_child_1, verbosity=1) + self.log.info("Checking orphan entry/expiration times") + assert_equal(orphan_1["entry"], entry_time) + assert_equal(orphan_1["expiration"], entry_time + ORPHAN_TX_EXPIRE_TIME) self.log.info("Checking orphan details (verbosity 2)") orphanage = node.getorphantxs(verbosity=2) @@ -125,5 +139,15 @@ class GetOrphanTxsTest(BitcoinTestFramework): self.log.info("Check the transaction hex of orphan") assert_equal(orphan["hex"], tx["hex"]) + def test_misc(self): + node = self.nodes[0] + assert_raises_rpc_error(-3, "Verbosity was boolean but only integer allowed", node.getorphantxs, verbosity=True) + assert_raises_rpc_error(-3, "Verbosity was boolean but only integer allowed", node.getorphantxs, verbosity=False) + help_output = node.help() + self.log.info("Check that getorphantxs is a hidden RPC") + assert "getorphantxs" not in help_output + assert "unknown command: getorphantxs" not in node.help("getorphantxs") + + if __name__ == '__main__': - GetOrphanTxsTest(__file__).main() + OrphanRPCsTest(__file__).main() diff --git a/test/functional/test_framework/mempool_util.py b/test/functional/test_framework/mempool_util.py index a6a7940c608..94d58b9e7df 100644 --- a/test/functional/test_framework/mempool_util.py +++ b/test/functional/test_framework/mempool_util.py @@ -19,6 +19,8 @@ from .wallet import ( MiniWallet, ) +ORPHAN_TX_EXPIRE_TIME = 1200 + def fill_mempool(test_framework, node, *, tx_sync_fun=None): """Fill mempool until eviction. diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 3d8c2300663..dc9053491cc 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -160,7 +160,7 @@ BASE_SCRIPTS = [ 'wallet_importmulti.py --legacy-wallet', 'mempool_limit.py', 'rpc_txoutproof.py', - 'rpc_getorphantxs.py', + 'rpc_orphans.py', 'wallet_listreceivedby.py --legacy-wallet', 'wallet_listreceivedby.py --descriptors', 'wallet_abandonconflict.py --legacy-wallet',