rpc: deprecate top-level fee fields in getmempool RPCs

josibake commented at 2:12 pm on August 12, 2021: member

per #22682 , top level fee fields for mempool entries have been deprecated since 0.17 but are still returned. this PR properly deprecates them so that they are no longer returned unless -deprecatedrpc=fees is passed.

the first commit takes care of deprecation and also updates test/functional/mempool_packages.py to only use the fees object. the second commit adds a new functional test for -deprecatedrpc=fees

closes #22682

questions for the reviewer

-deprecatedrpc=fees made the most sense to me, but happy to change if there is a name that makes more sense
#22682 seems to indicate that after some period of time, the fields will be removed all together. if we have a rough idea of when this will be, i can add a TODO: fully remove in vXX comment to entryToJSON

testing

to get started on testing, compile, run the tests, and start your node with the deprecated rpcs flag:

0./src/bitcoind -daemon -deprecatedrpc=fees

you should see entries with the deprecated fields like so:

 0{
 1  "<txid>": {
 2    "fees": {
 3      "base": 0.00000671,
 4      "modified": 0.00000671,
 5      "ancestor": 0.00000671,
 6      "descendant": 0.00000671
 7    },
 8    "fee": 0.00000671,
 9    "modifiedfee": 0.00000671,
10    "descendantfees": 671,
11    "ancestorfees": 671,
12    "vsize": 144,
13    "weight": 573,
14   ...
15  },

you can also check getmempoolentry using any of the txid’s from the output above.

next start the node without the deprecated flag, repeat the commands from above and verify that the deprecated fields are no longer present at the top level, but present in the “fees” object

DrahtBot added the label RPC/REST/ZMQ on Aug 12, 2021

josibake force-pushed on Aug 12, 2021

josibake commented at 3:16 pm on August 12, 2021: member

force pushed from 084d443 to 251d636 to fix linting and ci errors

josibake force-pushed on Aug 12, 2021

DrahtBot commented at 7:14 pm on August 12, 2021: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)
#22698 (Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status by mjdietzx)
#22341 (rpc: add getxpub by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake requested review from glozow on Aug 13, 2021

josibake force-pushed on Aug 13, 2021

in src/rpc/blockchain.cpp:484 in c4646f8c72 outdated

480@@ -481,7 +481,7 @@ static std::vector<RPCResult> MempoolEntryDescription() { return {
481     RPCResult{RPCResult::Type::BOOL, "unbroadcast", "Whether this transaction is currently unbroadcast (initial broadcast not yet acknowledged by any peers)"},
482 };}
483 
484-static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPoolEntry& e) EXCLUSIVE_LOCKS_REQUIRED(pool.cs)
485+static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPoolEntry& e, bool include_fee_fields) EXCLUSIVE_LOCKS_REQUIRED(pool.cs)

glozow commented at 9:37 am on August 13, 2021:

I agree with this approach of adding a bool similar to address/reqSigs, and think you should add a TODO to remove this in v24

Perhaps naming it include_old_fee_fields or something might be better because this could be slightly confusing

josibake commented at 1:00 pm on August 13, 2021:

added the comments and renamed to include_deprecated_fee_fields to better associate it with isDeprecatedRPCEnabled("fees")

in test/functional/rpc_mempool_entry_fee_fields_deprecation.py:5 in c4646f8c72 outdated

