with its pre-existing v23 default value of false, as it is intermittently absent during peer disconnection – see #26457#pullrequestreview-1181641835. Credit to Martin Zumsande for finding this.
This allows dropping the patch in #25176 here, and, if backported to v24, provides API continuity as relaytxes was always present in getpeerinfo since it was introduced in 2015.
Also fix -netinfo relaytxes doc to address #26109 (review).
Co-authored-by: Martin Zumsande mzumsande@gmail.com
with its pre-existing v23 default value of false, as it would only
potentially be absent (null) during peer disconnection.
Credit to Martin Zumsande for finding this.
This allows to also drop the null check for that field in -netinfo.
Also fix -netinfo relaytxes doc.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process. A summary of reviews will appear here.
Reviewers, 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.
199@@ -200,9 +200,7 @@ static RPCHelpMan getpeerinfo()
200 ServiceFlags services{fStateStats ? statestats.their_services : ServiceFlags::NODE_NONE};
201 obj.pushKV("services", strprintf("%016x", services));
202 obj.pushKV("servicesnames", GetServicesNames(services));
203- if (fStateStats) {
204- obj.pushKV("relaytxes", statestats.m_relay_txs);
205- }
206+ obj.pushKV("relaytxes", fStateStats ? statestats.m_relay_txs : false);
I think agree with the statement above, fStateStats is only true if the peer id exists in m_node_states and m_peer_map, so it seems like the node shouldn’t exist in the return value at all.
I guess according to the comment at #26457#pullrequestreview-1181641835, there is the potential for a race condition, but seems awkward to replicate in the wild as any disconnection removes the nodes pointers and states entirely by the time the rpc is called.
What is the status of this?
Think I agree with the two commentors above. Returning (potentially) incorrect data, just so that we have a value for a field that may otherwise not be present (but only if you’re racing a peer disconnect when calling the RPC, where it’s unclear if we should even be providing data), doesn’t necessarily seem like an improvement. @mzumsande given that you’re listed as co-author, do you have any thoughts?
My opinion is that in general, a good rule is that for each field that is optional, we should be able to explain when we do no report it (e.g. during startup) and include that explanation in the RPC help when it’s not completely obvious. In principle, the same is true for fields that are not technically optional but have default values. With boolean values, actual optionality seems better because we can’t pick a value that is easily recognizable as a default value like “-1” for an integer value or an empty string.
As for the fields of CNodeStateStats I think that the race at shutdown should be fixed. I suggested a simple fix in #26515, if that is not the preferred approach, maybe we could alternatively refactor getpeerinfo to take a lock while gathering data about a peer, making it impossible for peers to be disconnected while the rpc collects the data for a given peer (I haven’t looked into this).
As for the fields of CNodeStateStats I think that the race at shutdown should be fixed. @mzumsande Thanks. I agree, and think either of the approaches, of not returning data when it can’t be/isn’t complete, or taking a lock and ensuring data is complete, are better approaches than filling fields with default values, in the case that data may be missing.