Skip to content

Conversation

@jonatack
Copy link
Member

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

src/rpc/net.cpp Outdated
Copy link
Member

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.

Copy link
Member Author

@jonatack jonatack Sep 16, 2022

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.

Copy link
Member

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.

Copy link
Member Author

@jonatack jonatack Sep 16, 2022

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.

Copy link
Member Author

@jonatack jonatack Nov 10, 2022

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.

Copy link
Member Author

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.

@jonatack jonatack force-pushed the 2022-09-getpeerinfo-netinfo-updates branch from bd56f68 to e2f5c29 Compare September 16, 2022 17:08
@jonatack
Copy link
Member Author

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

git diff bd56f68 e2f5c29

diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 04d72ade3d..a8293afdc7 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -467,8 +467,8 @@ public:
         if (!batch[ID_NETWORKINFO]["error"].isNull()) return batch[ID_NETWORKINFO];
 
         const UniValue& networkinfo{batch[ID_NETWORKINFO]["result"]};
-        if (networkinfo["version"].getInt<int>() < 220000) {
-            throw std::runtime_error("This version of -netinfo requires bitcoind server to be running v22.0 and up");
+        if (networkinfo["version"].getInt<int>() < 239900) {
+            throw std::runtime_error("This version of -netinfo requires the bitcoind server to be running v24.0 and up");
         }
         const int64_t time_now{TicksSinceEpoch<std::chrono::seconds>(CliClock::now())};
 
@@ -478,7 +478,7 @@ public:
             const int8_t network_id{NetworkStringToId(network)};
             if (network_id == UNKNOWN_NETWORK) continue;
             const bool is_outbound{!peer["inbound"].get_bool()};
-            const bool is_tx_relay{peer["relaytxes"].get_bool()};
+            const bool is_tx_relay{peer["relaytxes"].isNull() ? true : peer["relaytxes"].get_bool()};
             const std::string conn_type{peer["connection_type"].get_str()};
             ++m_counts.at(is_outbound).at(network_id);      // in/out by network
             ++m_counts.at(is_outbound).at(NETWORKS.size()); // in/out overall
@@ -642,7 +642,7 @@ public:
         "  send     Time since last message sent to the peer, in seconds\n"
         "  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"
-        "           \"*\" - whether we currently relay transactions to this peer (relaytxes is false)\n"
+        "           \"*\" - whether we relay transactions to this peer (relaytxes is false)\n"
         "  blk      Time since last novel block passing initial validity checks received from the peer, in minutes\n"
         "  hb       High-bandwidth BIP152 compact block relay\n"
         "           \".\" (to)   - we selected the peer as a high-bandwidth peer\n"
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 781debd132..28148e1c65 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -271,11 +271,11 @@ struct Peer {
 
     struct TxRelay {
         mutable RecursiveMutex m_bloom_filter_mutex;
-        /** Whether we currently relay transactions to this peer.
+        /** Whether we relay transactions to this peer.
          *
          * On receiving a `version` message, this is initially set to false
-         * either if the peer is outbound block-relay-only, or otherwise based
-         * on the fRelay flag.
+         * if the peer is outbound block-relay-only and otherwise based on
+         * the fRelay flag.
          *
          * If initially set to false, it can only be flipped to true if we have
          * offered the peer NODE_BLOOM services and it sends us a `filterload`
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 41c4a2c539..e953f8ba25 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -114,7 +114,7 @@ static RPCHelpMan getpeerinfo()
                     {
                         {RPCResult::Type::STR, "SERVICE_NAME", "the service name if it is recognised"}
                     }},
-                    {RPCResult::Type::BOOL, "relaytxes", "Whether we currently relay transactions to this peer"},
+                    {RPCResult::Type::BOOL, "relaytxes", /*optional=*/true, "Whether we relay transactions to this peer"},
                     {RPCResult::Type::NUM_TIME, "lastsend", "The " + UNIX_EPOCH_TIME + " of the last send"},
                     {RPCResult::Type::NUM_TIME, "lastrecv", "The " + UNIX_EPOCH_TIME + " of the last receive"},
                     {RPCResult::Type::NUM_TIME, "last_transaction", "The " + UNIX_EPOCH_TIME + " of the last valid transaction received from this peer"},
@@ -200,7 +200,9 @@ static RPCHelpMan getpeerinfo()
         ServiceFlags services{fStateStats ? statestats.their_services : ServiceFlags::NODE_NONE};
         obj.pushKV("services", strprintf("%016x", services));
         obj.pushKV("servicesnames", GetServicesNames(services));
-        obj.pushKV("relaytxes", fStateStats ? statestats.m_relay_txs : false);
+        if (fStateStats) {
+            obj.pushKV("relaytxes", statestats.m_relay_txs);
+        }
         obj.pushKV("lastsend", count_seconds(stats.m_last_send));
         obj.pushKV("lastrecv", count_seconds(stats.m_last_recv));
         obj.pushKV("last_transaction", count_seconds(stats.m_last_tx_time));

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 17, 2022

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.

@jonatack jonatack force-pushed the 2022-09-getpeerinfo-netinfo-updates branch from e2f5c29 to 56edac7 Compare September 20, 2022 08:18
@jonatack
Copy link
Member Author

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

@jonatack jonatack force-pushed the 2022-09-getpeerinfo-netinfo-updates branch from 56edac7 to e6603e5 Compare September 20, 2022 08:23
Copy link
Member

@fanquake fanquake left a 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.

@jonatack
Copy link
Member Author

jonatack commented Sep 22, 2022

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.

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.
@jonatack jonatack force-pushed the 2022-09-getpeerinfo-netinfo-updates branch from e6603e5 to 4c70a59 Compare September 22, 2022 14:49
@jonatack
Copy link
Member Author

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.

@jonatack jonatack changed the title rpc, cli: getpeerinfo and -netinfo updates rpc: getpeerinfo updates Sep 26, 2022
@jonatack jonatack force-pushed the 2022-09-getpeerinfo-netinfo-updates branch from 4c70a59 to a3789c7 Compare September 26, 2022 16:32
@jonatack jonatack changed the title rpc: getpeerinfo updates rpc, doc: getpeerinfo updates Sep 26, 2022
Copy link
Contributor

@brunoerg brunoerg left a 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.

@jonatack
Copy link
Member Author

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?

Copy link
Member

@fanquake fanquake left a 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.

@jonatack
Copy link
Member Author

jonatack commented Sep 30, 2022

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: 68209a7. The other commit in that PR wasn't a bug, either.

JSON does not have field ordering

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."

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.

Copy link
Contributor

@vasild vasild left a 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.

@jonatack
Copy link
Member Author

jonatack commented Oct 3, 2022

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
Copy link
Member

ACK a3789c7

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 achow101 merged commit cb9764b into bitcoin:master Oct 13, 2022
@jonatack jonatack deleted the 2022-09-getpeerinfo-netinfo-updates branch October 14, 2022 02:50
" 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"
Copy link
Member

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?)

Copy link
Member

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.

Copy link
Member Author

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)

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

@jonatack jonatack Jan 11, 2023

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?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 23, 2022
fanquake added a commit to bitcoin-core/gui that referenced this pull request Jan 11, 2023
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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 12, 2023
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
@bitcoin bitcoin locked and limited conversation to collaborators Jan 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants