mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2024-11-19 01:42:58 +01:00
Code Issues Projects Releases Wiki Activity

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

Browse Source 
0ea84bc362 test: explicitly check boolean verbosity is disallowed (tdb3)
7a2e6b68cd doc: add rpc guidance for boolean verbosity avoidance (tdb3)
698f302df8 rpc: disallow boolean verbosity in getorphantxs (tdb3)
63f5e6ec79 test: add entry and expiration time checks (tdb3)
808a708107 rpc: add entry time to getorphantxs (tdb3)
56bf302714 refactor: rename rpc_getorphantxs to rpc_orphans (tdb3)
7824f6b077 test: check that getorphantxs is hidden (tdb3)
ac68fcca70 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) (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786093549)
  - Disallows boolean verbosity (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788273274) and adds guidance to developer-notes
  - Checks that `getorphantxs` is a hidden rpc (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786107786)
  - 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. (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743687732)
  - Minor cleanup (blank line removal and log message move) (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786092641)

  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 0ea84bc362
  glozow:
    utACK 0ea84bc362
  rkrux:
    tACK 0ea84bc362
  itornaza:
    tACK 0ea84bc362

Tree-SHA512: e48a088f333ebde132923072da58e970461e74362d0acebbc799c3043d5727cdf5f28e82b43cb38bbed27c603df6710695dba91ff0695e623ad168e985dce08e
This commit is contained in:
Ava Chow 2024-10-29 14:49:19 -04:00
commit 27d12cf17f
No known key found for this signature in database
GPG Key ID: 17565732E08E5E41
10 changed files with 60 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
doc/developer-notes.md
View File

@ -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.

2
src/rpc/blockchain.cpp
View File

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

1
src/rpc/client.cpp
View File

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

13
src/rpc/mempool.cpp
View File

@ -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,11 +889,11 @@ 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());
}
@ -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;

2
src/rpc/rawtransaction.cpp
View File

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

5
src/rpc/util.cpp
View File

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

6
src/rpc/util.h
View File

@ -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.

42
test/functional/rpc_getorphantxs.py → test/functional/rpc_orphans.py
View File

@ -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()

2
test/functional/test_framework/mempool_util.py
View File

@ -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.

2
test/functional/test_runner.py
View File

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