rpc, doc: getpeerinfo updates

jonatack commented at 1:58 pm on September 16, 2022: contributor

Various updates and fixups, mostly targeting v24. Please refer to the commit messages for details.

in src/rpc/net.cpp:203 in bd56f6896d outdated

199@@ -200,6 +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+        obj.pushKV("relaytxes", fStateStats ? statestats.m_relay_txs : false);

maflcko commented at 2:33 pm on September 16, 2022:

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.

jonatack commented at 2:46 pm on September 16, 2022:

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.

maflcko commented at 2:48 pm on September 16, 2022:

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.

jonatack commented at 3:47 pm on September 16, 2022:

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.

jonatack commented at 6:04 pm on November 10, 2022:

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.

jonatack commented at 7:07 pm on November 10, 2022:

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.

jonatack force-pushed on Sep 16, 2022

jonatack commented at 5:13 pm on September 16, 2022: contributor

Updated per @MarcoFalke’s feedback (thanks!).

 0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
 1index 04d72ade3d..a8293afdc7 100644
 2--- a/src/bitcoin-cli.cpp
 3+++ b/src/bitcoin-cli.cpp
 4@@ -467,8 +467,8 @@ public:
 5         if (!batch[ID_NETWORKINFO]["error"].isNull()) return batch[ID_NETWORKINFO];
 6 
 7         const UniValue& networkinfo{batch[ID_NETWORKINFO]["result"]};
 8-        if (networkinfo["version"].getInt<int>() < 220000) {
 9-            throw std::runtime_error("This version of -netinfo requires bitcoind server to be running v22.0 and up");
10+        if (networkinfo["version"].getInt<int>() < 239900) {
11+            throw std::runtime_error("This version of -netinfo requires the bitcoind server to be running v24.0 and up");
12         }
13         const int64_t time_now{TicksSinceEpoch<std::chrono::seconds>(CliClock::now())};
14 
15@@ -478,7 +478,7 @@ public:
16             const int8_t network_id{NetworkStringToId(network)};
17             if (network_id == UNKNOWN_NETWORK) continue;
18             const bool is_outbound{!peer["inbound"].get_bool()};
19-            const bool is_tx_relay{peer["relaytxes"].get_bool()};
20+            const bool is_tx_relay{peer["relaytxes"].isNull() ? true : peer["relaytxes"].get_bool()};
21             const std::string conn_type{peer["connection_type"].get_str()};
22             ++m_counts.at(is_outbound).at(network_id);      // in/out by network
23             ++m_counts.at(is_outbound).at(NETWORKS.size()); // in/out overall
24@@ -642,7 +642,7 @@ public:
25         "  send     Time since last message sent to the peer, in seconds\n"
26         "  recv     Time since last message received from the peer, in seconds\n"
27         "  txn      Time since last novel transaction received from the peer and accepted into our mempool, in minutes\n"
28-        "           \"*\" - whether we currently relay transactions to this peer (relaytxes is false)\n"
29+        "           \"*\" - whether we relay transactions to this peer (relaytxes is false)\n"
30         "  blk      Time since last novel block passing initial validity checks received from the peer, in minutes\n"
31         "  hb       High-bandwidth BIP152 compact block relay\n"
32         "           \".\" (to)   - we selected the peer as a high-bandwidth peer\n"
33diff --git a/src/net_processing.cpp b/src/net_processing.cpp
34index 781debd132..28148e1c65 100644
35--- a/src/net_processing.cpp
36+++ b/src/net_processing.cpp
37@@ -271,11 +271,11 @@ struct Peer {
38 
39     struct TxRelay {
40         mutable RecursiveMutex m_bloom_filter_mutex;
41-        /** Whether we currently relay transactions to this peer.
42+        /** Whether we relay transactions to this peer.
43          *
44          * On receiving a `version` message, this is initially set to false
45-         * either if the peer is outbound block-relay-only, or otherwise based
46-         * on the fRelay flag.
47+         * if the peer is outbound block-relay-only and otherwise based on
48+         * the fRelay flag.
49          *
50          * If initially set to false, it can only be flipped to true if we have
51          * offered the peer NODE_BLOOM services and it sends us a `filterload`
52diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
53index 41c4a2c539..e953f8ba25 100644
54--- a/src/rpc/net.cpp
55+++ b/src/rpc/net.cpp
56@@ -114,7 +114,7 @@ static RPCHelpMan getpeerinfo()
57                     {
58                         {RPCResult::Type::STR, "SERVICE_NAME", "the service name if it is recognised"}
59                     }},
60-                    {RPCResult::Type::BOOL, "relaytxes", "Whether we currently relay transactions to this peer"},
61+                    {RPCResult::Type::BOOL, "relaytxes", /*optional=*/true, "Whether we relay transactions to this peer"},
62                     {RPCResult::Type::NUM_TIME, "lastsend", "The " + UNIX_EPOCH_TIME + " of the last send"},
63                     {RPCResult::Type::NUM_TIME, "lastrecv", "The " + UNIX_EPOCH_TIME + " of the last receive"},
64                     {RPCResult::Type::NUM_TIME, "last_transaction", "The " + UNIX_EPOCH_TIME + " of the last valid transaction received from this peer"},
65@@ -200,7 +200,9 @@ static RPCHelpMan getpeerinfo()
66         ServiceFlags services{fStateStats ? statestats.their_services : ServiceFlags::NODE_NONE};
67         obj.pushKV("services", strprintf("%016x", services));
68         obj.pushKV("servicesnames", GetServicesNames(services));
69-        obj.pushKV("relaytxes", fStateStats ? statestats.m_relay_txs : false);
70+        if (fStateStats) {
71+            obj.pushKV("relaytxes", statestats.m_relay_txs);
72+        }
73         obj.pushKV("lastsend", count_seconds(stats.m_last_send));
74         obj.pushKV("lastrecv", count_seconds(stats.m_last_recv));
75         obj.pushKV("last_transaction", count_seconds(stats.m_last_tx_time));

in src/net_processing.cpp:278 in 9a309a6bb4 outdated

278-         * `version` message. If initially set to false, it can only be flipped
279-         * to true if we have offered the peer NODE_BLOOM services and it sends
280-         * us a `filterload` or `filterclear` message. See BIP37. */
281+         * On receiving a `version` message, this is initially set to false
282+         * if the peer is outbound block-relay-only and otherwise based on
283+         * the fRelay flag.

mzumsande commented at 5:17 pm on September 16, 2022:

I think that if this is an outbound block-relay-only peer, we don’t create a TxRelay object for this peer in the first place, so TxRelay::m_relay_txs is neither true nor false but doesn’t exist.

jonatack commented at 8:05 am on September 20, 2022:

Thanks, updated. I agree this was previously out of date and the proposed update was imprecise. It is already documented in the PeerManagerImpl::ProcessMessage()::VERSION code so it seems best to remove it here instead of duplicating.

in src/net_processing.cpp:282 in 9a309a6bb4 outdated

282+         * if the peer is outbound block-relay-only and otherwise based on
283+         * the fRelay flag.
284+         *
285+         * If initially set to false, it can only be flipped to true if we have
286+         * offered the peer NODE_BLOOM services and it sends us a `filterload`
287+         * or `filterclear` message. See BIP37. */

mzumsande commented at 5:23 pm on September 16, 2022:

This sentence could need an update after #22778: If we haven’t offered NODE_BLOOM and get fRelay=false, we no longer set m_relay_txs to false, but now don’t initialise a TxRelay object anymore. So the case where m_relay_txs is initialised to false and is impossible to flip cannot happen anymore.

jonatack commented at 8:06 am on September 20, 2022:

Thanks, removed per https://github.com/bitcoin/bitcoin/pull/26109/files#r975018566.

DrahtBot commented at 3:59 am on September 17, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26247 (refactor: Make m_mempool optional in PeerManager by MarcoFalke)
#25923 (p2p: always provide CNodeStateStats and getpeerinfo/netinfo/gui updates by jonatack)
#24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)

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.

in src/bitcoin-cli.cpp:471 in e2f5c2993a outdated

466@@ -467,8 +467,8 @@ class NetinfoRequestHandler : public BaseRequestHandler
467         if (!batch[ID_NETWORKINFO]["error"].isNull()) return batch[ID_NETWORKINFO];
468 
469         const UniValue& networkinfo{batch[ID_NETWORKINFO]["result"]};
470-        if (networkinfo["version"].getInt<int>() < 209900) {
471-            throw std::runtime_error("-netinfo requires bitcoind server to be running v0.21.0 and up");
472+        if (networkinfo["version"].getInt<int>() < 239900) {
473+            throw std::runtime_error("This version of -netinfo requires the bitcoind server to be running v24.0 and up");

luke-jr commented at 1:57 am on September 20, 2022:

0            throw std::runtime_error("This version of -netinfo requires the bitcoind server to be running v24.0 or newer");

jonatack commented at 8:01 am on September 20, 2022:

Done

jonatack force-pushed on Sep 20, 2022

jonatack commented at 8:21 am on September 20, 2022: contributor

Updated per @mzumsande and @luke-jr review feedback (thanks).

jonatack force-pushed on Sep 20, 2022

in src/rpc/net.cpp:149 in e6603e50f4 outdated

145@@ -146,7 +146,7 @@ static RPCHelpMan getpeerinfo()
146                     {
147                         {RPCResult::Type::STR, "permission_type", Join(NET_PERMISSIONS_DOC, ",\n") + ".\n"},
148                     }},
149-                    {RPCResult::Type::NUM, "minfeefilter", /*optional=*/true, "The minimum fee rate for transactions this peer accepts"},
150+                    {RPCResult::Type::NUM, "minfeefilter", "The minimum fee rate for transactions this peer accepts"},

maflcko commented at 8:23 am on September 20, 2022:

Is is worth it to change the interface, given that there are other optional fields?

jonatack commented at 8:27 am on September 20, 2022:

It maintains the existing interface in releases and follows your comment, “there are some fields that can obviously be made non-optional (such as the fee filter), because they can be absent on the wire even long after the connection has been spun up (thus require a default value anyway)”.

jonatack commented at 8:28 am on September 20, 2022:

(Anything that can have an always-present default is simpler for us and for API clients to deal with, code-wise.)

maflcko approved

in src/rpc/net.cpp:203 in e6603e50f4 outdated

199@@ -200,6 +200,9 @@ 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) {

fanquake commented at 8:49 am on September 22, 2022:

In 24289c6839790eda54baebf2344a8cbd0b1278d0:

Can you clarify how this commit (standalone) is a bug fix that needs to be backported to 24.x? (what will break if this isn’t done).

After this commit, there is no change in the interface. The same fields are returned, and still guarded by fStateStats. This just reorders them to match the help output, which I’m not sure is something we try to maintain?

jonatack commented at 2:05 pm on September 22, 2022:

Can you clarify how this commit (standalone) is a bug fix that needs to be backported to 24.x? (what will break if this isn’t done).

Other already-backported v24 commits aren’t bug fixes; some are doc updates and some are even arguably features.

The getpeerinfo RPC has many fields and they were in the same order in the help and the output in the previous releases. I don’t see a compelling reason to break that for these two fields for v24 and needlessly make the RPC more confusing.

mzumsande commented at 2:39 pm on September 22, 2022:

JSON objects are by definition collections of name/value pairs that are unordered. Given that, why not just shuffle the order in the help instead, so that we don’t need extra conditionals in the source code and can group things together that belong together?

jonatack commented at 2:57 pm on September 22, 2022:

On my Debian and macOS machines, the CLI and its help returns the getpeerinfo fields in the same order, which the codebase handles in the same order as well. This is handy; I don’t see a very compelling reason to change the API needlessly to be less helpful for those who read the help and run CLI commands.

jonatack commented at 3:07 pm on September 22, 2022:

can group things together that belong together?

It would be potentially nice for that fStateStats flag grouping to go away; it has been a source of bugs in the past.

fanquake commented at 3:14 pm on September 22, 2022:

to be less helpful for those who read the help and run CLI commands.

The point is that machines / parsers consuming the JSON do not, and should not care about ordering. Which means nothing is “broken” if the fields happen to change order, and modifying the code to preserve an order that isn’t guaranteed by anything is pointless. We also aren’t optimising our RPC output for humans reading JSON in a terminal.

to change the API needlessly

The API isn’t changing though. The ordering is not guaranteed, and is not part of any API. The fields returned remain exactly the same.

I agree with Martins comment, if you care about the ordering being the same, it would make more sense to change the docs.

jonatack commented at 10:34 am on September 26, 2022:

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.

Diffing src/rpc/net.cpp between the previous releases and master shows that changing the help order instead of the field order would mean needless churn in the interface. Needless, because there is no compelling reason. It makes more sense to keep the help the same in the absence of a real need to change it. A change from release to release ought to be justified, not stability. This commit chooses stability.

fanquake commented at 10:54 am on September 26, 2022:

shows that changing the help order instead of the field order would mean needless churn in the interface.

The documentation is not an interface, and changing the order of fields listed within it does not change the RPC interface/API. Ideally, the docs would be auto-generated from the code, and then more than likely, the fields would just be ordered alphanumerically.

Needless, because there is no compelling reason. It makes more sense to keep the help the same in the absence of a real need to change it. A change from release to release ought to be justified, not stability. This commit chooses stability.

I disagree that the solution to maintaining something (field ordering) that isn’t guaranteed, or part of any API, is to add more complexity too, or change the code. Especially when you could change the documentation and acheive the same thing.

jonatack commented at 11:09 am on September 26, 2022:

The diff to maintain stability is essentially the same without needlessly changing field position for the user. Code serves users.

jonatack commented at 11:10 am on September 26, 2022:

(I would like to remove the fStateStats grouping completely for the next release.)

jonatack commented at 1:14 pm on September 30, 2022:

Ideally the fields would just be ordered alphanumerically

That would be of benefit to neither machine nor humans? Rather it would be needless churn and confusion for humans reading the docs and using the RPC via the command line.

At least some of the RPCs return some of their output in an order that makes it easier to understand for a human, i.e. getpeerinfo begins with id/address/network fields for each peer. This seems helpful.

Whether everyone really wants to willy-nilly reorder all the fields and help docs across the board to make life confusing for people who use the CLI or not, it’s a little strange to go way out on that limb in reaction to a minimal fixup.

fanquake commented at 1:24 pm on September 30, 2022:

That would be of benefit to neither machine nor humans?

It would benefit both (and developers) by:

providing an ordering that is no-longer arbitrary
which can be tested/linted/enforced
same mechanism can be used to produce the field output, and the help output.
we can stop caring about / trying to enforce / fix completely arbitrary orderings, that are not enforced / tested / checked by anything.

jonatack commented at 1:48 pm on September 30, 2022:

Machines don’t care about ordering and humans can find ordering helpful.

Such a large change won’t be done for v24 (if at all) and the argument isn’t relevant here.

in src/rpc/net.cpp:149 in e6603e50f4 outdated

145@@ -146,7 +146,7 @@ static RPCHelpMan getpeerinfo()
146                     {
147                         {RPCResult::Type::STR, "permission_type", Join(NET_PERMISSIONS_DOC, ",\n") + ".\n"},
148                     }},
149-                    {RPCResult::Type::NUM, "minfeefilter", /*optional=*/true, "The minimum fee rate for transactions this peer accepts"},

fanquake commented at 9:07 am on September 22, 2022:

in 37d5cdbaac6f84aff90942c03f83e094a09c7bde:

I see this is a partial revert of #24691, which was a follow up to #21160? This commit would be easier to follow without the previous move-only commit.

This was inadvertently changed on master after the v23 branch-off.

It would be good to link to actual discussion / history rather than saying “inadvertently changed”. It’s not clear that anything inadvertent happened. The change was made, docs/optional was added / fixed, but now it’s being changed back again.

jonatack commented at 2:30 pm on September 22, 2022:

See the discussion from #26109 (review). It’s hard to tell what happened and inadvertent seems the kindest explanation if one is needed, along with not linking. If any fields become newly optional in v24, ISTM that should warned about in a release note (that users can be pointed to, if they ask what happened). Edit: removed all explanation referring to changes since the last release.

in src/rpc/net.cpp:128 in e6603e50f4 outdated

124@@ -125,7 +125,7 @@ static RPCHelpMan getpeerinfo()
125                     {RPCResult::Type::NUM, "timeoffset", "The time offset in seconds"},
126                     {RPCResult::Type::NUM, "pingtime", /*optional=*/true, "ping time (if available)"},
127                     {RPCResult::Type::NUM, "minping", /*optional=*/true, "minimum observed ping time (if any at all)"},
128-                    {RPCResult::Type::NUM, "pingwait", /*optional=*/true, "ping wait (if non-zero)"},
129+                    {RPCResult::Type::NUM, "pingwait", /*optional=*/true, "The duration in milliseconds (ms) of an outstanding ping (if non-zero)"},

fanquake commented at 9:14 am on September 22, 2022:

In 3c196b1c1dacc0c1d67d6c3ebd756e2614fef89f:

Doc change seems fine, but isn’t a bugfix/backport right? Any reason to change just this field? i.e why not make the same change to pingtime?

jonatack commented at 2:06 pm on September 22, 2022:

In 3c196b1:

Doc change seems fine, but isn’t a bugfix/backport right? Any reason to change just this field? i.e why not make the same change to pingtime?

Good idea, will look at doing that. Agree, not a backport candidate.

jonatack commented at 2:49 pm on September 22, 2022:

Done as well for pingtime and minping in the same commit.

in src/bitcoin-cli.cpp:470 in e6603e50f4 outdated

466@@ -467,8 +467,8 @@ class NetinfoRequestHandler : public BaseRequestHandler
467         if (!batch[ID_NETWORKINFO]["error"].isNull()) return batch[ID_NETWORKINFO];
468 
469         const UniValue& networkinfo{batch[ID_NETWORKINFO]["result"]};
470-        if (networkinfo["version"].getInt<int>() < 209900) {
471-            throw std::runtime_error("-netinfo requires bitcoind server to be running v0.21.0 and up");
472+        if (networkinfo["version"].getInt<int>() < 239900) {

fanquake commented at 9:27 am on September 22, 2022:

In e6603e50f4fbeeeb5a0dfd25f78b83b85d542984:

This change is because the getpeerinfo “relaytxes” field was changed between v23 and v24 to become optional

If we end up bumping/changing this anyway, because of the relaytxes change, do we actually need to revert minfeefilter to being non-optional?

jonatack commented at 2:19 pm on September 22, 2022:

The idea is to maintain the current released API interface and avoid potentially breaking user space without a compelling reason (minfeefilter was added to getpeerinfo in 2018 in https://github.com/bitcoin/bitcoin/commit/5778bf95d94f4d187451a72a5b86ba4daaafe4ab and I believe it has had the same behavior since then for around 7 releases. So far, -netinfo doesn’t use it).

luke-jr commented at 3:06 pm on September 24, 2022:

The commit message appears to have lost the rationale for this bump.

The reason in e6603e50f4fbeeeb5a0dfd25f78b83b85d542984 also doesn’t make sense: making a field optional should not break compatibility.

luke-jr commented at 3:08 pm on September 24, 2022:

(I’d also prefer inlining the rationale by checking the presence of the feature itself, rather than a numeric version comparison…)

jonatack commented at 10:38 am on September 26, 2022:

@luke-jr see #25176 for the patch required that will only be present in the next release.

jonatack commented at 10:44 am on September 26, 2022:

(I’d also prefer inlining the rationale by checking the presence of the feature itself, rather than a numeric version comparison…)

Multiple fields could change, so it’s simpler to check the server version.

luke-jr commented at 12:41 pm on September 26, 2022:

@jonatack #25176 is backward compatible. Only the server side broke compatibility, and fixing it on the client side shouldn’t affect operation with older nodes.

Multiple fields could change, so it’s simpler to check the server version.

Only the first one encountered that would cause an error needs to be checked. This is just a “friendlier error message”, right?

jonatack commented at 12:56 pm on September 26, 2022:

Ah, you’re right. It should instead check for minimum v22 when the 3 addr_* fields were added. Your suggestion would be to check if one of them IsNull()? (Yes, a friendlier message, it replaces the “JSON value is not of expected type” errors.)

luke-jr commented at 2:06 pm on September 26, 2022:

Yes, I’d just remove it temporarily to see where it fails first, and add a check at that location.

jonatack commented at 4:31 pm on September 26, 2022:

Thanks again @luke-jr. I’ve dropped that last commit and removed “-netinfo” from the PR title. After checking back version by version, the -netinfo code always handled the absence or presence of the three addr_* fields in a backward compatible manner, so no need to update -netinfo further for v24.

fanquake commented at 9:39 am on September 22, 2022: member

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.

jonatack commented at 1:54 pm on September 22, 2022: contributor

It [-netinfo] should really be a best-effort component of bitcoin-cli, that just adapts to any code changes we make

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.

Make getpeerinfo field order consistent with its help (for v24 backport)

This also keeps it consistent with the last release (v23)

9cd6682545

Always return getpeerinfo "minfeefilter" field (for v24 backport)

with its pre-existing v23 default value of 0.

1f448542e7

Update getpeerinfo/-netinfo/TxRelay#m_relay_txs relaytxes docs (for v24 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.

df660ddb1c

Improve getpeerinfo pingtime, minping, and pingwait help docs a3789c700b

jonatack force-pushed on Sep 22, 2022

jonatack commented at 2:52 pm on September 22, 2022: contributor

Updated the commit messages to indicate proposed backport (and removed mentions of previous inadvertent changes), and took @fanquake’s feedback to improve the help documentation for the pingtime and minping fields in a3789c7.

in src/rpc/net.cpp:250 in 4c70a59591 outdated

246@@ -246,6 +247,7 @@ static RPCHelpMan getpeerinfo()
247             permissions.push_back(permission);
248         }
249         obj.pushKV("permissions", permissions);
250+        obj.pushKV("minfeefilter", fStateStats ? ValueFromAmount(statestats.m_fee_filter_received) : 0);

luke-jr commented at 3:07 pm on September 24, 2022:

This seems broken. It’s not necessarily 0 if fStateStats is unavailable…?

jonatack commented at 10:42 am on September 26, 2022:

It has been the default since the field was added in 2018 in https://github.com/bitcoin/bitcoin/commit/5778bf95d94f4d187451a72a5b86ba4daaafe4ab. This only maintains that behavior.

luke-jr commented at 12:42 pm on September 26, 2022:

Returning the default here is incorrect. It may be set for the peer - we don’t know.

jonatack commented at 3:49 pm on September 26, 2022:

Recent review discussion seems in favor of keeping the minfeefilter behavior with a default value.

#25923#pullrequestreview-1086617087: “there are some fields that can obviously be made non-optional (such as the fee filter), because they can be absent on the wire even long after the connection has been spun up (thus require a default value anyway)”
#25923 (comment): “I think it is ok to display the values relaytxes=false and minfeefilter=0…this is what the code did before PR 21160. If this is stripped to the bare minimum needed for the fix, I think it will have higher chance of making it to 24.0”

Though perhaps with a different default value (not proposed here).

#25923 (comment): “I could see -1 or CAmount::max() being better defaults for minfeefilter if relaying isn’t happening though, since that way you could interpret it correctly without needing to also look at relaytxes”

luke-jr changes_requested

jonatack renamed this:
~~rpc, cli: getpeerinfo and -netinfo updates~~
rpc: getpeerinfo updates
on Sep 26, 2022

jonatack force-pushed on Sep 26, 2022

jonatack renamed this:
~~rpc: getpeerinfo updates~~
rpc, doc: getpeerinfo updates
on Sep 26, 2022

brunoerg approved

brunoerg commented at 4:53 pm on September 28, 2022: contributor

ACK a3789c700b5a43efd4b366b4241ae840d63f2349

In 9cd6682545e845277d2207654250d1ed78d0c695, I just compared the results (master x this branch) and could realize now it’s consistent with its help.

In 9cd6682545e845277d2207654250d1ed78d0c695, I modified the code to test it, like:

 0diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
 1index 704a23240..9acee54c6 100644
 2--- a/src/rpc/net.cpp
 3+++ b/src/rpc/net.cpp
 4@@ -184,7 +184,7 @@ static RPCHelpMan getpeerinfo()
 5     for (const CNodeStats& stats : vstats) {
 6         UniValue obj(UniValue::VOBJ);
 7         CNodeStateStats statestats;
 8-        bool fStateStats = peerman.GetNodeStateStats(stats.nodeid, statestats);
 9+        bool fStateStats = false;
10         obj.pushKV("id", stats.nodeid);
11         obj.pushKV("addr", stats.m_addr_name);
12         if (stats.addrBind.IsValid()) {

So, I ran getpeerinfo, and I could realize minfeefilter has been returned even with fStateStats false, e.g.:

 0  {
 1    "id": 8,
 2    "addr": "34.121.41.161:8333",
 3    "addrbind": "192.168.0.73:49948",
 4    "network": "ipv4",
 5    "services": "0000000000000000",
 6    "servicesnames": [
 7    ],
 8    "lastsend": 1664375138,
 9    "lastrecv": 1664375138,
10    "last_transaction": 0,
11    "last_block": 0,
12    "bytessent": 127,
13    "bytesrecv": 198,
14    "conntime": 1664375138,
15    "timeoffset": 0,
16    "version": 0,
17    "subver": "",
18    "inbound": false,
19    "bip152_hb_to": false,
20    "bip152_hb_from": false,
21    "permissions": [
22    ],
23    "minfeefilter": 0,
24    "bytessent_per_msg": {
25      "version": 127
26    },
27    "bytesrecv_per_msg": {
28      "sendaddrv2": 24,
29      "verack": 24,
30      "version": 126,
31      "wtxidrelay": 24
32    },
33    "connection_type": "outbound-full-relay"
34  }

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 (https://github.com/bitcoin/bitcoin/pull/25923#pullrequestreview-1086617087 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.

df660ddb1cce1ee330346fe1728d868f41ad0256 and a3789c700b5a43efd4b366b4241ae840d63f2349 are both docs improvement, they seem good for me.

jonatack commented at 10:42 am on September 30, 2022: contributor

Thanks for the review @brunoerg!

I’d prefer to return -1 as proposed by #25923 (comment) because it would be easier to distinguish that there aren’t any stats.

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 commented at 11:50 am on September 30, 2022: member

I still don’t understand 9cd6682545e845277d2207654250d1ed78d0c695, 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.

9cd6682545e845277d2207654250d1ed78d0c695 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.

jonatack commented at 12:55 pm on September 30, 2022: contributor

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.

we make changes in master (ideally) to fix bugs, and then, backport (select) bug fixes as required… 9cd6682 does not fix a bug.

You have tagged non-bugfixes for backport to v24. For instance: https://github.com/bitcoin/bitcoin/pull/26145/commits/68209a7b5c0326e14508d9cf749771605bd6ffe7. The other commit in that PR wasn’t a bug, either.

JSON does not have field ordering

I’ve addressed this with #26109 (review): “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.”

If we really wanted them to align

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.

an alternative would be to change the help docs to reflact the new ordering

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 approved

vasild commented at 9:59 am on October 3, 2022: contributor

ACK a3789c700b5a43efd4b366b4241ae840d63f2349

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.

jonatack commented at 10:11 am on October 3, 2022: contributor

Non-blocker: I think the versions mentions do not belong to the commit messages

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.

achow101 commented at 2:53 pm on October 13, 2022: member

ACK a3789c700b5a43efd4b366b4241ae840d63f2349

I agree with @vasild’s non-blocking comments, however this is small and simple enough to go forward with at this point.

I do think the RPC changes are backportable to 24.0 as corrections to user facing documentation is important to backport.

achow101 added the label Needs backport (24.x) on Oct 13, 2022

achow101 merged this on Oct 13, 2022

achow101 closed this on Oct 13, 2022

jonatack deleted the branch on Oct 14, 2022

in src/bitcoin-cli.cpp:645 in a3789c700b

641@@ -642,7 +642,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
642         "  send     Time since last message sent to the peer, in seconds\n"
643         "  recv     Time since last message received from the peer, in seconds\n"
644         "  txn      Time since last novel transaction received from the peer and accepted into our mempool, in minutes\n"
645-        "           \"*\" - the peer requested we not relay transactions to it (relaytxes is false)\n"
646+        "           \"*\" - whether we relay transactions to this peer (relaytxes is false)\n"

maflcko commented at 8:37 am on October 14, 2022:

This doesn’t seem right. Even if relaytxes is true, this might be false (for example this is an outbound block-relay-only connection?)

maflcko commented at 8:39 am on October 14, 2022:

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.

jonatack commented at 10:09 pm on October 17, 2022:

Good catch. Perhaps: we don't relay transactions to this peer (relaytxes is false or not available)

jonatack commented at 10:55 pm on October 17, 2022:

Proposed a fixup in #26328.

maflcko commented at 4:26 pm on January 11, 2023:

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?

jonatack commented at 5:07 pm on January 11, 2023:

Will clarify next time I touch the file that it’s the getpeerinfo "relaytxes" field (what is version.relaytxes?)

maflcko commented at 5:12 pm on January 11, 2023:

what is version.relaytxes?

https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#extensions-to-existing-messages

jonatack commented at 5:24 pm on January 11, 2023:

Thanks! The various namings for tx relay can be a little confusing.

sidhujag referenced this in commit d8ef0f618f on Oct 23, 2022

jonatack referenced this in commit 24cbea4274 on Nov 5, 2022

jonatack referenced this in commit 5c03df1fce on Nov 5, 2022

jonatack referenced this in commit 995cdbf210 on Nov 5, 2022

jonatack referenced this in commit 598db66c43 on Nov 5, 2022

maflcko removed the label Needs backport (24.x) on Nov 17, 2022

fanquake referenced this in commit 329d7e379d on Jan 11, 2023

sidhujag referenced this in commit 46c295a4b6 on Jan 12, 2023

bitcoin locked this on Jan 11, 2024

rpc, doc: getpeerinfo updates #26109

Conflicts