-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rpc: deprecate top-level fee fields in getmempool RPCs #22689
rpc: deprecate top-level fee fields in getmempool RPCs #22689
Conversation
084d443
to
251d636
Compare
251d636
to
8cae682
Compare
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
8cae682
to
c4646f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept and approach ACK
# note: we are only testing getmempoolentry even though | ||
# getrawmempool(verbose=True) also returns mempool entries | ||
# this is because getmempool entry and getrawmempool(verbose=True) | ||
# both call the same underlying entryToJSON function so testing | ||
# getmempoolentry is sufficient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also add the assertions to a helper function, and call that helper function with results you get from each RPC
Concept ACK |
1 similar comment
Concept ACK |
f470aaf
to
6940fe0
Compare
thanks for the review, @glozow! I added your suggestions and left a comment on why I think it makes sense to keep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK
You'll probably need to add release notes, something like 90ae3d8
src/rpc/blockchain.cpp
Outdated
@@ -491,19 +492,20 @@ 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); | |||
|
|||
if (include_deprecated_fee_fields) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you don't do:
if (IsDeprecatedRPCEnabled("fees")) {
here? And not have to change the function header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was following ScriptPubKeyToUniv
as an example:
bitcoin/src/rpc/blockchain.cpp
Lines 1936 to 1939 in 42b00a3
void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex) | |
{ | |
ScriptPubKeyToUniv(scriptPubKey, out, fIncludeHex, IsDeprecatedRPCEnabled("addresses")); | |
} |
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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree as well, but no big deal so didn't mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like its the more preferred approach, happy to switch
8fb8dbf
to
4df6ffd
Compare
4df6ffd
to
c3a2842
Compare
great callout, @fanquake . ive update all three commit messages with relevant tags (rpc, test, doc) and wrote more descriptive commit messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
doc/release-notes.md
Outdated
@@ -63,6 +63,8 @@ P2P and network changes | |||
|
|||
Updated RPCs | |||
------------ | |||
- `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. | |||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestions
- add the PR number
- add
getmempoolancestors
andgetmempooldescendants
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
-- `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.
-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
+- RPCs `getmempoolentry`, `getrawmempool verbose=true`, `getmempoolancestors
+ verbose=true` and `getmempooldescendants verbose=true` no longer return the
+ top-level fee fields `fee`, `modifiedfee`, `ancestorfees` and
+ `descendantfees`. These fields were deprecated in v0.17 and moved to the
+ `fees` JSON object in the response. The `-deprecated=fees` configuration
+ option must be passed for these fields to be included in the top-level
+ response. This option will only be available until v24.0, after which the
+ fields will be fully removed. Use the `fees` JSON object for these RPCS
+ instead. (#22689)
c3a2842
to
2ba3ead
Compare
good catch! i verified by grepping for |
2ba3ead
to
e5b22ec
Compare
force pushed e5b22ec to fix a failure in the linter |
looking at ci failures, not immediately clear to me what's happening - will take a look tomorrow |
61af49c
to
285cb3d
Compare
one-line change to fix the newly added test ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
285cb3d
to
7330beb
Compare
thanks for taking a look at this again, @jnewbery ! looks like i added back the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Test for old fields when `-deprecatedrpc=fees` flag is passed and verify values in the deprecated fields match values in the fees sub-object.
7330beb
to
2f9515f
Compare
reACK 2f9515f One minor comment above: #22689 (comment). No need to resolve that unless you need to push again for some other reason. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 2f9515f
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)"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing optional. see #23694
…etmempool RPCs 0ab0f52 rpc: move fees object to match help (josibake) 23889a0 doc: add release note for fee field deprecation (josibake) 12993d4 test: add functional test for deprecatedrpc=fees (josibake) 79f6fca rpc: deprecate fee fields from mempool entries (josibake) Pull request description: 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: ```bash ./src/bitcoind -daemon -deprecatedrpc=fees ``` you should see entries with the deprecated fields like so: ```json { "<txid>": { "fees": { "base": 0.00000671, "modified": 0.00000671, "ancestor": 0.00000671, "descendant": 0.00000671 }, "fee": 0.00000671, "modifiedfee": 0.00000671, "descendantfees": 671, "ancestorfees": 671, "vsize": 144, "weight": 573, ... }, ``` 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 ACKs for top commit: jnewbery: reACK 0ab0f52 glozow: utACK 0ab0f52 Tree-SHA512: b175f4d39d26d96dc5bae26717d3ccfa5842d98ab402065880bfdcf4921b14ca692a8919fe4e9969acbb5c4d6e6d07dd6462a7e0a0a7342556279b381e1a004e
…l entries 885694d doc: add release note about removal of `deprecatedrpc=fees` flag (Sebastian Falbesoner) 387ae8b rpc: remove deprecated fee fields from mempool entries (Sebastian Falbesoner) Pull request description: Deprecating the top-level fee fields (`fee`, `modifiedfee`, `ancestorfees` and `descendantfees`) from the mempool entries and introducing `-deprecatedrpc=fees` was done in PR #22689 (released in v23.0). For the next release v24.0, this configuration option can be removed. ACKs for top commit: fanquake: ACK 885694d Tree-SHA512: fec6b5be5c3f0cd55738a888b390ef9271e70b2dba913a14ce82427dac002e999f93df298bb3b494f3d1b850a23d2b5b3e010e901543b0d18db9be133579e1ec
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 thefees
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 senseTODO: fully remove in vXX
comment toentryToJSON
testing
to get started on testing, compile, run the tests, and start your node with the deprecated rpcs flag:
you should see entries with the deprecated fields like so:
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