doc: fix -netinfo relaytxes help

jonatack commented at 10:54 pm on October 17, 2022: contributor

Addresses #26109 (review) by Marco Falke (thanks!)

DrahtBot added the label Docs on Oct 18, 2022

in src/bitcoin-cli.cpp:645 in e49ecf35e2 outdated

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-        "           \"*\" - whether we relay transactions to this peer (relaytxes is false)\n"
646+        "           \"*\" - we do not relay transactions to this peer (relaytxes is false or not available)\n"

maflcko commented at 6:26 am on October 18, 2022:

if it is not available it will default to true, no?

shaavan commented at 12:08 pm on October 18, 2022:

@MarcoFalke Yes, I was able to verify your point

On line https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-cli.cpp#L542, the "*" or "" is selected based on bool value of is_tx_relay.

0peer.last_trxn ? ToString((time_now - peer.last_trxn) / 60) : peer.is_tx_relay ? "" : "*",

and, on line https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-cli.cpp#L481, is_tx_relay defaults to True when relaytxes is Null

0const bool is_tx_relay{peer["relaytxes"].isNull() ? true : peer["relaytxes"].get_bool()};

jonatack commented at 7:56 pm on October 18, 2022:

Thanks, done, and added displaying the new “not available” state distinctly from the “true but no last tx time” state.

jonatack renamed this:
~~doc: fix up -netinfo relaytxes help~~
netinfo: fix relaytxes doc, display 3 relaytxes states
on Oct 18, 2022

jonatack force-pushed on Oct 18, 2022

jonatack commented at 8:17 pm on October 18, 2022: contributor

@MarcoFalke @shaavan thanks for taking a look! Addressed your feedback, improved the getpeerinfo#relaytxes help, and added explicitly handling the new (v24.0) relaytxes 3rd state of “not available” in -netinfo.

jonatack force-pushed on Oct 18, 2022

shaavan commented at 12:48 pm on October 19, 2022: contributor

relaytxes is set based on the value of m_relay_txes if fStateStats is true; otherwise, relaytxes is unavailable.

net.cpp 203

0if (fStateStats) {
1            obj.pushKV("relaytxes", statestats.m_relay_txs);
2        }

And fStateStats is equal to false when either Node or peer is nullptr:

net_processing.cpp 1555

0if (state == nullptr)
1            return false;

net_processing.cpp 1566

0if (peer == nullptr) return false;

The value of relaytxes depends on the value of statestats.m_relay_txs.

And the value of statestats.m_relay_txs can be false in two cases:

if m_tx_relay→m_relay_txs is false.

net_processing.cpp 1580

