Merge bitcoin/bitcoin#31043: rpc: getorphantxs follow-up

0ea84bc test: explicitly check boolean verbosity is disallowed (tdb3)
7a2e6b6 doc: add rpc guidance for boolean verbosity avoidance (tdb3)
698f302 rpc: disallow boolean verbosity in getorphantxs (tdb3)
63f5e6e test: add entry and expiration time checks (tdb3)
808a708 rpc: add entry time to getorphantxs (tdb3)
56bf302 refactor: rename rpc_getorphantxs to rpc_orphans (tdb3)
7824f6b test: check that getorphantxs is hidden (tdb3)
ac68fcc rpc: disallow undefined verbosity in getorphantxs (tdb3)

Pull request description:

  Implements follow-up suggestions from #30793.

  - Now disallows undefined verbosity levels (below and above valid values) (bitcoin/bitcoin#30793 (comment))
  - Disallows boolean verbosity (bitcoin/bitcoin#30793 (comment)) and adds guidance to developer-notes
  - Checks that `getorphantxs` is a hidden rpc (bitcoin/bitcoin#30793 (comment))
  - Adds a test for `expiration` time
  - Adds `entry` time to the returned orphan objects (verbosity >=1) to relieve the user from having to calculate it from `expiration`.  Also adds associated test. (bitcoin/bitcoin#30793 (comment))
  - Minor cleanup (blank line removal and log message move) (bitcoin/bitcoin#30793 (comment))

  Included a commit to rename the test to a more generic `get_orphans` to better accommodate future orphanage-related RPCs (e.g. `getorphanangeinfo`).  Can drop the refactor commit from this PR if people feel strongly about it.

ACKs for top commit:
  achow101:
    ACK 0ea84bc
  glozow:
    utACK 0ea84bc
  rkrux:
    tACK 0ea84bc
  itornaza:
    tACK 0ea84bc

Tree-SHA512: e48a088f333ebde132923072da58e970461e74362d0acebbc799c3043d5727cdf5f28e82b43cb38bbed27c603df6710695dba91ff0695e623ad168e985dce08e

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.

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;

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" },

src/rpc/mempool.cpp

@@ -823,6 +823,7 @@ static std::vector<RPCResult> 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<std::chrono::seconds>(orphan.nTimeExpire - ORPHAN_TX_EXPIRE_TIME)});
     o.pushKV("expiration", int64_t{TicksSinceEpoch<std::chrono::seconds>(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,25 +889,26 @@ static RPCHelpMan getorphantxs()
             PeerManager& peerman = EnsurePeerman(node);
             std::vector<TxOrphanage::OrphanTxBase> 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());
                 }
             } else if (verbosity == 1) {
                 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;

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);

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<int>();

src/rpc/util.h

@@ -103,11 +103,13 @@ std::vector<unsigned char> 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.

test/functional/rpc_getorphantxs.py → test/functional/rpc_orphans.py

@@ -2,24 +2,33 @@
 # 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
 
     def run_test(self):
         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 @@ def test_orphan_activity(self):
         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 @@ def test_orphan_details(self):
         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 @@ def test_orphan_details(self):
         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 @@ def orphan_details_match(self, orphan, tx, verbosity):
             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()

test/functional/test_framework/mempool_util.py

@@ -19,6 +19,8 @@
     MiniWallet,
 )
 
+ORPHAN_TX_EXPIRE_TIME = 1200
+
 
 def fill_mempool(test_framework, node, *, tx_sync_fun=None):
     """Fill mempool until eviction.

test/functional/test_runner.py

@@ -160,7 +160,7 @@
     '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',