-
Notifications
You must be signed in to change notification settings - Fork 38.8k
rpc, doc: getpeerinfo updates #26109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rpc, doc: getpeerinfo updates #26109
Conversation
src/rpc/net.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A user reading false with bloom filters disabled may register the peer as one that txs aren't sent to, as the relay flag can't change after the handshake. So I think keeping this optional may be better. Also, in bitcoin-cli it might be better to display it as optional instead of a picking a constant which may later turn out to be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably, users expect the current released behavior. This patch maintains the API, whereas leaving it optional would annoyingly break user space without notice in v24. The API change was unclear but it appears unintended as it added neither optional mentions in the getpeerinfo help, which you fixed afterward, nor a release note. Absent a real need to change it (say, like removing a field), I have the impression that users prefer we don't break things willy-nilly and particularly in subtle ways like a field that is sometimes no longer present, at least without some explanation of what and why if we really want to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think whether the change was "unintended" was discussed already, so I am not going to reply to that.
If there is a missing release note, it can be added to the wiki.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To argue against my points, maybe getpeerinfo is mainly consumed by us developers and rarely by software clients or node operators -- I don't know -- and I agree with you that for us developers it is preferable not to return a relaytxes value while the connection is setting up. (Edit: have been informed that the getpeerinfo endpoint is used by at least one large bitcoin service as part of their CI pipeline sanity checks.)
Updating to take your feedback (appreciated, thank you) with the caveats I described.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a missing release note, it can be added to the wiki.
Note that it doesn't seem to have been added to the wiki (https://github.com/bitcoin-core/bitcoin-devwiki/wiki/24.0-Release-Notes-draft) if I'm reading the right doc and parsing it correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also heard from the financial software industry using bitcoin that these v24 changes will break their production systems without a patch. It was first reported to me orally by an engineer who observed the discussions in #25923, and today I received written confirmation of this by PMs and engineers there.
bd56f68 to
e2f5c29
Compare
|
Updated per @MarcoFalke's feedback (thanks!).
|
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
e2f5c29 to
56edac7
Compare
|
Updated per @mzumsande and @luke-jr review feedback (thanks). |
56edac7 to
e6603e5
Compare
fanquake
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have trouble keeping up with bitcoin-cli / netinfo changes. I think in general us changing code / reverting things to try and maintain compatibility with -netinfo is somewhat backwards. It should really be a best-effort component of bitcoin-cli, that just adapts to any code changes we make, not another interface / tool we need to worry about backwards compatibility for.
That's what is done here, as well as in #25176, for this release: adapting -netinfo to changes. Edit: no longer any -netinfo changes apart from a user-facing doc update to the current P2P behavior. |
This also keeps it consistent with the last release (v23)
with its pre-existing v23 default value of 0.
…24 backport) to the current p2p behavior. We only initialize the Peer::TxRelay m_relay_txs data structure if it isn't an outbound block-relay-only connection and fRelay=true (the peer wishes to receive tx announcements) or we're offering NODE_BLOOM to this peer.
e6603e5 to
4c70a59
Compare
4c70a59 to
a3789c7
Compare
brunoerg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK a3789c7
In 9cd6682, I just compared the results (master x this branch) and could realize now it's consistent with its help.
In 9cd6682, I modified the code to test it, like:
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 704a23240..9acee54c6 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -184,7 +184,7 @@ static RPCHelpMan getpeerinfo()
for (const CNodeStats& stats : vstats) {
UniValue obj(UniValue::VOBJ);
CNodeStateStats statestats;
- bool fStateStats = peerman.GetNodeStateStats(stats.nodeid, statestats);
+ bool fStateStats = false;
obj.pushKV("id", stats.nodeid);
obj.pushKV("addr", stats.m_addr_name);
if (stats.addrBind.IsValid()) {So, I ran getpeerinfo, and I could realize minfeefilter has been returned even with fStateStats false, e.g.:
{
"id": 8,
"addr": "34.121.41.161:8333",
"addrbind": "192.168.0.73:49948",
"network": "ipv4",
"services": "0000000000000000",
"servicesnames": [
],
"lastsend": 1664375138,
"lastrecv": 1664375138,
"last_transaction": 0,
"last_block": 0,
"bytessent": 127,
"bytesrecv": 198,
"conntime": 1664375138,
"timeoffset": 0,
"version": 0,
"subver": "",
"inbound": false,
"bip152_hb_to": false,
"bip152_hb_from": false,
"permissions": [
],
"minfeefilter": 0,
"bytessent_per_msg": {
"version": 127
},
"bytesrecv_per_msg": {
"sendaddrv2": 24,
"verack": 24,
"version": 126,
"wtxidrelay": 24
},
"connection_type": "outbound-full-relay"
}
While reviewing it, it seemed a little bit weird to return "minfeefilter": 0 because - as a user - I wouldn't be able to know if it returned 0 because it is the real value from m_fee_filter_received or because fStateStats is false. However, reading some discussions (#25923 (review) and #25923 (comment)) the reason seems ok for me, but I'd prefer to return -1 as proposed by #25923 (comment) because it would be easier to distinguish that there aren't any stats.
df660dd and a3789c7 are both docs improvement, they seem good for me.
|
Thanks for the review @brunoerg!
Yes, I don't disagree but left the default as it has been since the field was added in 2018 as the commit is intended for backport, unless people feel it's worth including. @MarcoFalke mind re-acking your review approval? |
fanquake
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't understand 9cd6682, and it specifically being marked "for v24 backport". We don't make code changes in master primarily to backport them, we make changes in master (ideally) to fix bugs, and then, backport (select) bug fixes as required.
9cd6682 does not fix a bug. It moves around code, while making it more complex (additional conditionals, ungroups related items), to make JSON output align with help documentation, which as far as I'm aware, is neither a goal we have across RPCs generally, nor enforced by anything. JSON does not have field ordering, so changing our code to maintain field ordering, doesn't make sense.
If we really wanted them to align, an alternative would be to change the help docs to reflact the new ordering, rather than make the code worse.
This was discussed at length in this thread.
|
The field and help went out of sync between v23 and now, so it makes sense both for master and for v24. I've been asked in the past to mention "for backport" in the commit names, but of course can drop that mention if that is now no longer our preferred practice.
You have tagged non-bugfixes for backport to v24. For instance: 68209a7. The other commit in that PR wasn't a bug, either.
I've addressed this with #26109 (comment): "Our RPC interface has the dual constraint of providing an API consumed by machines and the CLI interface used by humans -- cf the RPC helps that provide at least one example of each. It is therefore preferable to maintain the order, particularly when there is no need to change it."
Of course we do! If you look at the help and result of our RPCs --- including those with many fields like getpeerinfo, getnetworkinfo, getdeploymentinfo, etc. --- the help and the result are in the same order. This isn't a coincidence.
There is no need or reason to change the ordering in the first place. This represents the least change for the user, which is more worthwhile than saving a line of code and not worth repeatedly nitting over. |
vasild
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK a3789c7
I agree changing order of JSON fields is not a change or breaking of the API. Anything that breaks due to that is broken already. However, I sometimes consume the JSON output with my eyes. It would be nice if the order does not change (it follows that I am semi-broken). Also, it would be nice if the order in the cpp file of the help and the place where fields are "printed" is the same (may help devs a little bit when navigating the source code).
Non-blocker: I think the versions mentions do not belong to the commit messages, they should be timeless. Imagine if a commit like "... previous version v23 ..." is merged sometime in the future when the previous version is not 23 anymore. Also, "for backport in v24" implies that the decision is made to backport it already. Commits should make sense on their own, regardless of when releases are done. The decision whether to backport is a separate one.
Thanks! Yes, I've been asked in the past by a maintainer to add backport mentions to commit messages for previous releases, to clarify which commits in a pull are intended for backport. IDK if current practices have changed. |
| " recv Time since last message received from the peer, in seconds\n" | ||
| " txn Time since last novel transaction received from the peer and accepted into our mempool, in minutes\n" | ||
| " \"*\" - the peer requested we not relay transactions to it (relaytxes is false)\n" | ||
| " \"*\" - whether we relay transactions to this peer (relaytxes is false)\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right. Even if relaytxes is true, this might be false (for example this is an outbound block-relay-only connection?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated: It seems the cli will default to true if the field is not available, which might also be confusing if an outbound block-relay-only connection is spun up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Perhaps: we don't relay transactions to this peer (relaytxes is false or not available)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposed a fixup in #26328.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I think I read this as version.relaytxes is false, but this means getpeerinfo.relaytxes is false? The two are different, so for one interpretation this comment is right and for the other it is wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will clarify next time I touch the file that it's the getpeerinfo "relaytxes" field (what is version.relaytxes?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The various namings for tx relay can be a little confusing.
0f5fc4f doc: fix up -netinfo relaytxes help (Jon Atack) Pull request description: Addresses bitcoin/bitcoin#26109 (comment) by Marco Falke (thanks!) ACKs for top commit: mzumsande: Code Review ACK 0f5fc4f Tree-SHA512: d7345d1a94b15c4ec1a2bb0be5c04c472411d90cefb4c16ed524933d2bfc36816bb7519c2e109b2e41ff451b039dd2ddaa6d5db917ad54745332f2a1d8b85570
0f5fc4f doc: fix up -netinfo relaytxes help (Jon Atack) Pull request description: Addresses bitcoin#26109 (comment) by Marco Falke (thanks!) ACKs for top commit: mzumsande: Code Review ACK 0f5fc4f Tree-SHA512: d7345d1a94b15c4ec1a2bb0be5c04c472411d90cefb4c16ed524933d2bfc36816bb7519c2e109b2e41ff451b039dd2ddaa6d5db917ad54745332f2a1d8b85570
Various updates and fixups, mostly targeting v24. Please refer to the commit messages for details.