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

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

2. rpc: Remove deprecated dummy alias for listtransactions::label fa8e0956c2
3. 
   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, ryanofsky, achow101

   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.

4. 
   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.",
   

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

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

7. maflcko renamed this:
   rpc: Remove deprecated dummy alias for listtransactions::label
   rpc: Remove deprecated dummy alias for listtransactions::label
   on Dec 10, 2024
8. DrahtBot renamed this:
   rpc: Remove deprecated dummy alias for listtransactions::label
   rpc: Remove deprecated dummy alias for listtransactions::label
   on Dec 10, 2024
9. DrahtBot added the label RPC/REST/ZMQ on Dec 10, 2024
10. rkrux approved
11. 
    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

12. 
    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.
13. 
    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.
14. 
    maflcko commented at 3:38 pm on December 10, 2024: member

    Jup, thanks for writing out the historical context!
15. ryanofsky approved
16. 
    ryanofsky commented at 7:18 pm on January 8, 2025: contributor

    Code review ACK fa8e0956c23789fb819ad1b31377711df091ec1b
17. 
    achow101 commented at 7:36 pm on February 14, 2025: member

    ACK fa8e0956c23789fb819ad1b31377711df091ec1b
18. achow101 merged this on Feb 14, 2025
19. achow101 closed this on Feb 14, 2025

    ---

20. maflcko deleted the branch on Feb 17, 2025

Contributors
maflcko DrahtBot rkrux ryanofsky achow101

Labels
RPC/REST/ZMQ