rpc: remove optional from fStateStats fields #26727

1. 
   fanquake commented at 3:40 pm on December 19, 2022: member

   These are no-longer optional after #26515, so remove the documentation, and no-op fStateStats checks.
2. rpc: remove optional from fStateStats fields

   These are no-longer optional after #26515, so remove the documentation,
   and no-op fStateStats checks.

   1dc0e4bc6f
3. fanquake added the label RPC/REST/ZMQ on Dec 19, 2022
4. 
   DrahtBot commented at 3:40 pm on December 19, 2022: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	dergoegge

   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:

   • #26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)

   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.

5. 
   dergoegge commented at 4:01 pm on December 19, 2022: member

   Concept ACK
6. 
   mzumsande commented at 5:36 pm on December 20, 2022: contributor

   This should be fine (and was included in a previous version of #26515 ), but it should be weighed against the alternative of making some of these fields optional to replace default values during the setup of a new connection (#25923) when we don’t know the correct values yet. I don’t have a strong opinion (see my comment there #25923#pullrequestreview-1188773952), but we’d certainly would want to avoid removing the optional just to make it optional again in a follow-up PR:
7. bitcoin deleted a comment on Dec 20, 2022
8. 
   fanquake commented at 3:35 pm on January 11, 2023: member

   but we’d certainly would want to avoid removing the optional just to make it optional again in a follow-up PR:

   I agree on not wanting to make unecessary changes, although I’m not sure there is value keeping around what is currently dead code just incase we change something again later. It’s unclear to me what the status of #25923 is.

9. 
   maflcko commented at 3:43 pm on January 11, 2023: member

   lgtm. Given that this is dead code (nullopt is never returned), making it “truly” optional would be a breaking change. Not sure how much value there is in breaking this at this point. Maybe future fields can just be optional like pingtime is optional?
10. 
    jonatack commented at 6:23 pm on January 11, 2023: contributor

    It’s unclear to me what the status of #25923 is.

    I’m rebasing and updating #25923, should push today/tomorrow and provide any relevant feedback here.

11. fanquake requested review from dergoegge on Jan 17, 2023
12. dergoegge approved
13. 
    dergoegge commented at 10:20 am on January 18, 2023: member

    Code review ACK 1dc0e4bc6f3bd156b3fcd942b80820e60b83fa61
14. maflcko merged this on Jan 18, 2023
15. maflcko closed this on Jan 18, 2023

    ---

16. fanquake deleted the branch on Jan 18, 2023
17. sidhujag referenced this in commit a6dbeff60a on Jan 18, 2023
18. bitcoin locked this on Jan 18, 2024

Contributors
fanquake DrahtBot dergoegge mzumsande maflcko jonatack

Labels
RPC/REST/ZMQ