rpc: remove optional from fStateStats fields #26727
Conversation
These are no-longer optional after bitcoin#26515, so remove the documentation, and no-op fStateStats checks.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
There was a problem hiding this comment.
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 (review)), 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. |
|
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? |
6 participants
These are no-longer optional after #26515, so remove the documentation, and no-op
fStateStatschecks.