rpc: Remove deprecated RPC arg names #31412

1. 
   maflcko commented at 2:26 pm on December 3, 2024: member

   For compatibility, a single RPC argument may have multiple names. This is largely unused, because legacy code relying on it isn’t generally using names arguments anyway. Also, it is confusing to see in existing code (https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2477250414) and makes writing new RPC code harder.

   Fix all issues by removing them.

   In the future, if such a feature is needed, it would be clearer to implement via -rpcdeprecated=${rpc_name}_${field_old_name} to toggle between the field names.

2. 
   DrahtBot commented at 2:26 pm on December 3, 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/31412.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

3. DrahtBot renamed this:
   rpc: Remove deprecated RPC arg names
   rpc: Remove deprecated RPC arg names
   on Dec 3, 2024
4. DrahtBot added the label RPC/REST/ZMQ on Dec 3, 2024
5. rpc: Remove deprecated RPC arg names fa27af02b7
6. rpc: Remove dead code to handle deprecated arg names fa659fd880
7. maflcko force-pushed on Dec 3, 2024
8. DrahtBot added the label CI failed on Dec 3, 2024
9. 
   ryanofsky commented at 3:00 pm on December 3, 2024: contributor

   First commit dropping verbosity|verbose seems ok, but I think the ability for different arguments names to exist in the same position is useful for #24963. #24963 is an old PR but Luke just updated it 2 weeks ago and I acked it in #24963#pullrequestreview-2443280039, describing the benefits I think it offers.

   Do you think letting multiple arguments occupy the same position makes implementing #29912 harder? I would think an automatically generated schema could just ignore deprecated aliases and only show the current non-deprecated way of calling the API.

   I’m also not sure about the -rpcdeprecated=${rpc_name}_${field_old_name} idea as a replacement for aliases. It seems like the implementation of that idea could only be more complicated than the implementation of aliases, and while it might have benefit of giving clients a stronger incentive to modernize, it also disincentives us from cleaning up inconsistent and misleading parameter names because instead of just adding the new names, we might need to change more code and write and release notes and go through a deprecation cycle.

10. 
    edilmedeiros commented at 3:06 pm on December 3, 2024: contributor

    Concept ACK. This incrementally reduces complexity and is an easy fix for those that may have something broken because of it.

    What I don’t like is to have RPC methods with different parameter names doing the same thing. For instance:

    0getblock "blockhash" ( verbosity )
    1getblockheader "blockhash" ( verbose )
    

    Maybe it would be good to clean up the RPC API (in a more comprehensive follow-up) before fixing #29912 (which seems to be done in the foreseeable future)?

    Edit: still in favor of the change, but taking more time to digest #24963#pullrequestreview-2443280039 as I was not aware of it.

11. 
    maflcko commented at 3:50 pm on December 3, 2024: member

    Do you think letting multiple arguments occupy the same position makes implementing #29912 harder? I would think an automatically generated schema could just ignore deprecated aliases and only show the current non-deprecated way of calling the API.

    I agree, but it is still annoying to have the for-loop bloat everywhere internally in this repo for no real use-case. If this is needed for #24963, then I guess there is a use-case and I can close it for now.

    I’m also not sure about the -rpcdeprecated=${rpc_name}_${field_old_name} idea as a replacement for aliases. It seems like the implementation of that idea could only be more complicated than the implementation of aliases

    -rpcdeprecated is limited both spatially and temporary: To only the RPC method that needs it, not the whole RPC framework, and only to a single release. So it seems preferable to me.

12. maflcko closed this on Dec 3, 2024

    ---

13. maflcko deleted the branch on Dec 3, 2024
14. 
    ryanofsky commented at 5:12 pm on December 3, 2024: contributor

    re: #31412 (comment)

    -rpcdeprecated is limited both spatially and temporary: To only the RPC method that needs it, not the whole RPC framework, and only to a single release. So it seems preferable to me.

    Got it, so it sounds like a big motivation is to simplify rpc framework code. Agree that’s worth doing, and I think that should be possible even with #24963, or we could revisit this approach if #24963 doesn’t go anywhere.

    re: #31412 (comment)

    Edit: still in favor of the change, but taking more time to digest #24963#pullrequestreview-2443280039 as I was not aware of it.

    Thanks! Feel free to ask any questions there. That post was kind of a brain dump and I would like that PR to get more attention.

Contributors
maflcko DrahtBot ryanofsky edilmedeiros

Labels
RPC/REST/ZMQ CI failed