rpc: Remove deprecated dummy alias for listtransactions::label

maflcko commented at 4:00 pm on December 3, 2024: member

The RPC arg is not a dummy, but a label, so offering an undocumented alias is inconsistent with all other label interfaces and confusing at best, if not entirely unused.

Fix it by removing the deprecated alias.

This pull is a breaking change, but it should be limited, because it only affects someone using the deprecated named arg on this RPC. I can’t imagine anyone doing this, because in all other places where label args are accepted, they are called label. If someone really didn’t use label here as named arg, it would be trivial and less confusing for them to fix it up.

rpc: Remove deprecated dummy alias for listtransactions::label fa8e0956c2

DrahtBot commented at 2:15 am on December 6, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31413.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux
Concept ACK	ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

rkrux commented at 5:46 am on December 10, 2024: none

The code change seems trivial but my review would be helped if you can share some more context because Idk history around this dummy arg/alias.

From #18607 I found

Unrelated to that, there have been a bunch of aliases in the server code that are not present in the source code of the documentation.

Is this dummy alias not used in server code anymore? Can you share the commit for it?

I also found couple dummy RPC args in 2 other RPCs below. I’m guessing they can’t be removed because they are not aliases but separate args instead?

0src/wallet/rpc/coins.cpp:170:                    {"dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Remains for b
1ackward compatibility. Must be excluded or set to \"*\"."},
2...
3src/wallet/rpc/spend.cpp:330:                    {"dummy", RPCArg::Type::STR, RPCArg::Default{"\"\""}, "Must be set to
4\"\" for backwards compatibility.",

maflcko commented at 7:48 am on December 10, 2024: member

Is this dummy alias not used in server code anymore? Can you share the commit for it?

It is used in server code in current master. This pull removes it, so the commit is in this pull.

I also found couple dummy RPC args in 2 other RPCs below. I’m guessing they can’t be removed because they are not aliases but separate args instead?

Those args do not represent labels, as you can’t send from a label, or calculate the balance of a label. They can be removed, but it would be a breaking change.

This pull is also a breaking change, but it should be limited, because it only affects someone using the deprecated named arg on this RPC. I can’t imagine anyone doing this, because in all other places where label args are accepted, they are called label. If someone really didn’t use label here as named arg, it would be trivial and less confusing for them to fix it up.

rkrux commented at 10:07 am on December 10, 2024: none

It is used in server code in current master. This pull removes it, so the commit is in this pull.

Ahh I see, I guess I got confused by the comment in the previous PR. Thanks for answering both the questions.

maflcko renamed this:
~~rpc: Remove deprecated dummy alias for listtransactions::label~~
rpc: Remove deprecated dummy alias for listtransactions::label
on Dec 10, 2024

DrahtBot renamed this:
~~rpc: Remove deprecated dummy alias for listtransactions::label~~
rpc: Remove deprecated dummy alias for listtransactions::label
on Dec 10, 2024

DrahtBot added the label RPC/REST/ZMQ on Dec 10, 2024

rkrux approved

rkrux commented at 10:10 am on December 10, 2024: none

This pull is also a breaking change, but it should be limited, because it only affects someone using the deprecated named arg on this RPC. I can’t imagine anyone doing this, because in all other places where label args are accepted, they are called label. If someone really didn’t use label here as named arg, it would be trivial and less confusing for them to fix it up.

I believe this is good context and would be helpful to mention it in the PR description so that the merge script picks it up - specially because it’s a breaking change although with a smaller impact.

tACK fa8e0956c23789fb819ad1b31377711df091ec1b

ryanofsky commented at 3:18 pm on December 10, 2024: contributor

Concept ACK. Reading this I was confused what original purpose of adding the dummy alias in fa168d754221a83cab0d2984a02c41cf6819e873 from #18607 was if we don’t want the alias now. But actually fa168d754221a83cab0d2984a02c41cf6819e873 was only documenting the alias not adding it, because before #20012 RPC method registration worked differently and aliases were registered and documented separately.

ryanofsky commented at 3:27 pm on December 10, 2024: contributor

Going back further, the first listtransactions argument was originally account, then #12953 dropped it and changed it to dummy, then #14411 added it back as label. So there should be little reason for anyone to be using the dummy alias today, or ever since it could always be omitted.

maflcko commented at 3:38 pm on December 10, 2024: member

Jup, thanks for writing out the historical context!

rpc: Remove deprecated dummy alias for listtransactions::label #31413

Code Coverage & Benchmarks

Reviews

Conflicts