From ac68fcca701e0b3b90c6bb81d66bfa38b57f39bf Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sun, 6 Oct 2024 14:45:07 -0400 Subject: [PATCH 1/8] rpc: disallow undefined verbosity in getorphantxs --- src/rpc/mempool.cpp | 7 ++++--- test/functional/rpc_getorphantxs.py | 13 ++++++++----- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 27a00c5d917..9a6a8d87756 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -891,7 +891,7 @@ static RPCHelpMan getorphantxs() UniValue ret(UniValue::VARR); - if (verbosity <= 0) { + if (verbosity == 0) { for (auto const& orphan : orphanage) { ret.push_back(orphan.tx->GetHash().ToString()); } @@ -899,13 +899,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/test/functional/rpc_getorphantxs.py b/test/functional/rpc_getorphantxs.py index dfa1168b2ec..1e08f15065f 100755 --- a/test/functional/rpc_getorphantxs.py +++ b/test/functional/rpc_getorphantxs.py @@ -7,7 +7,10 @@ from test_framework.mempool_util import 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 @@ -37,13 +40,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"]) From 7824f6b07703463707bb4f10577ff6d34118e248 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sun, 6 Oct 2024 21:11:49 -0400 Subject: [PATCH 2/8] test: check that getorphantxs is hidden --- test/functional/rpc_getorphantxs.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/functional/rpc_getorphantxs.py b/test/functional/rpc_getorphantxs.py index 1e08f15065f..b905d54954e 100755 --- a/test/functional/rpc_getorphantxs.py +++ b/test/functional/rpc_getorphantxs.py @@ -23,6 +23,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") @@ -128,5 +129,13 @@ 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] + 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() From 56bf3027144b4fa6ce9586d3d249b275acb7bcce Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sun, 6 Oct 2024 15:37:45 -0400 Subject: [PATCH 3/8] refactor: rename rpc_getorphantxs to rpc_orphans Generalizes the test to accommodate additional orphan-related RPCs --- test/functional/{rpc_getorphantxs.py => rpc_orphans.py} | 6 +++--- test/functional/test_runner.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) rename test/functional/{rpc_getorphantxs.py => rpc_orphans.py} (98%) diff --git a/test/functional/rpc_getorphantxs.py b/test/functional/rpc_orphans.py similarity index 98% rename from test/functional/rpc_getorphantxs.py rename to test/functional/rpc_orphans.py index b905d54954e..802580bc7cc 100755 --- a/test/functional/rpc_getorphantxs.py +++ b/test/functional/rpc_orphans.py @@ -2,7 +2,7 @@ # 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 from test_framework.messages import msg_tx @@ -15,7 +15,7 @@ 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 @@ -138,4 +138,4 @@ class GetOrphanTxsTest(BitcoinTestFramework): if __name__ == '__main__': - GetOrphanTxsTest(__file__).main() + OrphanRPCsTest(__file__).main() 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', From 808a708107e65e52f54373d2e26f807cf1e444e1 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sun, 6 Oct 2024 19:25:05 -0400 Subject: [PATCH 4/8] rpc: add entry time to getorphantxs --- src/rpc/mempool.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 9a6a8d87756..7bdbd35ec42 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 From 63f5e6ec795f3d5ddfed03f3c51f79ad7a51db1e Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sun, 6 Oct 2024 19:58:25 -0400 Subject: [PATCH 5/8] test: add entry and expiration time checks --- test/functional/rpc_orphans.py | 12 +++++++++++- test/functional/test_framework/mempool_util.py | 2 ++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/test/functional/rpc_orphans.py b/test/functional/rpc_orphans.py index 802580bc7cc..f781cbda404 100755 --- a/test/functional/rpc_orphans.py +++ b/test/functional/rpc_orphans.py @@ -4,7 +4,12 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """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 ( @@ -90,6 +95,8 @@ class OrphanRPCsTest(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"])) @@ -109,6 +116,9 @@ class OrphanRPCsTest(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) 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. From 698f302df8b7cc6e4077c911d3c129960bdb5e07 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Fri, 25 Oct 2024 12:01:44 -0400 Subject: [PATCH 6/8] rpc: disallow boolean verbosity in getorphantxs Updates ParseVerbosity() to support disallowing boolean verbosity. Removes boolean verbosity for getorphantxs to encourage integer verbosity usage --- src/rpc/blockchain.cpp | 2 +- src/rpc/client.cpp | 1 - src/rpc/mempool.cpp | 4 ++-- src/rpc/rawtransaction.cpp | 2 +- src/rpc/util.cpp | 5 ++++- src/rpc/util.h | 6 ++++-- 6 files changed, 12 insertions(+), 8 deletions(-) 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 7bdbd35ec42..b26d94b0825 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -854,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}}, }, { @@ -889,7 +889,7 @@ 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); 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 d71d7d737b0..81461f91ad6 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. From 7a2e6b68cd928a32dd307273727a85890a74c7da Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Fri, 25 Oct 2024 12:04:24 -0400 Subject: [PATCH 7/8] doc: add rpc guidance for boolean verbosity avoidance --- doc/developer-notes.md | 6 ++++++ 1 file changed, 6 insertions(+) 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. From 0ea84bc362f395fd247623c22942eb5ca3d1b874 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Fri, 25 Oct 2024 12:05:50 -0400 Subject: [PATCH 8/8] test: explicitly check boolean verbosity is disallowed --- test/functional/rpc_orphans.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/functional/rpc_orphans.py b/test/functional/rpc_orphans.py index f781cbda404..4871166a390 100755 --- a/test/functional/rpc_orphans.py +++ b/test/functional/rpc_orphans.py @@ -141,6 +141,8 @@ class OrphanRPCsTest(BitcoinTestFramework): 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