From 35d928c63237e31c99215e2d9d84782befd618d5 Mon Sep 17 00:00:00 2001 From: josibake Date: Thu, 12 Aug 2021 15:37:06 +0200 Subject: rpc: deprecate fee fields from mempool entries Unless `-deprecatedrpc=fees` is passed, top level fee fields are no longer returned for mempool entries. Add instructions to field help on how to access deprecated fields, update help text for readability, and include units. This is important to help avoid any confusion as users move from deprecated fields to the fee fields object (credit: jonatack). This affects `getmempoolentry`, `getrawmempool`, `getmempoolancestors`, and `getmempooldescendants` Modify `test/functional/mempool_packages.py` and `test/functional/rpc_fundrawtransaction.py` tests to no longer use deprecated fields. Co-authored-by: jonatack --- src/rpc/blockchain.cpp | 33 +++++++++++++++++++------------ test/functional/mempool_packages.py | 25 +++++++++-------------- test/functional/rpc_fundrawtransaction.py | 10 +++++----- 3 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index aa7a55e7a9..2c7f1a70ae 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -462,23 +462,23 @@ static RPCHelpMan getdifficulty() static std::vector MempoolEntryDescription() { return { RPCResult{RPCResult::Type::NUM, "vsize", "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", "transaction weight as defined in BIP 141."}, - RPCResult{RPCResult::Type::STR_AMOUNT, "fee", "transaction fee in " + CURRENCY_UNIT + " (DEPRECATED)"}, - RPCResult{RPCResult::Type::STR_AMOUNT, "modifiedfee", "transaction fee with fee deltas used for mining priority (DEPRECATED)"}, + RPCResult{RPCResult::Type::STR_AMOUNT, "fee", "transaction fee, denominated in " + CURRENCY_UNIT + " (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"}, + RPCResult{RPCResult::Type::STR_AMOUNT, "modifiedfee", "transaction fee with fee deltas used for mining priority, denominated in " + CURRENCY_UNIT + " (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"}, RPCResult{RPCResult::Type::NUM_TIME, "time", "local time transaction entered pool in seconds since 1 Jan 1970 GMT"}, RPCResult{RPCResult::Type::NUM, "height", "block height when transaction entered pool"}, RPCResult{RPCResult::Type::NUM, "descendantcount", "number of in-mempool descendant transactions (including this one)"}, RPCResult{RPCResult::Type::NUM, "descendantsize", "virtual transaction size of in-mempool descendants (including this one)"}, - RPCResult{RPCResult::Type::STR_AMOUNT, "descendantfees", "modified fees (see above) of in-mempool descendants (including this one) (DEPRECATED)"}, + RPCResult{RPCResult::Type::STR_AMOUNT, "descendantfees", "transaction fees of in-mempool descendants (including this one) with fee deltas used for mining priority, denominated in " + CURRENCY_ATOM + "s (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"}, RPCResult{RPCResult::Type::NUM, "ancestorcount", "number of in-mempool ancestor transactions (including this one)"}, RPCResult{RPCResult::Type::NUM, "ancestorsize", "virtual transaction size of in-mempool ancestors (including this one)"}, - RPCResult{RPCResult::Type::STR_AMOUNT, "ancestorfees", "modified fees (see above) of in-mempool ancestors (including this one) (DEPRECATED)"}, + RPCResult{RPCResult::Type::STR_AMOUNT, "ancestorfees", "transaction fees of in-mempool ancestors (including this one) with fee deltas used for mining priority, denominated in " + CURRENCY_ATOM + "s (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"}, RPCResult{RPCResult::Type::STR_HEX, "wtxid", "hash of serialized transaction, including witness data"}, RPCResult{RPCResult::Type::OBJ, "fees", "", { - RPCResult{RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT}, - RPCResult{RPCResult::Type::STR_AMOUNT, "modified", "transaction fee with fee deltas used for mining priority in " + CURRENCY_UNIT}, - RPCResult{RPCResult::Type::STR_AMOUNT, "ancestor", "modified fees (see above) of in-mempool ancestors (including this one) in " + CURRENCY_UNIT}, - RPCResult{RPCResult::Type::STR_AMOUNT, "descendant", "modified fees (see above) of in-mempool descendants (including this one) in " + CURRENCY_UNIT}, + RPCResult{RPCResult::Type::STR_AMOUNT, "base", "transaction fee, denominated in " + CURRENCY_UNIT}, + RPCResult{RPCResult::Type::STR_AMOUNT, "modified", "transaction fee with fee deltas used for mining priority, denominated in " + CURRENCY_UNIT}, + RPCResult{RPCResult::Type::STR_AMOUNT, "ancestor", "transaction fees of in-mempool ancestors (including this one) with fee deltas used for mining priority, denominated in " + CURRENCY_UNIT}, + RPCResult{RPCResult::Type::STR_AMOUNT, "descendant", "transaction fees of in-mempool descendants (including this one) with fee deltas used for mining priority, denominated in " + CURRENCY_UNIT}, }}, RPCResult{RPCResult::Type::ARR, "depends", "unconfirmed transactions used as inputs for this transaction", {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "parent transaction id"}}}, @@ -498,19 +498,26 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool fees.pushKV("ancestor", ValueFromAmount(e.GetModFeesWithAncestors())); fees.pushKV("descendant", ValueFromAmount(e.GetModFeesWithDescendants())); info.pushKV("fees", fees); - info.pushKV("vsize", (int)e.GetTxSize()); info.pushKV("weight", (int)e.GetTxWeight()); - info.pushKV("fee", ValueFromAmount(e.GetFee())); - info.pushKV("modifiedfee", ValueFromAmount(e.GetModifiedFee())); + // TODO: top-level fee fields are deprecated. deprecated_fee_fields_enabled blocks should be removed in v24 + const bool deprecated_fee_fields_enabled{IsDeprecatedRPCEnabled("fees")}; + if (deprecated_fee_fields_enabled) { + info.pushKV("fee", ValueFromAmount(e.GetFee())); + info.pushKV("modifiedfee", ValueFromAmount(e.GetModifiedFee())); + } info.pushKV("time", count_seconds(e.GetTime())); info.pushKV("height", (int)e.GetHeight()); info.pushKV("descendantcount", e.GetCountWithDescendants()); info.pushKV("descendantsize", e.GetSizeWithDescendants()); - info.pushKV("descendantfees", e.GetModFeesWithDescendants()); + if (deprecated_fee_fields_enabled) { + info.pushKV("descendantfees", e.GetModFeesWithDescendants()); + } info.pushKV("ancestorcount", e.GetCountWithAncestors()); info.pushKV("ancestorsize", e.GetSizeWithAncestors()); - info.pushKV("ancestorfees", e.GetModFeesWithAncestors()); + if (deprecated_fee_fields_enabled) { + info.pushKV("ancestorfees", e.GetModFeesWithAncestors()); + } info.pushKV("wtxid", pool.vTxHashes[e.vTxHashesIdx].first.ToString()); const CTransaction& tx = e.GetTx(); std::set setDepends; diff --git a/test/functional/mempool_packages.py b/test/functional/mempool_packages.py index ff5e45519f..872c178103 100755 --- a/test/functional/mempool_packages.py +++ b/test/functional/mempool_packages.py @@ -91,7 +91,7 @@ class MempoolPackagesTest(BitcoinTestFramework): assert_equal(ancestor_vsize, sum([mempool[tx]['vsize'] for tx in mempool])) ancestor_count = MAX_ANCESTORS - assert_equal(ancestor_fees, sum([mempool[tx]['fee'] for tx in mempool])) + assert_equal(ancestor_fees, sum([mempool[tx]['fees']['base'] for tx in mempool])) descendants = [] ancestors = list(chain) @@ -102,11 +102,8 @@ class MempoolPackagesTest(BitcoinTestFramework): # Check that the descendant calculations are correct assert_equal(entry['descendantcount'], descendant_count) - descendant_fees += entry['fee'] - assert_equal(entry['modifiedfee'], entry['fee']) - assert_equal(entry['fees']['base'], entry['fee']) - assert_equal(entry['fees']['modified'], entry['modifiedfee']) - assert_equal(entry['descendantfees'], descendant_fees * COIN) + descendant_fees += entry['fees']['base'] + assert_equal(entry['fees']['modified'], entry['fees']['base']) assert_equal(entry['fees']['descendant'], descendant_fees) descendant_vsize += entry['vsize'] assert_equal(entry['descendantsize'], descendant_vsize) @@ -114,10 +111,10 @@ class MempoolPackagesTest(BitcoinTestFramework): # Check that ancestor calculations are correct assert_equal(entry['ancestorcount'], ancestor_count) - assert_equal(entry['ancestorfees'], ancestor_fees * COIN) + assert_equal(entry['fees']['ancestor'], ancestor_fees) assert_equal(entry['ancestorsize'], ancestor_vsize) ancestor_vsize -= entry['vsize'] - ancestor_fees -= entry['fee'] + ancestor_fees -= entry['fees']['base'] ancestor_count -= 1 # Check that parent/child list is correct @@ -168,9 +165,8 @@ class MempoolPackagesTest(BitcoinTestFramework): ancestor_fees = 0 for x in chain: entry = self.nodes[0].getmempoolentry(x) - ancestor_fees += entry['fee'] + ancestor_fees += entry['fees']['base'] assert_equal(entry['fees']['ancestor'], ancestor_fees + Decimal('0.00001')) - assert_equal(entry['ancestorfees'], ancestor_fees * COIN + 1000) # Undo the prioritisetransaction for later tests self.nodes[0].prioritisetransaction(txid=chain[0], fee_delta=-1000) @@ -182,9 +178,8 @@ class MempoolPackagesTest(BitcoinTestFramework): descendant_fees = 0 for x in reversed(chain): entry = self.nodes[0].getmempoolentry(x) - descendant_fees += entry['fee'] + descendant_fees += entry['fees']['base'] assert_equal(entry['fees']['descendant'], descendant_fees + Decimal('0.00001')) - assert_equal(entry['descendantfees'], descendant_fees * COIN + 1000) # Adding one more transaction on to the chain should fail. assert_raises_rpc_error(-26, "too-long-mempool-chain", chain_transaction, self.nodes[0], [txid], [vout], value, fee, 1) @@ -205,11 +200,9 @@ class MempoolPackagesTest(BitcoinTestFramework): descendant_fees = 0 for x in reversed(chain): entry = self.nodes[0].getmempoolentry(x) - descendant_fees += entry['fee'] + descendant_fees += entry['fees']['base'] if (x == chain[-1]): - assert_equal(entry['modifiedfee'], entry['fee'] + Decimal("0.00002")) - assert_equal(entry['fees']['modified'], entry['fee'] + Decimal("0.00002")) - assert_equal(entry['descendantfees'], descendant_fees * COIN + 2000) + assert_equal(entry['fees']['modified'], entry['fees']['base'] + Decimal("0.00002")) assert_equal(entry['fees']['descendant'], descendant_fees + Decimal("0.00002")) # Check that node1's mempool is as expected (-> custom ancestor limit) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index b0e46c6ca7..98f91221ea 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -417,7 +417,7 @@ class RawTransactionsTest(BitcoinTestFramework): # Create same transaction over sendtoaddress. txId = self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 1.1) - signedFee = self.nodes[0].getmempoolentry(txId)['fee'] + signedFee = self.nodes[0].getmempoolentry(txId)['fees']['base'] # Compare fee. feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) @@ -443,7 +443,7 @@ class RawTransactionsTest(BitcoinTestFramework): # Create same transaction over sendtoaddress. txId = self.nodes[0].sendmany("", outputs) - signedFee = self.nodes[0].getmempoolentry(txId)['fee'] + signedFee = self.nodes[0].getmempoolentry(txId)['fees']['base'] # Compare fee. feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) @@ -470,7 +470,7 @@ class RawTransactionsTest(BitcoinTestFramework): # Create same transaction over sendtoaddress. txId = self.nodes[0].sendtoaddress(mSigObj, 1.1) - signedFee = self.nodes[0].getmempoolentry(txId)['fee'] + signedFee = self.nodes[0].getmempoolentry(txId)['fees']['base'] # Compare fee. feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) @@ -514,7 +514,7 @@ class RawTransactionsTest(BitcoinTestFramework): # Create same transaction over sendtoaddress. txId = self.nodes[0].sendtoaddress(mSigObj, 1.1) - signedFee = self.nodes[0].getmempoolentry(txId)['fee'] + signedFee = self.nodes[0].getmempoolentry(txId)['fees']['base'] # Compare fee. feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) @@ -651,7 +651,7 @@ class RawTransactionsTest(BitcoinTestFramework): # Create same transaction over sendtoaddress. txId = self.nodes[1].sendmany("", outputs) - signedFee = self.nodes[1].getmempoolentry(txId)['fee'] + signedFee = self.nodes[1].getmempoolentry(txId)['fees']['base'] # Compare fee. feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) -- cgit v1.2.3 From 2ee406ce3e9c252734cb391d85044ac389c34279 Mon Sep 17 00:00:00 2001 From: josibake Date: Thu, 12 Aug 2021 15:38:28 +0200 Subject: test: add functional test for deprecatedrpc=fees Test for old fields when `-deprecatedrpc=fees` flag is passed and verify values in the deprecated fields match values in the fees sub-object. --- .../rpc_mempool_entry_fee_fields_deprecation.py | 67 ++++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 68 insertions(+) create mode 100755 test/functional/rpc_mempool_entry_fee_fields_deprecation.py diff --git a/test/functional/rpc_mempool_entry_fee_fields_deprecation.py b/test/functional/rpc_mempool_entry_fee_fields_deprecation.py new file mode 100755 index 0000000000..82761ff7c8 --- /dev/null +++ b/test/functional/rpc_mempool_entry_fee_fields_deprecation.py @@ -0,0 +1,67 @@ +#!/usr/bin/env python3 +# Copyright (c) 2021 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 deprecation of fee fields from top level mempool entry object""" + +from test_framework.blocktools import COIN +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal +from test_framework.wallet import MiniWallet + + +def assertions_helper(new_object, deprecated_object, deprecated_fields): + for field in deprecated_fields: + assert field in deprecated_object + assert field not in new_object + + +class MempoolFeeFieldsDeprecationTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 2 + self.extra_args = [[], ["-deprecatedrpc=fees"]] + + def run_test(self): + # we get spendable outputs from the premined chain starting + # at block 76. see BitcoinTestFramework._initialize_chain() for details + self.wallet = MiniWallet(self.nodes[0]) + self.wallet.rescan_utxos() + + # we create the tx on the first node and wait until it syncs to node_deprecated + # thus, any differences must be coming from getmempoolentry or getrawmempool + tx = self.wallet.send_self_transfer(from_node=self.nodes[0]) + self.nodes[1].sendrawtransaction(tx["hex"]) + + deprecated_fields = ["ancestorfees", "descendantfees", "modifiedfee", "fee"] + self.test_getmempoolentry(tx["txid"], deprecated_fields) + self.test_getrawmempool(tx["txid"], deprecated_fields) + self.test_deprecated_fields_match(tx["txid"]) + + def test_getmempoolentry(self, txid, deprecated_fields): + + self.log.info("Test getmempoolentry rpc") + entry = self.nodes[0].getmempoolentry(txid) + deprecated_entry = self.nodes[1].getmempoolentry(txid) + assertions_helper(entry, deprecated_entry, deprecated_fields) + + def test_getrawmempool(self, txid, deprecated_fields): + + self.log.info("Test getrawmempool rpc") + entry = self.nodes[0].getrawmempool(verbose=True)[txid] + deprecated_entry = self.nodes[1].getrawmempool(verbose=True)[txid] + assertions_helper(entry, deprecated_entry, deprecated_fields) + + def test_deprecated_fields_match(self, txid): + + self.log.info("Test deprecated fee fields match new fees object") + entry = self.nodes[0].getmempoolentry(txid) + deprecated_entry = self.nodes[1].getmempoolentry(txid) + + assert_equal(deprecated_entry["fee"], entry["fees"]["base"]) + assert_equal(deprecated_entry["modifiedfee"], entry["fees"]["modified"]) + assert_equal(deprecated_entry["descendantfees"], entry["fees"]["descendant"] * COIN) + assert_equal(deprecated_entry["ancestorfees"], entry["fees"]["ancestor"] * COIN) + + +if __name__ == "__main__": + MempoolFeeFieldsDeprecationTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 916cd94b79..2fc5d2c56e 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -309,6 +309,7 @@ BASE_SCRIPTS = [ 'feature_presegwit_node_upgrade.py', 'feature_settings.py', 'rpc_getdescriptorinfo.py', + 'rpc_mempool_entry_fee_fields_deprecation.py', 'rpc_help.py', 'feature_help.py', 'feature_shutdown.py', -- cgit v1.2.3 From 07ade7db8f919826c5e69bdaf7d54a6ae653175e Mon Sep 17 00:00:00 2001 From: josibake Date: Mon, 16 Aug 2021 17:41:47 +0200 Subject: doc: add release note for fee field deprecation --- doc/release-notes.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/doc/release-notes.md b/doc/release-notes.md index b460cd3eb2..231195108b 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -99,6 +99,15 @@ Updated RPCs causes the lock to be written persistently to the wallet database. This allows UTXOs to remain locked even after node restarts or crashes. (#23065) +- The top-level fee fields `fee`, `modifiedfee`, `ancestorfees` and `descendantfees` + returned by RPCs `getmempoolentry`,`getrawmempool(verbose=true)`, + `getmempoolancestors(verbose=true)` and `getmempooldescendants(verbose=true)` + are deprecated and will be removed in the next major version (use + `-deprecated=fees` if needed in this version). The same fee fields can be accessed + through the `fees` object in the result. WARNING: deprecated + fields `ancestorfees` and `descendantfees` are denominated in sats, whereas all + fields in the `fees` object are denominated in BTC. (#22689) + New RPCs -------- -- cgit v1.2.3 From 2f9515f37addabde84c79926d7a24b2897a21dd1 Mon Sep 17 00:00:00 2001 From: josibake Date: Thu, 19 Aug 2021 15:38:58 +0200 Subject: rpc: move fees object to match help --- src/rpc/blockchain.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 2c7f1a70ae..ccfef3a7c7 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -492,12 +492,6 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool { AssertLockHeld(pool.cs); - UniValue fees(UniValue::VOBJ); - fees.pushKV("base", ValueFromAmount(e.GetFee())); - fees.pushKV("modified", ValueFromAmount(e.GetModifiedFee())); - fees.pushKV("ancestor", ValueFromAmount(e.GetModFeesWithAncestors())); - fees.pushKV("descendant", ValueFromAmount(e.GetModFeesWithDescendants())); - info.pushKV("fees", fees); info.pushKV("vsize", (int)e.GetTxSize()); info.pushKV("weight", (int)e.GetTxWeight()); // TODO: top-level fee fields are deprecated. deprecated_fee_fields_enabled blocks should be removed in v24 @@ -519,6 +513,14 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool info.pushKV("ancestorfees", e.GetModFeesWithAncestors()); } info.pushKV("wtxid", pool.vTxHashes[e.vTxHashesIdx].first.ToString()); + + UniValue fees(UniValue::VOBJ); + fees.pushKV("base", ValueFromAmount(e.GetFee())); + fees.pushKV("modified", ValueFromAmount(e.GetModifiedFee())); + fees.pushKV("ancestor", ValueFromAmount(e.GetModFeesWithAncestors())); + fees.pushKV("descendant", ValueFromAmount(e.GetModFeesWithDescendants())); + info.pushKV("fees", fees); + const CTransaction& tx = e.GetTx(); std::set setDepends; for (const CTxIn& txin : tx.vin) -- cgit v1.2.3