rpc: Remove deprecated -rpcserialversion #28890

1. 
   maflcko commented at 9:35 am on November 16, 2023: member

   The flag is problematic for many reasons:

   • It is deprecated
   • It is a global flag, requiring a restart to change, as opposed to a flag that can be set on each RPC invocation
   • It may be hidden in config files by accident, hard to debug, causing LND crashes and bugs, see #28730 (comment)
   • It makes performance improvements harder to implement: #17529 (comment)

   Fix all issues by removing it.

   If there is a use-case, likely a per-RPC flag can be added, if needed.

2. 
   DrahtBot commented at 9:35 am on November 16, 2023: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	ajtowns, TheCharlatan
   Concept ACK	fanquake, kristapsk

   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:

   • #29007 (test: create deterministic addrman in the functional tests by stratospher)
   • #28528 (test: Use test framework utils in functional tests by osagie98)
   • #27433 (getblocktemplate improvements for segwit and sigops 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.

3. DrahtBot added the label RPC/REST/ZMQ on Nov 16, 2023
4. fanquake requested review from ajtowns on Nov 16, 2023
5. fanquake requested review from theuni on Nov 16, 2023
6. 
   fanquake commented at 10:46 am on November 16, 2023: member

   Concept ACK.
7. 
   ajtowns commented at 11:52 am on November 16, 2023: contributor

   It is deprecated

   It’s only been deprecated in master for a couple of months; better to wait for it to actually be deprecated in the current release for a little while to give people a chance to complain if they’re still relying on this functionality?

   Otherwise, changes here look like what I’d expect.

   If there is a use-case, likely a per-RPC flag can be added, if needed.

   Could give bitcoin-util the capability to strip witness data from a block/tx and leave it up to users who need it, rather than making the RPC API more complex for what’s presumably a very rare use case.

8. 
   maflcko commented at 12:01 pm on November 16, 2023: member

   better to wait for it to actually be deprecated in the current release for a little while to give people a chance to complain if they’re still relying on this functionality?

   Happy to wait, but there shouldn’t be any merge conflicts, so it should be trivial to just type git revert cleanly in the rare case that the deprecation will be undone. If the deprecation isn’t undone, I don’t see a need to delay for another release, because anyone affected can stay at 26.0, for as long as they need to change their code.

9. maflcko added this to the milestone 27.0 on Nov 30, 2023
10. Remove deprecated -rpcserialversion fa46cc22bc
11. maflcko force-pushed on Dec 11, 2023
12. 
    maflcko commented at 4:05 pm on December 27, 2023: member

    The deprecation was covered in https://bitcoinops.org/en/newsletters/2023/09/20/ and 26.0 was released a few weeks ago. Unless anyone heard someone complain, this seems good to move forward now?
13. 
    kristapsk commented at 4:46 pm on December 27, 2023: contributor

    Concept ACK
14. 
    ajtowns commented at 10:41 am on December 29, 2023: contributor

    The deprecation was covered in https://bitcoinops.org/en/newsletters/2023/09/20/ and 26.0 was released a few weeks ago. Unless anyone heard someone complain, this seems good to move forward now?

    Given we’re doing a short cycle, probably should either merge this soon for 27.0 or defer it to 28.0 and merge it shortly after branch off. No opinion either way on that personally.

    crACK fa46cc22bc696e6845915ae91d6b68e36bf4c242

15. DrahtBot removed review request from ajtowns on Dec 29, 2023
16. DrahtBot requested review from fanquake on Dec 29, 2023
17. TheCharlatan approved
18. 
    TheCharlatan commented at 10:29 am on January 5, 2024: contributor

    lgtm ACK fa46cc22bc696e6845915ae91d6b68e36bf4c242

    If there is a use-case, likely a per-RPC flag can be added, if needed.

19. fanquake merged this on Jan 5, 2024
20. fanquake closed this on Jan 5, 2024

    ---

21. maflcko deleted the branch on Jan 5, 2024
22. 
    theuni commented at 11:38 am on January 5, 2024: member

    Post-merge utACK fa46cc22bc696e6845915ae91d6b68e36bf4c242. Nice cleanup :)

Contributors
maflcko DrahtBot fanquake ajtowns kristapsk TheCharlatan theuni

Review Requested
theuni fanquake

Labels
RPC/REST/ZMQ

Milestone
27.0