0@@ -0,0 +1,62 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2021 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test deprecation of fee fields from top level mempool entry object"""

glozow commented at 9:42 am on August 13, 2021:

Any reason you didn’t use the existing test file, rpc_deprecated.py? I don’t personally have an opinion on whether it would be better, but it looks like a good place to put these tests too.

josibake commented at 1:17 pm on August 13, 2021:

i saw rpc_deprecated.py and wasn’t sure why it was there? right now it doesn’t test anything. i then saw rpc_addresses_deprecation.py was its own file, so i followed that one as an example.

i guess the advantage of putting tests in rpc_deprecated.py is it might help things run faster than running individual test files for each deprecated rpc. i can put my test there and then move rpc_addresses_deprecation.py to rpc_deprecated.py as a follow-up pr, if that’s the standard we want to follow

glozow commented at 1:24 pm on August 13, 2021:

I assumed that rpc_deprecated.py is there to test RPC deprecation stuff and address/reqSigs tests are in a separate file because they need the wallet to be built. So they can’t be moved to rpc_deprecated.py either.

josibake commented at 1:26 pm on August 13, 2021:

actually, ive talked myself out of this. if we have individual files, they can run in parallel and if we put all the deprecated tests under the same class it could be really annoying later on as you are adding a new test. you’d have to wait for N deprecation tests to run each time as you’re iterating on your test, which starts to impact productivity if it takes 30 ~ 45 seconds to run through all the tests each time.

what are your thoughts, @glozow ?

josibake commented at 2:44 pm on August 13, 2021:

I assumed that rpc_deprecated.py is there to test RPC deprecation stuff and address/reqSigs tests are in a separate file because they need the wallet to be built

makes sense. i read through the comments on rpc_deprecated.py and it seems focused on testing that a whole rpc command is deprecated. i think it works well for that and we can consolidate rpc command deprecation’s there.

this is test slightly different in that an rpc is not being deprecated (meaning we cant just check for an rpc not found error), but fields on an rpc response are being deprecated, which makes the setup for the test a bit more complicated. for this reason, id suggest keeping this test in a separate file

glozow commented at 2:41 pm on August 19, 2021:

I don’t think it matters that much :shrug: either way makes sense to me. I’d imagine that the extra overhead of spinning up test frameworks for separate test files overrides the gains from running them in parallel.

in test/functional/rpc_mempool_entry_fee_fields_deprecation.py:33 in c4646f8c72 outdated

28+
29+        # note: we are only testing getmempoolentry even though
30+        # getrawmempool(verbose=True) also returns mempool entries
31+        # this is because getmempool entry and getrawmempool(verbose=True)
32+        # both call the same underlying entryToJSON function so testing
33+        # getmempoolentry is sufficient

glozow commented at 9:44 am on August 13, 2021:

You could also add the assertions to a helper function, and call that helper function with results you get from each RPC

in test/functional/rpc_mempool_entry_fee_fields_deprecation.py:10 in c4646f8c72 outdated

 5+"""Test deprecation of fee fields from top level mempool entry object"""
 6+
 7+from test_framework.wallet import MiniWallet
 8+from test_framework.test_framework import BitcoinTestFramework
 9+from test_framework.util import assert_equal
10+from test_framework.blocktools import COIN

glozow commented at 9:50 am on August 13, 2021:

nit: sort alphabetically :grin:

josibake commented at 2:50 pm on August 13, 2021:

:doh:

in test/functional/rpc_fundrawtransaction.py:383 in c4646f8c72 outdated

378@@ -379,7 +379,7 @@ def test_fee_p2pkh(self):
379 
380         # Create same transaction over sendtoaddress.
381         txId = self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 1.1)
382-        signedFee = self.nodes[0].getrawmempool(True)[txId]['fee']
383+        signedFee = self.nodes[0].getrawmempool(True)[txId]['fees']['base']

glozow commented at 9:57 am on August 13, 2021:

To make the commits atomic, rpc_fundrawtransaction.py needs to be updated in 5adcba2562 as well

josibake commented at 1:03 pm on August 13, 2021:

hm, im not sure how you were able to see https://github.com/bitcoin/bitcoin/commit/5adcba25623f35c07e8fe266883a9c21931f3d0d as i thought i force pushed past it. but just to make sure i understand, the changes to rpc_fundrawtransaction.py and mempool_packages.py should be in the same commit as the changes to blockchain.cpp. this ensures the tests still pass for this commit, correct?

glozow commented at 1:26 pm on August 13, 2021:

ah hm, I must have fetched before you pushed.

just to make sure i understand, the changes to rpc_fundrawtransaction.py and mempool_packages.py should be in the same commit as the changes to blockchain.cpp. this ensures the tests still pass for this commit, correct?

correct, yes :)

glozow commented at 9:57 am on August 13, 2021: member

Concept and approach ACK

theStack commented at 10:26 am on August 13, 2021: contributor

Concept ACK

Zero-1729 commented at 2:02 pm on August 13, 2021: contributor

Concept ACK

josibake force-pushed on Aug 13, 2021

josibake commented at 2:50 pm on August 13, 2021: member

thanks for the review, @glozow! I added your suggestions and left a comment on why I think it makes sense to keep rpc_mempool_entry_fields_deprecation.py separate from rpc_deprecated.py; ultimately, I’m happy to go with whichever option you recommend

in src/rpc/blockchain.cpp:495 in 6940fe0620 outdated

491@@ -491,19 +492,20 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool
492     fees.pushKV("ancestor", ValueFromAmount(e.GetModFeesWithAncestors()));
493     fees.pushKV("descendant", ValueFromAmount(e.GetModFeesWithDescendants()));
494     info.pushKV("fees", fees);
495-
496+    if (include_deprecated_fee_fields) {

mjdietzx commented at 6:07 am on August 16, 2021:

Is there a reason you don’t do:

0if (IsDeprecatedRPCEnabled("fees")) {

here? And not have to change the function header?

josibake commented at 3:29 pm on August 16, 2021:

i was following ScriptPubKeyToUniv as an example: https://github.com/bitcoin/bitcoin/blob/42b00a37580a11fbb81a3748d9e834ae8913cdba/src/rpc/blockchain.cpp#L1936-L1939

personally, i agree with this approach more than using the IsDeprecatedRPCEnabled function directly as it it’s more clear, imo, what’s happening and allows for more flexibility.

for example, lets say we wanted to only allow getmempoolentry to return the fields when -deprecated=fees is passed but wanted getrawmempool to never return the fields. with this setup, we can have one of the functions use the original function signature with IsDeprecated passed as the default, whereas we could call entryToJSON with false passed as the default whenever it’s used in getrawmempool.

tldr; i think functional overloading is super cool and this seemed a good use case for it :smile:

maflcko commented at 7:54 pm on August 18, 2021:

As this entryToJSON isn’t called anywhere, except for entryToJSON, I think it is fine to use the suggestion by @mjdietzx . No strong opinion, though.

jonatack commented at 8:08 pm on August 18, 2021:

I agree as well, but no big deal so didn’t mention it.

josibake commented at 10:58 am on August 19, 2021:

seems like its the more preferred approach, happy to switch

mjdietzx commented at 6:09 am on August 16, 2021: contributor

Code review ACK

You’ll probably need to add release notes, something like 90ae3d8ca68334ec712d67b21a8d4721c6d79788

josibake force-pushed on Aug 16, 2021

josibake commented at 3:47 pm on August 16, 2021: member

Code review ACK

You’ll probably need to add release notes, something like 90ae3d8

thanks for the review, @mjdietzx ! ive updated the release notes in https://github.com/bitcoin/bitcoin/pull/22689/commits/4df6ffd4dcdfd2b26457c420cdbb4c382bc7a488

josibake requested review from glozow on Aug 17, 2021

fanquake commented at 11:45 am on August 17, 2021: member

thanks for the review, @mjdietzx ! ive updated the release notes in 4df6ffd

Please write more descriptive commit messages than update release notes. i.e This could be something like doc: add release note for fee field deprecation in RPC.

josibake force-pushed on Aug 17, 2021

josibake commented at 12:06 pm on August 17, 2021: member

Please write more descriptive commit messages than update release notes. i.e This could be something like doc: add release note for fee field deprecation in RPC.

great callout, @fanquake . ive update all three commit messages with relevant tags (rpc, test, doc) and wrote more descriptive commit messages

in doc/release-notes.md:67 in c3a2842134 outdated

62@@ -63,6 +63,8 @@ P2P and network changes
63 
64 Updated RPCs
65 ------------
66+- `getmempoolentry` and `getrawmempool true` no longer return top level fee fields `fee`, `modifiedfee`, `ancestorfees`, `descendantfees`. These fields were deprecated in `0.17` and moved to the `fees` sub-object in the response.
67+The `-deprecated=fees` flag must be passed for these fields to be included in the top level response. Note: this flag will only be available until `v24`, after which the fields will be fully removed. It is recommended you migrate to using the `fees` sub-object in the response

jonatack commented at 1:37 pm on August 18, 2021:

suggestions

add the PR number
add getmempoolancestors and getmempooldescendants that are also changed
s/true/verbose=true/ for clarity
wrap the lines
s/sub-object/JSON object/
s/top level/top-level/
no need to wrap version numbers in code markup

 0-- `getmempoolentry` and `getrawmempool true` no longer return top level fee fields `fee`, `modifiedfee`, `ancestorfees`, `descendantfees`. These fields were deprecated in `0.17` and moved to the `fees` sub-object in the response.
 1-The `-deprecated=fees` flag must be passed for these fields to be included in the top level response. Note: this flag will only be available until `v24`, after which the fields will be fully removed. It is recommended you migrate to using the `fees` sub-object in the response
 2+- RPCs `getmempoolentry`, `getrawmempool verbose=true`, `getmempoolancestors
 3+  verbose=true` and `getmempooldescendants verbose=true` no longer return the
 4+  top-level fee fields `fee`, `modifiedfee`, `ancestorfees` and
 5+  `descendantfees`. These fields were deprecated in v0.17 and moved to the
 6+  `fees` JSON object in the response. The `-deprecated=fees` configuration
 7+  option must be passed for these fields to be included in the top-level
 8+  response. This option will only be available until v24.0, after which the
 9+  fields will be fully removed. Use the `fees` JSON object for these RPCS
10+  instead. (#22689)

jonatack commented at 1:58 pm on August 18, 2021: contributor

Code review ACK modulo the comment below and updating the first commit message, as getmempoolancestors and getmempooldescendants are also changed unless I’m misreading the code

Edit: perhaps rename the PR to “rpc: deprecate top-level fee fields in getmempool RPCs”

josibake renamed this:
~~rpc: properly deprecate mempool entry fee fields~~
rpc: deprecate top-level fee fields in getmempool RPCs
on Aug 18, 2021

josibake force-pushed on Aug 18, 2021

josibake commented at 7:26 pm on August 18, 2021: member

getmempoolancestors and getmempooldescendants are also changed unless I’m misreading the code

good catch! i verified by grepping for entryToJSON and it is used in those RPC’s as well. updated the commit message and release docs. thanks for the suggestions on the release doc; it reads much better

josibake requested review from jonatack on Aug 18, 2021

josibake force-pushed on Aug 18, 2021

josibake commented at 7:33 pm on August 18, 2021: member

force pushed https://github.com/bitcoin/bitcoin/pull/22689/commits/e5b22ecc858f03be29a8f5aa1802b679a782a593 to fix a failure in the linter

josibake force-pushed on Aug 19, 2021

josibake commented at 11:16 am on August 19, 2021: member

force pushed https://github.com/bitcoin/bitcoin/pull/22689/commits/07801c89d6782d404370a52566a55375f36846d7 based on #22689 (review)

josibake requested review from maflcko on Aug 19, 2021

in doc/release-notes.md:72 in b409e33c68 outdated

67+  verbose=true` and `getmempooldescendants verbose=true` no longer return the
68+  top-level fee fields `fee`, `modifiedfee`, `ancestorfees` and
69+  `descendantfees`. These fields were deprecated in v0.17 and moved to the
70+  `fees` JSON object in the response. The `-deprecated=fees` configuration
71+  option must be passed for these fields to be included in the top-level
72+  response. This option will only be available until v24.0, after which the

maflcko commented at 11:27 am on August 19, 2021:

nit: It won’t be available in v24.0

0  response. This option will only be available in this major version, after which the

in src/rpc/blockchain.cpp:484 in b409e33c68 outdated

480@@ -481,6 +481,7 @@ static std::vector<RPCResult> MempoolEntryDescription() { return {
481     RPCResult{RPCResult::Type::BOOL, "unbroadcast", "Whether this transaction is currently unbroadcast (initial broadcast not yet acknowledged by any peers)"},
482 };}
483 
484+// TODO: top level fee fields are deprecated. this method should be refactored in v24 to completely remove them

maflcko commented at 11:28 am on August 19, 2021:

nit: Could move the comment to the if IsDeprecatedRPCEnabled?

in test/functional/mempool_packages.py:193 in b409e33c68 outdated

192+            descendant_fees += mempool[x]['fees']['base']
193             if (x == chain[-1]):
194-                assert_equal(mempool[x]['modifiedfee'], mempool[x]['fee']+satoshi_round(0.00002))
195-                assert_equal(mempool[x]['fees']['modified'], mempool[x]['fee']+satoshi_round(0.00002))
196-            assert_equal(mempool[x]['descendantfees'], descendant_fees * COIN + 2000)
197+                assert_equal(mempool[x]['fees']['modified'], mempool[x]['fees']['base']+satoshi_round(0.00002))

maflcko commented at 11:31 am on August 19, 2021:

nit: pep-8

0                assert_equal(mempool[x]['fees']['modified'], mempool[x]['fees']['base'] + satoshi_round(0.00002))

maflcko approved

maflcko commented at 11:32 am on August 19, 2021: member

cr ACK b409e33c682a37ecd965b6d9dc2cae35ee672b59

josibake force-pushed on Aug 19, 2021

josibake commented at 11:44 am on August 19, 2021: member

thanks for the review, @MarcoFalke , ive updated based on your suggestions

maflcko commented at 11:46 am on August 19, 2021: member

re-ACK a0a0c7c1c8e41157bb2798ba0d564e78e5217ff9

mjdietzx commented at 11:49 am on August 19, 2021: contributor

reACK a0a0c7c1c8e41157bb2798ba0d564e78e5217ff9 well done 👍

in src/rpc/blockchain.cpp:502 in a0a0c7c1c8 outdated

496+    if (IsDeprecatedRPCEnabled("fees")) {
497+        info.pushKV("fee", ValueFromAmount(e.GetFee()));
498+        info.pushKV("modifiedfee", ValueFromAmount(e.GetModifiedFee()));
499+        info.pushKV("descendantfees", e.GetModFeesWithDescendants());
500+        info.pushKV("ancestorfees", e.GetModFeesWithAncestors());
501+    }

jonatack commented at 12:13 pm on August 19, 2021:

Might be more user-friendly to not alter the field order here so that it is the same as the help. E.g. insert the IsDeprecated… conditional (or cache the result of the call to a local bool variable) before the fields where they already are. (Sorry, I didn’t notice this in my previous review.)

maflcko commented at 12:23 pm on August 19, 2021:

The code will go away soon, so I don’t think this gives much benefit.

josibake commented at 1:05 pm on August 19, 2021:

i noticed that the current order doesn’t match the help (fees object is listed after wtxid but is first in the returned object), so didn’t worry about re-arranging fields, but i agree that it can be confusing for the user.

i think grouping the fields makes it easier to deprecate them later, so maybe a better solution would be to re-arrange the help to match the actual ordering? or just leave it as is per @MarcoFalke ’s comment, i dont have a strong opinion either way

jonatack commented at 1:14 pm on August 19, 2021:

I would say, please don’t re-arrange the help to make up for something the code can easily do.

Yes, I agree it’s not of earth-shattering importance, but I also think it’s a more important aspect than in which file to put the deprecation test or which way to check for deprecation. It affects users.

josibake commented at 1:21 pm on August 19, 2021:

I guess it’s a question of degree of focus on users

good point, @jonatack , i’ll update based on your suggestion

jonatack commented at 1:23 pm on August 19, 2021:

Happy to re-review ASAP if you do (it’s not a blocker).

jonatack commented at 3:30 pm on August 19, 2021:

i noticed that the current order doesn’t match the help (fees object is listed after wtxid but is first in the returned object)

Oh, I see now what you meant. Hm, the fees output/help mismatch was done in 7de1de7da49. ISTM we usually place JSON objects after top-level fields? So your change ~seems correct, unless other reviewers think it’s better to change the help to be like the output.

josibake commented at 4:01 pm on August 19, 2021:

JSON objects after top-level fields

that’s the convention i’m used to (and much prefer)

josibake force-pushed on Aug 19, 2021

josibake commented at 1:49 pm on August 19, 2021: member

pushed changes per @jonatack ’s suggestion to preserve field order to match rpc help. i also added https://github.com/bitcoin/bitcoin/pull/22689/commits/c1a8a35e89d5b3c889d64562c2e5f042c16a4ae2 to make the fees object also match the help. given the discussion in #22689 (review) , this seemed like a good change

i did it in a separate commit because it is slightly unrelated to deprecating the fields and i can easily drop the commit if people feel its too out of scope.

josibake requested review from jonatack on Aug 19, 2021

in doc/release-notes.md:74 in c1a8a35e89 outdated

69+  `descendantfees`. These fields were deprecated in v0.17 and moved to the
70+  `fees` JSON object in the response. The `-deprecated=fees` configuration
71+  option must be passed for these fields to be included in the top-level
72+  response. This option will only be available in this major version, after
73+  which the fields will be fully removed. Use the `fees` JSON object for these
74+  RPCS instead. (#22689)

glozow commented at 2:32 pm on August 19, 2021:

Mostly wording suggestions, but also I think you should warn users about the fact that the fees are denominated slightly differently when they switch to using the fees object.

0- The top-level fee fields `fee`, `modifiedfee`, `ancestorfees` and `descendantfees`
1  returned by RPCs `getmempoolentry`,`getrawmempool(verbose=true)`,
2  `getmempoolancestors(verbose=true)` and `getmempooldescendants(verbose=true)`
3  are now deprecated and will be removed in the next major version (use
4  `-deprecated=fees` if needed in this version). The same values, denominated
5  in satoshis, can be accessed through the `fees` object in the result. (#22689)

glozow commented at 2:33 pm on August 19, 2021:

Doesn’t seem to be an issue here, but for future reference, you might want to put it in a release-notes-22689.md to avoid conflicts with other PRs updating release notes.

josibake commented at 3:51 pm on August 19, 2021:

slightly differently when they switch to using the fees object

just double checking, the update should read “denominated in BTC,” correct?

glozow commented at 10:39 am on August 25, 2021:

yes, BTC, sorry

glozow commented at 2:53 pm on August 19, 2021: member

utACK, Are we missing tests for getmempooldescendants and getmempoolancestors?

in src/rpc/blockchain.cpp:513 in c1a8a35e89 outdated

510+        info.pushKV("descendantfees", e.GetModFeesWithDescendants());
511     info.pushKV("ancestorcount", e.GetCountWithAncestors());
512     info.pushKV("ancestorsize", e.GetSizeWithAncestors());
513-    info.pushKV("ancestorfees", e.GetModFeesWithAncestors());
514+    if (IsDeprecatedRPCEnabled("fees"))
515+        info.pushKV("ancestorfees", e.GetModFeesWithAncestors());

jonatack commented at 3:13 pm on August 19, 2021:

brackets per developer-notes.md:

0  - If an `if` only has a single-statement `then`-clause, it can appear
1    on the same line as the `if`, without braces. In every other case,
2    braces are required, and the `then` and `else` clauses must appear
3    correctly indented on a new line.

IsDeprecatedRPCEnabled() loops through a vector to match strings, so maybe invoke it once and store the result

 0@@ -493,7 +493,8 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool
 1     info.pushKV("vsize", (int)e.GetTxSize());
 2     info.pushKV("weight", (int)e.GetTxWeight());
 3     // TODO: top-level fee fields are deprecated. IsDeprecatedRPCEnabled("fees") blocks should be removed in v24
 4-    if (IsDeprecatedRPCEnabled("fees")) {
 5+    const bool deprecated_fees_enabled{IsDeprecatedRPCEnabled("fees")};
 6+    if (deprecated_fees_enabled) {
 7         info.pushKV("fee", ValueFromAmount(e.GetFee()));
 8         info.pushKV("modifiedfee", ValueFromAmount(e.GetModifiedFee()));
 9     }
10@@ -501,12 +502,14 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool
11     info.pushKV("height", (int)e.GetHeight());
12     info.pushKV("descendantcount", e.GetCountWithDescendants());
13     info.pushKV("descendantsize", e.GetSizeWithDescendants());
14-    if (IsDeprecatedRPCEnabled("fees"))
15+    if (deprecated_fees_enabled) {
16         info.pushKV("descendantfees", e.GetModFeesWithDescendants());
17+    }
18     info.pushKV("ancestorcount", e.GetCountWithAncestors());
19     info.pushKV("ancestorsize", e.GetSizeWithAncestors());
20-    if (IsDeprecatedRPCEnabled("fees"))
21+    if (deprecated_fees_enabled) {
22         info.pushKV("ancestorfees", e.GetModFeesWithAncestors());
23+    }

josibake force-pushed on Aug 19, 2021

josibake commented at 4:14 pm on August 19, 2021: member

pushed changes per @jonatack suggestions and @glozow ’s recommendation on the release notes. if the release notes need further refinement, i think we can wait until the draft is moved to the wiki. ive made a note for myself to revisit then. @glozow , regarding your question on missing tests for getmempoolancestors and getmempooldescendants: i had started to write tests for these, but since each RPC is calling entryToJSON underneath, it didn’t seem like there was any added benefit and creating the tests was quite a bit more complicated than the the ones for getrawmempool and getmempoolentry. as i understand, rpc_mempool_entry_fee_fields_deprecation.py will be removed in the in v24 anyways, so i dont see the benefit of writing them, unless others feel strongly that they are needed.

thanks again for the reviews!

in doc/release-notes.md:71 in 30563b4bf3 outdated

62@@ -63,6 +63,12 @@ P2P and network changes
63 
64 Updated RPCs
65 ------------
66+- The top-level fee fields `fee`, `modifiedfee`, `ancestorfees` and `descendantfees`
67+  returned by RPCs `getmempoolentry`,`getrawmempool(verbose=true)`,
68+  `getmempoolancestors(verbose=true)` and `getmempooldescendants(verbose=true)`
69+  are now deprecated and will be removed in the next major version (use
70+  `-deprecated=fees` if needed in this version). The same values, denominated
71+  in BTC, can be accessed through the `fees` object in the result. (#22689)

jonatack commented at 4:36 pm on August 19, 2021:

Seems no need to mention the units if they aren’t changed; didn’t verify yet.

Feel free to ignore (the help prints the wrong unit as well, I believe)

0  in BTC/kvB, can be accessed through the `fees` object in the result. (#22689)

josibake commented at 5:17 pm on August 19, 2021:

yeah, i think the help is incorrect here. i can open a followup RPC cleanup PR. for starters:

address where argument order doesn’t match help order in other rpcs
fix fee vs feerate and other small errors

jonatack commented at 4:16 pm on August 23, 2021:

So, fee and modifiedfee are uniformly in BTC/kvB. OTOH, it looks to me like ancestorfees and descendantfees are in sat/vB in the deprecated fields and in BTC/kvB in the fees object (urgh, we’ve been moving from BTC/kvB to sat/vB elsewhere).

If we mention this in the release notes (and I think it’s a good idea as @glozow suggested), it may be best to (a) use the correct units of BTC/kvB and (b) clarify which fields change from sat/vB to BTC/kvB (ancestorfees and descendantfees) in the release notes.

Finally, here’s a commit in https://github.com/jonatack/bitcoin/commits/pr22689-help-fixups that both fixes and improves the docs for these fields, feel free to use or pull it in here since these lines are already being touched (or later, if you prefer) as the helps could use some…help :)

glozow commented at 10:59 am on August 25, 2021:

Note that the units for ancestor and descendant fees are different - they were in satoshis and are now in BTC

jonatack commented at 4:47 pm on August 19, 2021: contributor

ACK 30563b4bf32b24a25a2f40d2e48e5d64cc79f5c2

DrahtBot added the label Needs rebase on Aug 20, 2021

josibake force-pushed on Aug 23, 2021

josibake commented at 2:14 pm on August 23, 2021: member

rebased to fix conflicts from #22707 @MarcoFalke @mjdietzx @jonatack @glozow if everything looks good, a re-ack would be much appreciated! thanks!

DrahtBot removed the label Needs rebase on Aug 23, 2021

josibake force-pushed on Aug 24, 2021

josibake commented at 10:49 am on August 24, 2021: member

@jonatack thanks for the fix-ups on the field help! i added your suggestions and included you as a co-author in https://github.com/bitcoin/bitcoin/pull/22689/commits/4a6e7b2f0321df876e1d90d06c44325116bf5eec. i also updated the release note docs in https://github.com/bitcoin/bitcoin/pull/22689/commits/61a5b370594b538488dbddc4aadb208c2770d748 to be more clear that ancestorfees and descendantfees were previously sat/vB and are now BTC/kvB in the fees object.

in src/rpc/blockchain.cpp:522 in 19b482002f outdated

515-    info.pushKV("ancestorfees", e.GetModFeesWithAncestors());
516+    if (deprecated_fee_fields_enabled) {
517+        info.pushKV("ancestorfees", e.GetModFeesWithAncestors());
518+    }
519     info.pushKV("wtxid", pool.vTxHashes[e.vTxHashesIdx].first.ToString());
520+    info.pushKV("fees", fees);

jnewbery commented at 1:57 pm on August 24, 2021:

I don’t think the last commit needs to be in this PR, since it’s not really related to deprecating the legacy fields.

If you do move this down, then it makes sense to also move the preceding lines down so the fees code isn’t split up:

0    UniValue fees(UniValue::VOBJ);
1    fees.pushKV("base", ValueFromAmount(e.GetFee()));
2    fees.pushKV("modified", ValueFromAmount(e.GetModifiedFee()));
3    fees.pushKV("ancestor", ValueFromAmount(e.GetModFeesWithAncestors()));
4    fees.pushKV("descendant", ValueFromAmount(e.GetModFeesWithDescendants()));

jnewbery commented at 10:57 am on November 1, 2021:

nit: add a blank line after info.pushKV("fees", fees); since these 6 lines form a logical block.

jnewbery commented at 1:58 pm on August 24, 2021: contributor

Concept ACK

josibake force-pushed on Aug 24, 2021

josibake commented at 3:19 pm on August 24, 2021: member

thanks for the review @jnewbery ! i added your suggestion in https://github.com/bitcoin/bitcoin/pull/22689/commits/0dd79b51a3de0cc618704077f71aa591cdc8c01e

while i agree moving the fee object is not directly related to fee field deprecation, my (albeit weak) argument for keeping it in this PR is making sure the descriptions are accurate for the affected RPC’s will help avoid confusion on the affected user’s part as they are looking at the updated descriptions and returned objects and migrating to the new fee fields.

that being said, very easy to pop this commit off and include it in a follow up PR for cleaning up RPC help text discrepancies

in doc/release-notes.md:73 in 0dd79b51a3 outdated

68+  `getmempoolancestors(verbose=true)` and `getmempooldescendants(verbose=true)`
69+  are now deprecated and will be removed in the next major version (use
70+  `-deprecated=fees` if needed in this version). The same fee rates, all denominated
71+  in BTC/kvB, can be accessed through the `fees` object in the result. Note: deprecated
72+  fields `ancestorfees` and `descendantfees` are denominated in sat/vB, whereas all
73+  fields in the `fees` object are denomainted in BTC/kvB (#22689)

jonatack commented at 5:06 pm on August 24, 2021:

0  fields in the `fees` object are denominated in BTC/kvB (#22689)

(test/lint/lint-spelling.sh is helpful for catching some spelling errors, but it doesn’t appear to check the release notes)

perhaps s/Note:/Warning: (either it matters enough to warn, or “note” is unneeded)
also, add a blank line between “Updated RPCs” and your entry

josibake commented at 7:43 pm on August 24, 2021:

oof, good catch. i need to enable spell-check for vim :roll_eyes:

josibake force-pushed on Aug 24, 2021

josibake commented at 7:44 pm on August 24, 2021: member

force pushed update to release notes fixing wording and a spelling error

in src/rpc/blockchain.cpp:474 in a7ef2cbc46 outdated

477-            RPCResult{RPCResult::Type::STR_AMOUNT, "ancestor", "modified fees (see above) of in-mempool ancestors (including this one) in " + CURRENCY_UNIT},
478-            RPCResult{RPCResult::Type::STR_AMOUNT, "descendant", "modified fees (see above) of in-mempool descendants (including this one) in " + CURRENCY_UNIT},
479+            RPCResult{RPCResult::Type::STR_AMOUNT, "base", "transaction fee rate in " + CURRENCY_UNIT + "/kvB"},
480+            RPCResult{RPCResult::Type::STR_AMOUNT, "modified", "modified fee rate in " + CURRENCY_UNIT + "/kvB with fee rate deltas used for mining priority"},
481+            RPCResult{RPCResult::Type::STR_AMOUNT, "ancestor", "transaction fee rate of in-mempool ancestors (including this one) in " + CURRENCY_UNIT + "/kvB with fee rate deltas used for mining priority"},
482+            RPCResult{RPCResult::Type::STR_AMOUNT, "descendant", "transaction fee rate of in-mempool descendants (including this one) in " + CURRENCY_UNIT + "/kvB with fee rate deltas used for mining priority"},

glozow commented at 10:51 am on August 25, 2021:

CTxMemPoolEntry.GetModifiedFee(), CTxMemPoolEntry.GetFee(), CTxMemPoolEntry.GetModFeesWithAncestors(), CTxMemPoolEntry.GetModFeesWithDescendants() return CAmounts. See their declarations in txmempool.h.

We aren’t dividing these fee amounts by sizes, just pushing them directly into the result object in entryToJSON.

jonatack commented at 3:26 pm on August 25, 2021:

Interesting. In CTxMemPool::PrioritiseTransaction(), is TxMempoolInfo::nFeeDelta incorrectly described as a fee rate? In BIP22 it’s described as a fee.

0LogPrintf("PrioritiseTransaction: %s feerate += %s\n", hash.ToString(), FormatMoney(nFeeDelta));

josibake commented at 3:52 pm on August 25, 2021:

you are correct! my apologies, i should have double-checked the declarations before making the change :grimacing:

jnewbery commented at 3:00 pm on August 26, 2021:

@jonatack Yes, that log is incorrect.

For historic context, it was added here: https://github.com/bitcoin/bitcoin/commit/f9b9371c6027905f73a2558d6bcaca8a355c28a6#diff-c065d4cd2398ad0dbcef393c5dfc53f465bf44723348892395fffd2fb3bac522R932. I expect the author intended to update a different log which says fee but prints feerate: #9602 (review).

jonatack commented at 3:23 pm on August 26, 2021:

@jnewbery thanks for the context and history! :heart_eyes:

in doc/release-notes.md:72 in a7ef2cbc46 outdated

63@@ -64,6 +64,15 @@ P2P and network changes
64 Updated RPCs
65 ------------
66 
67+- The top-level fee fields `fee`, `modifiedfee`, `ancestorfees` and `descendantfees`
68+  returned by RPCs `getmempoolentry`,`getrawmempool(verbose=true)`,
69+  `getmempoolancestors(verbose=true)` and `getmempooldescendants(verbose=true)`
70+  are now deprecated and will be removed in the next major version (use
71+  `-deprecated=fees` if needed in this version). The same fee rates, all denominated
72+  in BTC/kvB, can be accessed through the `fees` object in the result. Warning: deprecated

glozow commented at 10:57 am on August 25, 2021:

0  `-deprecated=fees` if needed in this version). The same fee, all denominated
1  in BTC, can be accessed through the `fees` object in the result. Warning: deprecated

glozow changes_requested

glozow commented at 10:58 am on August 25, 2021: member

I’m sorry but I don’t think it’s correct to relabel these as feerates instead of fees. It’s possible there are places where we are confusing the two, but this is not one of those cases.

josibake force-pushed on Aug 25, 2021

josibake commented at 3:56 pm on August 25, 2021: member

as @glozow pointed out, all fee fields returned by entryToJSON are absolute amounts (not fee rates). ive update the rpc help and release notes to reflect this (thanks @glozow , sorry for the churn on this).

also kept the wording changes from @jonatack as i think they really improve the readability of the help and i added in the units for each of the fields so it’s more obvious that we are changing ancestorfees and descendantfees from sats to BTC

in test/functional/rpc_mempool_entry_fee_fields_deprecation.py:55 in 4792072c50 outdated

50+        entry = self.nodes[0].getrawmempool(verbose=True)[txid]
51+        deprecated_entry = self.nodes[1].getrawmempool(verbose=True)[txid]
52+        assertions_helper(entry, deprecated_entry, deprecated_fields)
53+
54+    def test_deprecated_fields_match(self, txid):
55+

glozow commented at 11:07 am on September 1, 2021:

nit in e2cada0ce5: these newlines are unnecessary

 0    def test_getmempoolentry(self, txid, deprecated_fields):
 1        self.log.info("Test getmempoolentry rpc")
 2        entry = self.nodes[0].getmempoolentry(txid)
 3        deprecated_entry = self.nodes[1].getmempoolentry(txid)
 4        assertions_helper(entry, deprecated_entry, deprecated_fields)
 5
 6    def test_getrawmempool(self, txid, deprecated_fields):
 7        self.log.info("Test getrawmempool rpc")
 8        entry = self.nodes[0].getrawmempool(verbose=True)[txid]
 9        deprecated_entry = self.nodes[1].getrawmempool(verbose=True)[txid]
10        assertions_helper(entry, deprecated_entry, deprecated_fields)
11
12    def test_deprecated_fields_match(self, txid):

josibake commented at 3:48 pm on October 28, 2021:

:+1:

glozow commented at 11:27 am on December 7, 2021:

newlines still there?

glozow commented at 11:22 am on September 1, 2021: member

ACK 4792072c50af1d3cefa0899fb914f9306ee3a57c

Doc improvements can be nice, but I personally don’t recommend refactoring/rewording things that you intend to remove, as it requires more attention from reviewers but adds little value.

pinging @jonatack, if you have time to take another look?

in doc/release-notes.md:74 in 4792072c50 outdated

69+  `getmempoolancestors(verbose=true)` and `getmempooldescendants(verbose=true)`
70+  are now deprecated and will be removed in the next major version (use
71+  `-deprecated=fees` if needed in this version). The same fee fields can be accessed
72+  through the `fees` object in the result. WARNING: deprecated
73+  fields `ancestorfees` and `descendantfees` are denominated in sats, whereas all
74+  fields in the `fees` object are denominated in BTC (#22689)

jonatack commented at 12:41 pm on September 2, 2021:

0  fields in the `fees` object are denominated in BTC. (#22689)

in doc/release-notes.md:70 in 4792072c50 outdated

63@@ -64,6 +64,15 @@ P2P and network changes
64 Updated RPCs
65 ------------
66 
67+- The top-level fee fields `fee`, `modifiedfee`, `ancestorfees` and `descendantfees`
68+  returned by RPCs `getmempoolentry`,`getrawmempool(verbose=true)`,
69+  `getmempoolancestors(verbose=true)` and `getmempooldescendants(verbose=true)`
70+  are now deprecated and will be removed in the next major version (use

jonatack commented at 12:42 pm on September 2, 2021:

nit, can omit

0  are deprecated and will be removed in the next major version (use

in doc/release-notes.md:106 in 4792072c50 outdated

63@@ -64,6 +64,15 @@ P2P and network changes
64 Updated RPCs
65 ------------
66 
67+- The top-level fee fields `fee`, `modifiedfee`, `ancestorfees` and `descendantfees`
68+  returned by RPCs `getmempoolentry`,`getrawmempool(verbose=true)`,
69+  `getmempoolancestors(verbose=true)` and `getmempooldescendants(verbose=true)`
70+  are now deprecated and will be removed in the next major version (use
71+  `-deprecated=fees` if needed in this version). The same fee fields can be accessed

jonatack commented at 12:42 pm on September 2, 2021:

0  the `-deprecated=fees` configuration option if needed in this version). The same fee fields can be accessed

in src/rpc/blockchain.cpp:474 in 4792072c50 outdated

466-    RPCResult{RPCResult::Type::STR_AMOUNT, "descendantfees", "modified fees (see above) of in-mempool descendants (including this one) (DEPRECATED)"},
467+    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)"},
468     RPCResult{RPCResult::Type::NUM, "ancestorcount", "number of in-mempool ancestor transactions (including this one)"},
469     RPCResult{RPCResult::Type::NUM, "ancestorsize", "virtual transaction size of in-mempool ancestors (including this one)"},
470-    RPCResult{RPCResult::Type::STR_AMOUNT, "ancestorfees", "modified fees (see above) of in-mempool ancestors (including this one) (DEPRECATED)"},
471+    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)"},

jonatack commented at 1:13 pm on September 2, 2021:

only if you retouch, here and line 464

0    RPCResult{RPCResult::Type::STR_AMOUNT, "ancestorfees", "transaction fees of in-mempool ancestors (including this one) with fee deltas used for mining priority, denominated in satoshis (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"},

jonatack commented at 1:17 pm on September 2, 2021: contributor

ACK 4792072c50af1d3cefa0899fb914f9306ee3a57c

modulo some release note fixups and maybe s/sats/satoshis/.

I’m glad to see the helps for the four RPCs updated consistently/coherently for both the non-deprecated and deprecated fields, as otherwise it could be confusing for users of 0.23, both while it is the newest release and also afterward for as long as users can run it, which could be a very long time.

Sorry for the time to re-review, will be quicker if you retouch as the changes should be very easy to re-review.

DrahtBot added the label Needs rebase on Sep 20, 2021

maflcko referenced this in commit eb180d807a on Sep 21, 2021

josibake force-pushed on Oct 28, 2021

DrahtBot removed the label Needs rebase on Oct 28, 2021

josibake force-pushed on Oct 28, 2021

josibake commented at 3:43 pm on October 28, 2021: member

rebased and force pushed. the rebase changes can be checked with git range-diff 4792072...61af49c

also removed the extra newlines from the newly added functional test as suggested by @glozow and included the two wording changes from @jonatack in doc/release-notes.md as part of the rebase

josibake commented at 3:55 pm on October 28, 2021: member

looking at ci failures, not immediately clear to me what’s happening - will take a look tomorrow

in test/functional/rpc_mempool_entry_fee_fields_deprecation.py:28 in 61af49cf21 outdated

23+
24+    def run_test(self):
25+        # we get spendable outputs from the premined chain starting
26+        # at block 76. see BitcoinTestFramework._initialize_chain() for details
27+        self.wallet = MiniWallet(self.nodes[0])
28+        self.wallet.scan_blocks(start=76, num=1)

glozow commented at 9:53 am on October 29, 2021:

CI issue is here, silent merge conflict with #23017. scan_blocks was removed in fa7e3f1fc114056967963a4ad4863a56e406c57e

josibake commented at 2:15 pm on October 29, 2021:

thanks! forgot to run the new test locally :man_facepalming:

josibake force-pushed on Oct 29, 2021

josibake commented at 2:22 pm on October 29, 2021: member

one-line change to fix the newly added test (git range-diff 61af49c...285cb3d)

jnewbery commented at 11:00 am on November 1, 2021: contributor

The set of changes look fine, but there a couple of issues with commits:

The mempool_packages.py fails after the first commit since you remove the COIN import (and then add it back in the second commit). I think you just need to squash the changes to mempool_packages.py in the second commit into the first commit.
Can you fixup the commit logs with standard punctuation/capitalization, backticks to indicate code, full filenames, etc.

josibake force-pushed on Nov 1, 2021

josibake commented at 12:26 pm on November 1, 2021: member

The set of changes look fine, but there a couple of issues with commits:

* The mempool_packages.py fails after the first commit since you remove the `COIN` import (and then add it back in the second commit). I think you just need to squash the changes to mempool_packages.py in the second commit into the first commit.

* Can you fixup the commit logs with standard punctuation/capitalization, backticks to indicate code, full filenames, etc.

thanks for taking a look at this again, @jnewbery ! looks like i added back the COIN import as part of the rebase but added it to the wrong commit. fixed now. also cleaned up the commit messages and added the newline per your suggestion.

git range-diff 285cb3d...7330beb

in test/functional/rpc_mempool_entry_fee_fields_deprecation.py:33 in 7330beb391 outdated

28+        self.wallet.rescan_utxos()
29+
30+        # we create the tx on the first node and wait until it syncs to node_deprecated
31+        # thus, any differences must be coming from getmempoolentry or getrawmempool
32+        txid = self.wallet.send_self_transfer(from_node=self.nodes[0])["txid"]
33+        self.sync_mempools(nodes=self.nodes)

jnewbery commented at 3:46 pm on November 1, 2021:

This sync can take several seconds since bitcoind will add some random delay on transaction relay to improve privacy. Since testing tx relay isn’t the point of this test, you could make the test run reliably faster by submitting the transaction directly to node 1 with the sendrawtransaction RPC.

josibake commented at 9:08 am on November 2, 2021:

thanks for the tip! i did notice the new test was taking longer than i expected. fixed!

josibake commented at 9:10 am on November 2, 2021:

change since your last ACK: git range-diff 7330beb...2f9515f

jnewbery commented at 10:39 am on November 2, 2021:

Comment above is now inaccurate: “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” :eyes:

jnewbery commented at 3:46 pm on November 1, 2021: contributor

Code review ACK 7330beb391

One suggestion inline to speed up the new function test. Thanks for the quick turnaround @josibake!

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 <jon@atack.com>

35d928c632

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.

2ee406ce3e

doc: add release note for fee field deprecation 07ade7db8f

rpc: move fees object to match help 2f9515f37a

josibake force-pushed on Nov 2, 2021

jnewbery commented at 10:45 am on November 2, 2021: contributor

reACK 2f9515f37addabde84c79926d7a24b2897a21dd1

One minor comment above: #22689 (review). No need to resolve that unless you need to push again for some other reason.

glozow commented at 11:29 am on December 7, 2021: member

utACK 2f9515f37addabde84c79926d7a24b2897a21dd1

glozow approved

maflcko merged this on Dec 7, 2021

maflcko closed this on Dec 7, 2021

in src/rpc/blockchain.cpp:466 in 2f9515f37a

461@@ -462,23 +462,23 @@ static RPCHelpMan getdifficulty()
462 static std::vector<RPCResult> MempoolEntryDescription() { return {
463     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."},
464     RPCResult{RPCResult::Type::NUM, "weight", "transaction weight as defined in BIP 141."},
465-    RPCResult{RPCResult::Type::STR_AMOUNT, "fee", "transaction fee in " + CURRENCY_UNIT + " (DEPRECATED)"},
466-    RPCResult{RPCResult::Type::STR_AMOUNT, "modifiedfee", "transaction fee with fee deltas used for mining priority (DEPRECATED)"},
467+    RPCResult{RPCResult::Type::STR_AMOUNT, "fee", "transaction fee, denominated in " + CURRENCY_UNIT + " (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"},
468+    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)"},

maflcko commented at 2:48 pm on December 7, 2021:

missing optional. see https://github.com/bitcoin/bitcoin/pull/23694

sidhujag referenced this in commit 10749fc233 on Dec 7, 2021

RandyMcMillan referenced this in commit 96a5e0d14e on Dec 23, 2021

maflcko referenced this in commit b6ab45ae5c on May 30, 2022

bitcoin locked this on Dec 7, 2022

josibake deleted the branch on Jan 26, 2024

rpc: deprecate top-level fee fields in getmempool RPCs #22689

questions for the reviewer

testing

Conflicts