From 0bbd873f27559a9c3cf93390b675fc6201bc7365 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Thu, 5 Sep 2024 21:40:52 -0400 Subject: [PATCH 01/15] refactor: add OrphanTxBase for external use Enables external entities to obtain orphan information Github-Pull: #30793 Rebased-From: 91b65adff2aaf16f42c5ccca6e16b951e0e84f9a --- src/txorphanage.cpp | 2 +- src/txorphanage.h | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index faab208333..171b0a3136 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -33,7 +33,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) return false; } - auto ret = m_orphans.emplace(wtxid, OrphanTx{tx, peer, Now() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size()}); + auto ret = m_orphans.emplace(wtxid, OrphanTx{{tx, peer, Now() + ORPHAN_TX_EXPIRE_TIME}, m_orphan_list.size()}); assert(ret.second); m_orphan_list.push_back(ret.first); for (const CTxIn& txin : tx->vin) { diff --git a/src/txorphanage.h b/src/txorphanage.h index 2c53d1d40f..5123bfe867 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -72,11 +72,15 @@ public: return m_orphans.size(); } -protected: - struct OrphanTx { + /** Allows providing orphan information externally */ + struct OrphanTxBase { CTransactionRef tx; NodeId fromPeer; NodeSeconds nTimeExpire; + }; + +protected: + struct OrphanTx : public OrphanTxBase { size_t list_pos; }; From a5fd1f26ece187dd40927cd59e35c9eec145fc37 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Wed, 18 Sep 2024 12:28:04 -0400 Subject: [PATCH 02/15] net: add GetOrphanTransactions() to PeerManager Updates PeerManager (and Impl) to provide orphans with metadata Github-Pull: #30793 Rebased-From: 532491faf1aa90053af52cbedce403b9eccf0bc3 --- src/net_processing.cpp | 7 +++++++ src/net_processing.h | 3 +++ src/txorphanage.cpp | 10 ++++++++++ src/txorphanage.h | 2 ++ 4 files changed, 22 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 13ea3a29be..baf3453888 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -517,6 +517,7 @@ public: std::optional FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); + std::vector GetOrphanTransactions() override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); PeerManagerInfo GetInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void RelayTransaction(const uint256& txid, const uint256& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); @@ -1918,6 +1919,12 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c return true; } +std::vector PeerManagerImpl::GetOrphanTransactions() +{ + LOCK(m_tx_download_mutex); + return m_orphanage.GetOrphanTransactions(); +} + PeerManagerInfo PeerManagerImpl::GetInfo() const { return PeerManagerInfo{ diff --git a/src/net_processing.h b/src/net_processing.h index a413db98e8..b9028b0c69 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -7,6 +7,7 @@ #define BITCOIN_NET_PROCESSING_H #include +#include #include #include @@ -93,6 +94,8 @@ public: /** Get statistics from node state */ virtual bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const = 0; + virtual std::vector GetOrphanTransactions() = 0; + /** Get peer manager info. */ virtual PeerManagerInfo GetInfo() const = 0; diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 171b0a3136..bc027f8890 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -277,3 +277,13 @@ std::vector> TxOrphanage::GetChildrenFromDiff } return children_found; } + +std::vector TxOrphanage::GetOrphanTransactions() const +{ + std::vector ret; + ret.reserve(m_orphans.size()); + for (auto const& o : m_orphans) { + ret.push_back({o.second.tx, o.second.fromPeer, o.second.nTimeExpire}); + } + return ret; +} diff --git a/src/txorphanage.h b/src/txorphanage.h index 5123bfe867..5501d10922 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -79,6 +79,8 @@ public: NodeSeconds nTimeExpire; }; + std::vector GetOrphanTransactions() const; + protected: struct OrphanTx : public OrphanTxBase { size_t list_pos; From 6cbc025ce72dc0c88c5794d42ec1b63f8f6946bc Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Wed, 18 Sep 2024 12:28:43 -0400 Subject: [PATCH 03/15] RPC: Add ParseVerbosity helper Provides a common way for rpcs to obtain verbosity from an rpc parameter Github-Pull: #30793 Rebased-From: f511ff3654d999951a64098c8a9f2f8e29771dad (addition only) --- src/rpc/util.cpp | 12 ++++++++++++ src/rpc/util.h | 9 +++++++++ 2 files changed, 21 insertions(+) diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index cc49670198..7b1d5714b8 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -80,6 +80,18 @@ void RPCTypeCheckObj(const UniValue& o, } } +int ParseVerbosity(const UniValue& arg, int default_verbosity) +{ + if (!arg.isNull()) { + if (arg.isBool()) { + return arg.get_bool(); // true = 1 + } else { + return arg.getInt(); + } + } + return default_verbosity; +} + CAmount AmountFromValue(const UniValue& value, int decimals) { if (!value.isNum() && !value.isStr()) diff --git a/src/rpc/util.h b/src/rpc/util.h index 23024376e0..b8e6759768 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -100,6 +100,15 @@ uint256 ParseHashO(const UniValue& o, std::string_view strKey); std::vector ParseHexV(const UniValue& v, std::string_view name); 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] default_verbosity The value to return if verbosity argument is null + * @returns An integer describing the verbosity level (e.g. 0, 1, 2, etc.) + */ +int ParseVerbosity(const UniValue& arg, int default_verbosity); + /** * Validate and return a CAmount from a UniValue number or string. * From ed95ab1b3b9d489dda6027a832d19165b2a77674 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Wed, 18 Sep 2024 12:29:32 -0400 Subject: [PATCH 04/15] rpc: add getorphantxs Adds an rpc to obtain data about the transactions currently in the orphanage. Hidden and marked as experimental Github-Pull: #30793 Rebased-From: 34a9c10e8cdb3e9cd40fc3a420df8f73e0208a48 --- src/rpc/client.cpp | 2 + src/rpc/mempool.cpp | 102 ++++++++++++++++++++++++++++++++++++++++++ src/test/fuzz/rpc.cpp | 1 + 3 files changed, 105 insertions(+) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 5d36bd7e0c..acaaf525aa 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -253,6 +253,8 @@ static const CRPCConvertParam vRPCConvertParams[] = { "keypoolrefill", 0, "newsize" }, { "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 b97c4a5e2f..25d8c8aee5 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -8,8 +8,10 @@ #include #include +#include #include #include +#include #include #include #include @@ -26,6 +28,7 @@ #include #include #include +#include #include #include @@ -851,6 +854,104 @@ static RPCHelpMan savemempool() }; } +static std::vector OrphanDescription() +{ + return { + RPCResult{RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"}, + RPCResult{RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"}, + 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, "expiration", "The orphan expiration time expressed in " + UNIX_EPOCH_TIME}, + RPCResult{RPCResult::Type::ARR, "from", "", + { + RPCResult{RPCResult::Type::NUM, "peer_id", "Peer ID"}, + }}, + }; +} + +static UniValue OrphanToJSON(const TxOrphanage::OrphanTxBase& orphan) +{ + UniValue o(UniValue::VOBJ); + o.pushKV("txid", orphan.tx->GetHash().ToString()); + o.pushKV("wtxid", orphan.tx->GetWitnessHash().ToString()); + o.pushKV("bytes", orphan.tx->GetTotalSize()); + o.pushKV("vsize", GetVirtualTransactionSize(*orphan.tx)); + o.pushKV("weight", GetTransactionWeight(*orphan.tx)); + o.pushKV("expiration", int64_t{TicksSinceEpoch(orphan.nTimeExpire)}); + UniValue from(UniValue::VARR); + from.push_back(orphan.fromPeer); // only one fromPeer for now + o.pushKV("from", from); + return o; +} + +static RPCHelpMan getorphantxs() +{ + return 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", + RPCArgOptions{.skip_type_check = true}}, + }, + { + RPCResult{"for verbose = 0", + RPCResult::Type::ARR, "", "", + { + {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"}, + }}, + RPCResult{"for verbose = 1", + RPCResult::Type::ARR, "", "", + { + {RPCResult::Type::OBJ, "", "", OrphanDescription()}, + }}, + RPCResult{"for verbose = 2", + RPCResult::Type::ARR, "", "", + { + {RPCResult::Type::OBJ, "", "", + Cat>( + OrphanDescription(), + {{RPCResult::Type::STR_HEX, "hex", "The serialized, hex-encoded transaction data"}} + ) + }, + }}, + }, + RPCExamples{ + HelpExampleCli("getorphantxs", "2") + + HelpExampleRpc("getorphantxs", "2") + }, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue + { + const NodeContext& node = EnsureAnyNodeContext(request.context); + PeerManager& peerman = EnsurePeerman(node); + std::vector orphanage = peerman.GetOrphanTransactions(); + + int verbosity{ParseVerbosity(request.params[0], /*default_verbosity=*/0)}; + + UniValue ret(UniValue::VARR); + + 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 + for (auto const& orphan : orphanage) { + UniValue o{OrphanToJSON(orphan)}; + o.pushKV("hex", EncodeHexTx(*orphan.tx)); + ret.push_back(o); + } + } + + return ret; + }, + }; +} + static RPCHelpMan submitpackage() { return RPCHelpMan{"submitpackage", @@ -1067,6 +1168,7 @@ void RegisterMempoolRPCCommands(CRPCTable& t) {"blockchain", &importmempool}, {"blockchain", &savemempool}, {"blockchain", &maxmempool}, + {"hidden", &getorphantxs}, {"rawtransactions", &submitpackage}, }; for (const auto& c : commands) { diff --git a/src/test/fuzz/rpc.cpp b/src/test/fuzz/rpc.cpp index 2bd9c3cbda..ed7e65025a 100644 --- a/src/test/fuzz/rpc.cpp +++ b/src/test/fuzz/rpc.cpp @@ -143,6 +143,7 @@ const std::vector RPC_COMMANDS_SAFE_FOR_FUZZING{ "getnetworkhashps", "getnetworkinfo", "getnodeaddresses", + "getorphantxs", "getpeerinfo", "getprioritisedtransactions", "getrawaddrman", From 28f583d7952f94a555eabb768b852a00afc1611c Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Wed, 18 Sep 2024 12:29:55 -0400 Subject: [PATCH 05/15] test: add tx_in_orphanage() Allows tests to check if a transaction is contained within the orphanage Github-Pull: #30793 Rebased-From: 93f48fceb7dd332ef980ce890ff7750b995d6077 --- test/functional/test_framework/mempool_util.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/functional/test_framework/mempool_util.py b/test/functional/test_framework/mempool_util.py index 148cc935ed..100b0542a7 100644 --- a/test/functional/test_framework/mempool_util.py +++ b/test/functional/test_framework/mempool_util.py @@ -8,6 +8,7 @@ from decimal import Decimal from .blocktools import ( COINBASE_MATURITY, ) +from .messages import CTransaction from .util import ( assert_equal, assert_greater_than, @@ -79,3 +80,8 @@ def fill_mempool(test_framework, node): test_framework.log.debug("Check that mempoolminfee is larger than minrelaytxfee") assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) assert_greater_than(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) + +def tx_in_orphanage(node, tx: CTransaction) -> bool: + """Returns true if the transaction is in the orphanage.""" + found = [o for o in node.getorphantxs(verbosity=1) if o["txid"] == tx.rehash() and o["wtxid"] == tx.getwtxid()] + return len(found) == 1 From 4e9d1d5b8ee95f4964efda6628c49f11e121963e Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Wed, 18 Sep 2024 12:30:15 -0400 Subject: [PATCH 06/15] test: add getorphantxs tests Adds functional tests for getorphantxs Github-Pull: #30793 Rebased-From: 98c1536852d1de9a978b11046e7414e79ed40b46 --- test/functional/rpc_getorphantxs.py | 130 ++++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 131 insertions(+) create mode 100755 test/functional/rpc_getorphantxs.py diff --git a/test/functional/rpc_getorphantxs.py b/test/functional/rpc_getorphantxs.py new file mode 100755 index 0000000000..8d32ce1638 --- /dev/null +++ b/test/functional/rpc_getorphantxs.py @@ -0,0 +1,130 @@ +#!/usr/bin/env python3 +# 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.""" + +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.test_framework import BitcoinTestFramework +from test_framework.wallet import MiniWallet + + +class GetOrphanTxsTest(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() + + def test_orphan_activity(self): + self.log.info("Check that orphaned transactions are returned with getorphantxs") + node = self.nodes[0] + + self.log.info("Create two 1P1C packages, but only broadcast the children") + tx_parent_1 = self.wallet.create_self_transfer() + tx_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_1["new_utxo"]) + tx_parent_2 = self.wallet.create_self_transfer() + tx_child_2 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_2["new_utxo"]) + peer = node.add_p2p_connection(P2PInterface()) + peer.send_and_ping(msg_tx(tx_child_1["tx"])) + peer.send_and_ping(msg_tx(tx_child_2["tx"])) + + 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)) + assert tx_in_orphanage(node, tx_child_1["tx"]) + assert tx_in_orphanage(node, tx_child_2["tx"]) + + self.log.info("Broadcast parent 1") + peer.send_and_ping(msg_tx(tx_parent_1["tx"])) + self.log.info("Check that parent 1 and child 1 are in the mempool") + raw_mempool = node.getrawmempool() + assert_equal(len(raw_mempool), 2) + assert tx_parent_1["txid"] in raw_mempool + assert tx_child_1["txid"] in raw_mempool + + self.log.info("Check that orphanage only contains child 2") + orphanage = node.getorphantxs() + assert_equal(len(orphanage), 1) + assert tx_in_orphanage(node, tx_child_2["tx"]) + + peer.send_and_ping(msg_tx(tx_parent_2["tx"])) + self.log.info("Check that all parents and children are now in the mempool") + raw_mempool = node.getrawmempool() + assert_equal(len(raw_mempool), 4) + assert tx_parent_1["txid"] in raw_mempool + assert tx_child_1["txid"] in raw_mempool + assert tx_parent_2["txid"] in raw_mempool + assert tx_child_2["txid"] in raw_mempool + self.log.info("Check that the orphanage is empty") + assert_equal(len(node.getorphantxs()), 0) + + self.log.info("Confirm the transactions (clears mempool)") + self.generate(node, 1) + assert_equal(node.getmempoolinfo()["size"], 0) + + def test_orphan_details(self): + self.log.info("Check the transaction details returned from getorphantxs") + node = self.nodes[0] + + self.log.info("Create two orphans, from different peers") + tx_parent_1 = self.wallet.create_self_transfer() + tx_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_1["new_utxo"]) + tx_parent_2 = self.wallet.create_self_transfer() + 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()) + peer_1.send_and_ping(msg_tx(tx_child_1["tx"])) + peer_2.send_and_ping(msg_tx(tx_child_2["tx"])) + + orphanage = node.getorphantxs(verbosity=2) + assert tx_in_orphanage(node, tx_child_1["tx"]) + assert tx_in_orphanage(node, tx_child_2["tx"]) + + self.log.info("Check that orphan 1 and 2 were from different peers") + assert orphanage[0]["from"][0] != orphanage[1]["from"][0] + + self.log.info("Unorphan child 2") + peer_2.send_and_ping(msg_tx(tx_parent_2["tx"])) + assert not tx_in_orphanage(node, tx_child_2["tx"]) + + self.log.info("Checking orphan details") + orphanage = node.getorphantxs(verbosity=1) + 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 details (verbosity 2)") + orphanage = node.getorphantxs(verbosity=2) + orphan_1 = orphanage[0] + self.orphan_details_match(orphan_1, tx_child_1, verbosity=2) + + def orphan_details_match(self, orphan, tx, verbosity): + self.log.info("Check txid/wtxid of orphan") + assert_equal(orphan["txid"], tx["txid"]) + assert_equal(orphan["wtxid"], tx["wtxid"]) + + self.log.info("Check the sizes of orphan") + assert_equal(orphan["bytes"], len(tx["tx"].serialize())) + assert_equal(orphan["vsize"], tx["tx"].get_vsize()) + assert_equal(orphan["weight"], tx["tx"].get_weight()) + + if verbosity == 2: + self.log.info("Check the transaction hex of orphan") + assert_equal(orphan["hex"], tx["hex"]) + + +if __name__ == '__main__': + GetOrphanTxsTest(__file__).main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index b85bf1c668..2e5a9de300 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -157,6 +157,7 @@ BASE_SCRIPTS = [ 'wallet_importmulti.py --legacy-wallet', 'mempool_limit.py', 'rpc_txoutproof.py', + 'rpc_getorphantxs.py', 'wallet_listreceivedby.py --legacy-wallet', 'wallet_listreceivedby.py --descriptors', 'wallet_abandonconflict.py --legacy-wallet', From c74b525c8abfef04842edc941b2e3d9c864bf821 Mon Sep 17 00:00:00 2001 From: kevkevinpal Date: Sun, 6 Oct 2024 10:22:10 -0400 Subject: [PATCH 07/15] test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped Github-Pull: #31040 Rebased-From: 5c299ecafe6f336cffa145d28036b04b87e26712 --- test/functional/p2p_orphan_handling.py | 45 ++++++++++++++++++++++++++ test/functional/rpc_getorphantxs.py | 1 - 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py index 22600bf8a4..9ec23e9d6e 100755 --- a/test/functional/p2p_orphan_handling.py +++ b/test/functional/p2p_orphan_handling.py @@ -5,6 +5,7 @@ import time +from test_framework.mempool_util import tx_in_orphanage from test_framework.messages import ( CInv, CTxInWitness, @@ -41,6 +42,8 @@ from test_framework.wallet import ( # for one peer and y seconds for another, use specific values instead. TXREQUEST_TIME_SKIP = NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY + OVERLOADED_PEER_TX_DELAY + 1 +DEFAULT_MAX_ORPHAN_TRANSACTIONS = 100 + def cleanup(func): # Time to fastfoward (using setmocktime) in between subtests to ensure they do not interfere with # one another, in seconds. Equal to 12 hours, which is enough to expire anything that may exist @@ -566,6 +569,47 @@ class OrphanHandlingTest(BitcoinTestFramework): assert tx_child["txid"] in node_mempool assert_equal(node.getmempoolentry(tx_child["txid"])["wtxid"], tx_child["wtxid"]) + @cleanup + def test_max_orphan_amount(self): + self.log.info("Check that we never exceed our storage limits for orphans") + + node = self.nodes[0] + self.generate(self.wallet, 1) + peer_1 = node.add_p2p_connection(P2PInterface()) + + self.log.info("Check that orphanage is empty on start of test") + assert len(node.getorphantxs()) == 0 + + self.log.info("Filling up orphanage with " + str(DEFAULT_MAX_ORPHAN_TRANSACTIONS) + "(DEFAULT_MAX_ORPHAN_TRANSACTIONS) orphans") + orphans = [] + parent_orphans = [] + for _ in range(DEFAULT_MAX_ORPHAN_TRANSACTIONS): + tx_parent_1 = self.wallet.create_self_transfer() + tx_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_1["new_utxo"]) + parent_orphans.append(tx_parent_1["tx"]) + orphans.append(tx_child_1["tx"]) + peer_1.send_message(msg_tx(tx_child_1["tx"])) + + peer_1.sync_with_ping() + orphanage = node.getorphantxs() + assert_equal(len(orphanage), DEFAULT_MAX_ORPHAN_TRANSACTIONS) + + for orphan in orphans: + assert tx_in_orphanage(node, orphan) + + self.log.info("Check that we do not add more than the max orphan amount") + tx_parent_1 = self.wallet.create_self_transfer() + tx_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_1["new_utxo"]) + peer_1.send_and_ping(msg_tx(tx_child_1["tx"])) + parent_orphans.append(tx_parent_1["tx"]) + orphanage = node.getorphantxs() + assert_equal(len(orphanage), DEFAULT_MAX_ORPHAN_TRANSACTIONS) + + self.log.info("Clearing the orphanage") + for index, parent_orphan in enumerate(parent_orphans): + peer_1.send_and_ping(msg_tx(parent_orphan)) + assert_equal(len(node.getorphantxs()),0) + def run_test(self): self.nodes[0].setmocktime(int(time.time())) @@ -582,6 +626,7 @@ class OrphanHandlingTest(BitcoinTestFramework): self.test_same_txid_orphan() self.test_same_txid_orphan_of_orphan() self.test_orphan_txid_inv() + self.test_max_orphan_amount() if __name__ == '__main__': diff --git a/test/functional/rpc_getorphantxs.py b/test/functional/rpc_getorphantxs.py index 8d32ce1638..dfa1168b2e 100755 --- a/test/functional/rpc_getorphantxs.py +++ b/test/functional/rpc_getorphantxs.py @@ -125,6 +125,5 @@ class GetOrphanTxsTest(BitcoinTestFramework): self.log.info("Check the transaction hex of orphan") assert_equal(orphan["hex"], tx["hex"]) - if __name__ == '__main__': GetOrphanTxsTest(__file__).main() From b590f2bc888476d46ca8650f431e7630ce06e3e8 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 08/15] rpc: disallow undefined verbosity in getorphantxs Github-Pull: #31043 Rebased-From: ac68fcca701e0b3b90c6bb81d66bfa38b57f39bf --- 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 25d8c8aee5..43d6dcf82e 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -930,7 +930,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()); } @@ -938,13 +938,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 dfa1168b2e..1e08f15065 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 4c770de7dc97bdf7f92b1190eecdade4a7c1b800 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 09/15] test: check that getorphantxs is hidden Github-Pull: #31043 Rebased-From: 7824f6b07703463707bb4f10577ff6d34118e248 --- 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 1e08f15065..b905d54954 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 700c917c6d040bd248d166376f14dd16d61e8991 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 10/15] refactor: rename rpc_getorphantxs to rpc_orphans Generalizes the test to accommodate additional orphan-related RPCs Github-Pull: #31043 Rebased-From: 56bf3027144b4fa6ce9586d3d249b275acb7bcce --- 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 b905d54954..802580bc7c 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 2e5a9de300..5cf6839434 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -157,7 +157,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 f9d078166b73bc1102303979d833002ed909d633 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 11/15] rpc: add entry time to getorphantxs Github-Pull: #31043 Rebased-From: 808a708107e65e52f54373d2e26f807cf1e444e1 --- src/rpc/mempool.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 43d6dcf82e..30187fd472 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -862,6 +862,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", "", { @@ -878,6 +879,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 6703ee85994b8ed44d86c01228988d2bc9c745d4 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 12/15] test: add entry and expiration time checks Github-Pull: #31043 Rebased-From: 63f5e6ec795f3d5ddfed03f3c51f79ad7a51db1e --- 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 802580bc7c..f781cbda40 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 100b0542a7..67343ae485 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): """Fill mempool until eviction. From f915da77cba8a6262cf06a1a455fe0a205013ad8 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 13/15] rpc: disallow boolean verbosity in getorphantxs Updates ParseVerbosity() to support disallowing boolean verbosity. Removes boolean verbosity for getorphantxs to encourage integer verbosity usage Github-Pull: #31043 Rebased-From: 698f302df8b7cc6e4077c911d3c129960bdb5e07 --- src/rpc/client.cpp | 1 - src/rpc/mempool.cpp | 4 ++-- src/rpc/util.cpp | 5 ++++- src/rpc/util.h | 6 ++++-- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index acaaf525aa..31c3aae0db 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -254,7 +254,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 30187fd472..4730ee9b22 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -893,7 +893,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}}, }, { @@ -928,7 +928,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/util.cpp b/src/rpc/util.cpp index 7b1d5714b8..481a7c36ac 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -80,10 +80,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 b8e6759768..f7268f0001 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 ba9560b59aed76cdcefbd40295b834b7911a0285 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 14/15] test: explicitly check boolean verbosity is disallowed Github-Pull: #31043 Rebased-From: 0ea84bc362f395fd247623c22942eb5ca3d1b874 --- 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 f781cbda40..4871166a39 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 From abb1cc09785afbc14278ef1501dd9b8f014131e4 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 8 Feb 2025 05:36:17 +0000 Subject: [PATCH 15/15] RPC/mempool: doc: Warn that vsize returned by getorphantxs can be incorrect --- src/rpc/mempool.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 4730ee9b22..e1846203de 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -860,7 +860,7 @@ static std::vector OrphanDescription() RPCResult{RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"}, RPCResult{RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"}, 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, "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. CAUTION: Since orphan transactions are missing input data, this can be incorrect!"}, 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},