rpc: deprecate top-level fee fields in getmempool RPCs

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

src/rpc/blockchain.cpp

@@ -462,23 +462,23 @@ static RPCHelpMan getdifficulty()
 static std::vector<RPCResult> 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"}}},
@@ -492,26 +492,35 @@ 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());
-    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());
+
+    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<std::string> setDepends;
     for (const CTxIn& txin : tx.vin)

test/functional/mempool_packages.py

@@ -91,7 +91,7 @@ def run_test(self):
 
         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,22 +102,19 @@ def run_test(self):
 
             # 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)
             descendant_count += 1
 
             # 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 @@ def run_test(self):
         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 @@ def run_test(self):
         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 @@ def run_test(self):
         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)

test/functional/rpc_fundrawtransaction.py

@@ -417,7 +417,7 @@ def test_fee_p2pkh(self):
 
         # 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 @@ def test_fee_p2pkh_multi_out(self):
 
         # 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 @@ def test_fee_p2sh(self):
 
         # 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 @@ def test_fee_4of5(self):
 
         # 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 @@ def test_many_inputs_fee(self):
 
         # 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)

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

test/functional/test_runner.py

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