0if (auto tx_relay = peer->GetTxRelay(); tx_relay != nullptr) {
1        stats.m_relay_txs = WITH_LOCK(tx_relay->m_bloom_filter_mutex, return tx_relay->m_relay_txs);

or if m_tx_relay is a nullptr.

net_processing.cpp 1583

0else {
1        stats.m_relay_txs = false;

And according to the comment on m_tx_relay, it will be a nullptr when we are not relaying transactions to this peer

net_processing.cpp 398

0/** Transaction relay data. Will be a nullptr if we're not relaying
1     *  transactions with this peer (e.g., if it's a block-relay-only peer or
2     *  the peer has sent us fRelay=false with bloom filters disabled). */
3    std::unique_ptr<TxRelay> m_tx_relay GUARDED_BY(m_tx_relay_mutex);

So as far as I understood, there are four possible states:

Node asked to relay, and we will relay txs to this node (m_tx_relay ≠ nullptr and m_tx_relay → m_relay_txs = True)
Node asked not to relay (m_tx_relay ≠ nullptr and m_tx_relay→m_relay_txs = False)
Node is not yet setup (peer == nullptr, irrespective of values of m_tx_relay and m_relay_txs)
Node is setup but we can’t relay to it (m_tx_relay = nullptr)

The first three states are properly represented by the PR using “”, “*”, “.” repectively. However, I think there is a need to address the 4th state as well in which we can’t relay transactions to the peer.

jonatack commented at 5:25 pm on October 20, 2022: contributor

4. Node is setup but we can’t relay to it (`m_tx_relay` = **nullptr**)
The first three states are properly represented by the PR using “”, “*”, “.” repectively. However, I think there is a need to address the 4th state as well in which we can’t relay transactions to the peer. @shaavan thanks for looking. Running git grep -B4 "stats.m_relay_txs = false;", in the 4th case you describe PeerManagerImpl::GetNodeStateStats() returns m_relay_txs as false (and so getpeerinfo#relaytxes is false). This is one of the 3 states (true, false, not available) returned by that field and handled here.

brunoerg commented at 12:29 pm on October 26, 2022: contributor

utACK d51dbbc697adeac2563727e27f49b98bcc319914

I was able to verify the three states in the code and the lack of documentation that this PR fixes. Couldn’t we cover this case where relaytxes is not available in a functional test? I confess that I could test manually this field by running it, and could check true/false states but it has been a little difficult to test manually and get a n/a.

luke-jr commented at 1:08 am on October 27, 2022: member

If relaytxes is omitted ONLY if it hasn’t been setup yet, we should just include it as false IMO.

(But if it could be during peer destruction, that might not be correct)

jonatack commented at 5:23 pm on October 28, 2022: contributor

@luke-jr I tend to agree; nevertheless see #26109 (review) for the rationale, which was considered compelling enough to make getpeerinfo#relaytxes optional after being always present since the API field was introduced in 08843ed99843078acb10eecda2045d5f0f1c2b4f. Edit: I’ve added the link to the pull description.

Thank you @brunoerg! @MarcoFalke @shaavan mind re-reviewing? @w0xlt @dunxen @kristapsk @1440000bytes @Zero-1729 @0xB10C @jarolrod you have reviewed these kind of changes in the past, if you don’t mind having a look.

in src/bitcoin-cli.cpp:645 in d51dbbc697 outdated

642@@ -642,7 +643,8 @@ class NetinfoRequestHandler : public BaseRequestHandler
643         "  send     Time since last message sent to the peer, in seconds\n"
644         "  recv     Time since last message received from the peer, in seconds\n"
645         "  txn      Time since last novel transaction received from the peer and accepted into our mempool, in minutes\n"
646-        "           \"*\" - whether we relay transactions to this peer (relaytxes is false)\n"
647+        "           \".\" - we do not relay transactions to this peer (relaytxes not available as peer connection is still being set up)\n"
648+        "           \"*\" - we do not relay transactions to this peer (relaytxes is false)\n"

Zero-1729 commented at 1:05 am on October 30, 2022:

IMO, it would be worthwhile to use ‘.’ to signal we aren’t relaying txs (i.e. relaytxes is false) and have ‘*’ signal the case where the info is unavailable.

Mainly for two reasons, consistency and intuition. Regarding consistency, addrp already uses ‘.’ to signal that we aren’t relaying addresses (specifically, when addr_relay_enabled is false) which is a similar case here, as relaytxes is false. Though currently on master, ‘*’ is used to show when relaytxes is false, it breaks that consistency also, and I’d say ‘*’ intuitively seems more like an undetermined/pending state than ‘.’.

jonatack commented at 9:47 pm on November 4, 2022:

It’s a slightly larger change of behavior, but sure – done.

jonatack force-pushed on Nov 4, 2022

jonatack commented at 9:49 pm on November 4, 2022: contributor

Updated to take feedback by @Zero-1729, ACKs and re-ACKs welcome :)

jonatack referenced this in commit 4b5f99e926 on Nov 5, 2022

in doc/release-notes-26328.md:4 in 7c733bd25e outdated

0@@ -0,0 +1,7 @@
1+Tools and Utilities
2+-------------------
3+
4+- CLI `-netinfo` now displays 3 different states in the `txn` column (true, false, and not available), as the

Zero-1729 commented at 5:42 am on November 5, 2022:

Slight rewording for clarity

0- CLI `-netinfo` now displays 3 different states in the `txn` column (true, false, and not available). The

jonatack commented at 5:53 pm on November 5, 2022:

Done

in doc/release-notes-26328.md:5 in 7c733bd25e outdated

0@@ -0,0 +1,7 @@
1+Tools and Utilities
2+-------------------
3+
4+- CLI `-netinfo` now displays 3 different states in the `txn` column (true, false, and not available), as the
5+  latter value is a new, third state since 24.0 returned by the RPC getpeerinfo "relaytxes" field when the peer

Zero-1729 commented at 5:42 am on November 5, 2022:

(cont’d)

0  last value is a new third state since 24.0 returned by the RPC getpeerinfo "relaytxes" field when the peer

jonatack commented at 5:54 pm on November 5, 2022:

Thanks, replaced “latter” with “not available”.

Zero-1729 commented at 5:42 am on November 5, 2022: contributor

tACK 7c733bd25e024a23e75fb1015e1c394df88df2d0

Tested on macOS v13.0 (22A380)

I can confirm this patch splits the displayed relaytxes states into three (true, false, n/a due to pending setup), updates the help docs, and release notes.

Brief digest diff

On master (ae6bb6e71e3082dd783e78c52b3af649fd5256cc)

 0$ ./src/bitcoin-cli -signet -netinfo 4   
 1Bitcoin Core client v24.99.0-ae6bb6e71e30-dirty signet - server 70016/Satoshi:24.99.0/
 2
 3<->   type   net  mping   ping send recv  txn  blk  hb addrp addrl  age id address               version
 4out   full  ipv4    175    175   15   22         0       172          0  3 85.208.70.229:38333   70016/Satoshi:23.0.0/
 5out   full  ipv4    301    301   16   16                 123          0  4 202.61.248.219:38333  70016/Satoshi:24.99.0/
 6out   full  ipv4    517    517   22   22         0  .    184          0  2 72.48.253.168:38333   70016/Satoshi:22.0.0/
 7out   full  ipv4    570    570   10   10                 230          0  5 147.182.229.68:38333  70016/Satoshi:22.99.0/
 8out  block  ipv4    753    753   22   22    *    0         .          0  0 18.166.228.180:38333  70016/Satoshi:23.0.0/
 9out  block  ipv4    879    879   22   22    *    0  .      .          0  1 103.16.128.63:38333   70016/Satoshi:23.0.0/
10out   full  ipv4   1247   1247    7    7                 205          0  6 178.128.221.177:38333 70016/Satoshi:22.99.0/
11                     ms     ms  sec  sec  min  min                  min
12
13         ipv4    ipv6   total   block
14in          0       0       0
15out         7       0       7       2
16total       7       0       7
17
18Local addresses: n/a

 0$ ./src/bitcoin-cli -signet getpeerinfo   
 1[
 2  {
 3    "id": 0,
 4    "addr": "18.166.228.180:38333",
 5    "addrbind": "192.168.0.161:53506",
 6    "addrlocal": "102.91.5.10:53506",
 7    "network": "ipv4",
 8    "services": "0000000000000409",
 9    "servicesnames": [
10      "NETWORK",
11      "WITNESS",
12      "NETWORK_LIMITED"
13    ],
14    "relaytxes": false,
15    …
16  },
17  …
18  {
19    "id": 2,
20    "addr": "72.48.253.168:38333",
21    "addrbind": "192.168.0.161:53509",
22    "addrlocal": "197.210.70.183:53509",
23    "network": "ipv4",
24    "services": "000000000000040d",
25    "servicesnames": [
26      "NETWORK",
27      "BLOOM",
28      "WITNESS",
29      "NETWORK_LIMITED"
30    ],
31    "relaytxes": true,
32    …
33  },
34  …
35]

On Patch (7c733bd25e024a23e75fb1015e1c394df88df2d0)

 0$ ./src/bitcoin-cli -signet -netinfo 4  
 1Bitcoin Core client v24.99.0-7c733bd25e02 signet - server 70016/Satoshi:24.99.0/
 2
 3<->   type   net  mping   ping send recv  txn  blk  hb addrp addrl  age id address               version
 4out   full  ipv4    197    203   45   45                 242          4  7 49.12.208.214:38333   70016/Satoshi:0.21.1/
 5out   full  ipv4    198    200    2   53                 172          4  4 85.208.70.229:38333   70016/Satoshi:23.0.0/
 6out   full  ipv4    200    249   54   53    2            124          4  3 202.61.248.219:38333  70016/Satoshi:24.99.0/
 7out   full  ipv4    225    225   32   32                 148          4  9 31.14.40.19:38333     70016/Satoshi:22.0.0/
 8out   full  ipv4    249    284   51   51                 121          4  6 108.161.223.181:38333 70016/Satoshi:22.0.0/
 9out   full  ipv4    291    291   54   54    1            178          4  2 45.33.47.11:38333     70016/Satoshi:23.0.0/
10out   full  ipv4    302    310   52   52                 139          4  5 104.128.228.143:38333 70016/Satoshi:23.0.0/
11out   full  ipv4    303    444   44   44    3    2  .    230          4  8 147.182.229.68:38333  70016/Satoshi:22.99.0/
12out  block  ipv4    387    405   62   62    .    5  .      .          5  0 18.166.228.180:38333  70016/Satoshi:23.0.0/
13out  block  ipv4    442    442   61   61    .              .          5  1 103.16.128.63:38333   70016/Satoshi:23.0.0/
14                     ms     ms  sec  sec  min  min                  min
15
16         ipv4    ipv6   total   block
17in          0       0       0
18out        10       0      10       2
19total      10       0      10
20
21Local addresses: n/a

 0$ ./src/bitcoin-cli -signet getpeerinfo   
 1[
 2  {
 3    "id": 0,
 4    "addr": "18.166.228.180:38333",
 5    "addrbind": "192.168.0.161:59682",
 6    "addrlocal": "197.210.71.89:64632",
 7    "network": "ipv4",
 8    "services": "0000000000000409",
 9    "servicesnames": [
10      "NETWORK",
11      "WITNESS",
12      "NETWORK_LIMITED"
13    ],
14    "relaytxes": false,
15    "lastsend": 1667626204,
16    "lastrecv": 1667626204,
17    "last_transaction": 0,
18    …
19  },
20  …
21  {
22    "id": 4,
23    "addr": "85.208.70.229:38333",
24    "addrbind": "192.168.0.161:59702",
25    "addrlocal": "102.91.5.10:59702",
26    "network": "ipv4",
27    "services": "0000000000000409",
28    "servicesnames": [
29      "NETWORK",
30      "WITNESS",
31      "NETWORK_LIMITED"
32    ],
33    "relaytxes": true,
34    "lastsend": 1667626198,
35    "lastrecv": 1667626093,
36    "last_transaction": 0,
37    …
38  },
39  …
40 {
41    "id": 8,
42    "addr": "147.182.229.68:38333",
43    "addrbind": "192.168.0.161:59717",
44    "addrlocal": "197.210.71.89:59717",
45    "network": "ipv4",
46    "services": "0000000000000409",
47    "servicesnames": [
48      "NETWORK",
49      "WITNESS",
50      "NETWORK_LIMITED"
51    ],
52    "relaytxes": true,
53    "lastsend": 1667626197,
54    "lastrecv": 1667626102,
55    "last_transaction": 1667625918,
56    …
57  }
58  …
59]

jonatack force-pushed on Nov 5, 2022

jonatack commented at 5:57 pm on November 5, 2022: contributor

Updated to take feedback by @Zero-1729, ACKs and re-ACKs welcome :)

Zero-1729 approved

Zero-1729 commented at 6:01 pm on November 5, 2022: contributor

crACK cfabd15309eb42a9f20f4c11152c54fd27978fab

in src/bitcoin-cli.cpp:647 in b54d8de243 outdated

642@@ -642,7 +643,8 @@ class NetinfoRequestHandler : public BaseRequestHandler
643         "  send     Time since last message sent to the peer, in seconds\n"
644         "  recv     Time since last message received from the peer, in seconds\n"
645         "  txn      Time since last novel transaction received from the peer and accepted into our mempool, in minutes\n"
646-        "           \"*\" - we do not relay transactions to this peer (relaytxes is false)\n"
647+        "           \".\" - we do not relay transactions to this peer (relaytxes is false)\n"
648+        "           \"*\" - we do not relay transactions to this peer (relaytxes not available as peer connection is still being set up)\n"

mzumsande commented at 3:59 pm on November 16, 2022:

See #26457 (review), I think the third state (N/A) would not be used during connection startup as intended. I confirmed this by adding a sleep at the beginning of ::VERSION processing in ProcessMessage and querying getpeerinfo with this branch - I’d get relaytxes= false but the field is included in the response.

DrahtBot commented at 3:59 pm on November 16, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande
Stale ACK	brunoerg, Zero-1729

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

bitcoin deleted a comment on Nov 16, 2022

jonatack commented at 4:15 am on November 17, 2022: contributor

Closing for now in favor of #26516.

jonatack closed this on Nov 17, 2022

jonatack commented at 4:25 am on November 17, 2022: contributor

Re-opening after reading #26457 (comment), pending reviewer feedback here and on #26516 (the latter has the benefit of not changing user space).

jonatack reopened this on Nov 17, 2022

fanquake commented at 10:48 am on December 5, 2022: member

What are the next steps here?

pending reviewer feedback here and on #26516 (the latter has the benefit of not changing user space).

The reviewer feedback in #26516 seems to be that the change is either incorrect, or maybe needs better justification.

The reviewer feedback in this PR seems to be that the tri-state change is unnecessary, because the third state to display (N/A), will never be used, as it’s impossible to have missing values during connection setup, as described in the docs added here.

maflcko commented at 11:28 am on December 5, 2022: member

I think how RPC returns that a value is missing, (with optional, or a magic value, or some extended logic) is orthogonal to cli netinfo changes. However, it would be beneficial to first figure out the RPC interface and then adjust the cli and gui code to that.

jonatack renamed this:
~~netinfo: fix relaytxes doc, display 3 relaytxes states~~
doc: fix -netinfo relaytxes help
on Jan 10, 2023

doc: fix up -netinfo relaytxes help

Co-authored-by: "MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>"

0f5fc4f656

jonatack force-pushed on Jan 10, 2023

jonatack commented at 9:08 pm on January 10, 2023: contributor

Simplified to just the fixup to #26109 (review) found by @MarcoFalke.

jonatack referenced this in commit a34312b3b7 on Jan 10, 2023

mzumsande commented at 4:19 pm on January 11, 2023: contributor

Code Review ACK 0f5fc4f656d0990802ab552c0e620f49e785fed4

maflcko commented at 4:32 pm on January 11, 2023: member

I think my misunderstanding came from the fact that it is unclear if version.relaytxs or getpeerinfo.relaytxs was meant, see https://github.com/bitcoin/bitcoin/pull/26109/files#r1067201660

In any case https://github.com/bitcoin/bitcoin/commit/0f5fc4f656d0990802ab552c0e620f49e785fed4 lgtm

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

fanquake commented at 4:40 pm on January 11, 2023: member

This has been merged.

fanquake closed this on Jan 11, 2023

jonatack deleted the branch on Jan 11, 2023

jonatack commented at 5:00 pm on January 11, 2023: contributor

I think my misunderstanding came from the fact that it is unclear if version.relaytxs or getpeerinfo.relaytxs was meant, see #26109 (files)

In any case 0f5fc4f lgtm

I almost added s/relaytxes/getpeerinfo \"relaytxes\" field/ here yesterday. Might sneak it next time I touch the file.

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

bitcoin locked this on Jan 11, 2024

doc: fix -netinfo relaytxes help #26328

Brief digest diff

Reviews

Conflicts