Fix frequent -netinfo JSON errors from missing getpeerinfo#relaytxes

jonatack commented at 12:48 pm on May 20, 2022: contributor

CLI -netinfo frequently returns “error: JSON value is not a boolean as expected” since the merge of #21160, which moved fRelayTxes (renamed to m_relay_txs in that pull) from CNodeStats to CNodeStateStats.

This change made getpeerinfo “relaytxes” an optional field that can return UniValue IsNull(). It is the only optional field consumed by -netinfo where the latter didn’t already handle that case. See also #24691.

Also rename the NetinfoRequestHandler::is_block_relay data member to is_tx_relay and inverse its boolean logic. The naming is out of date and incorrect, as lack of request of tx relay does not imply block relay, and a preference for tx relay doesn’t imply that block relay isn’t happening. Thanks to Marco Falke and Martin Zumsande for their feedback on this.

(I may look at reducing the number of optional node stats fields via refactoring at the net processing level, but ongoing refactoring there may make that slow or complicated and this is a one-line fix that works now.)

Fix frequent -netinfo JSON errors from null getpeerinfo#relaytxes

"error: JSON value is not a boolean as expected"

due to fRelayTxes/m_relay_txs being moved in PR 21160 from CNodeStats to
CNodeStateStats, which made getpeerinfo#relaytxes an optional field that
can return UniValue IsNull().

f0bb7db34c

in src/bitcoin-cli.cpp:480 in f0bb7db34c outdated

476@@ -477,7 +477,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
477             const int8_t network_id{NetworkStringToId(network)};
478             if (network_id == UNKNOWN_NETWORK) continue;
479             const bool is_outbound{!peer["inbound"].get_bool()};
480-            const bool is_block_relay{!peer["relaytxes"].get_bool()};
481+            const bool is_block_relay{peer["relaytxes"].isNull() ? false : !peer["relaytxes"].get_bool()};

maflcko commented at 1:03 pm on May 20, 2022:

Might be better to call this is_tx_relay or has_tx_relay

(Lack of request of tx relay doesn’t imply block relay and a preference for tx relay doesn’t imply that block relay isn’t happening)

jonatack commented at 1:09 pm on May 20, 2022:

Good point, and the data member is being used here for showing tx relay and not block relay, updating.

jonatack commented at 1:27 pm on May 20, 2022:

Thanks, done.

maflcko approved

maflcko commented at 1:04 pm on May 20, 2022: member

ACK

Rename NetinfoRequestHandler::is_block_relay data member to is_tx_relay

and inverse its logic.

The naming is out of date and incorrect, as lack of request of tx relay does not
imply block relay, and a preference for tx relay doesn't imply that block relay
isn't happening.

a17c5e96b6

in src/bitcoin-cli.cpp:480 in 14c5f3a82f outdated

476@@ -477,7 +477,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
477             const int8_t network_id{NetworkStringToId(network)};
478             if (network_id == UNKNOWN_NETWORK) continue;
479             const bool is_outbound{!peer["inbound"].get_bool()};
480-            const bool is_block_relay{peer["relaytxes"].isNull() ? false : !peer["relaytxes"].get_bool()};
481+            const bool is_tx_relay{peer["relaytxes"].isNull() ? false : !peer["relaytxes"].get_bool()};

mzumsande commented at 1:45 pm on May 20, 2022:

Wouldn’t renaming this to is_tx_relay require to invert the bool? Currently, if relaytxes==true, is_tx_relay==false which seems confusing.

jonatack commented at 2:08 pm on May 20, 2022:

Yes, good call, thank you. Done.

jonatack force-pushed on May 20, 2022

DrahtBot added the label Utils/log/libs on May 20, 2022

maflcko approved

mzumsande commented at 12:00 pm on May 23, 2022: contributor

Code review ACK a17c5e96b602fed65166037b78d98605e915206b

maflcko merged this on May 23, 2022

maflcko closed this on May 23, 2022

jonatack deleted the branch on May 23, 2022

in src/bitcoin-cli.cpp:480 in a17c5e96b6

476@@ -477,7 +477,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
477             const int8_t network_id{NetworkStringToId(network)};
478             if (network_id == UNKNOWN_NETWORK) continue;
479             const bool is_outbound{!peer["inbound"].get_bool()};
480-            const bool is_block_relay{!peer["relaytxes"].get_bool()};
481+            const bool is_tx_relay{peer["relaytxes"].isNull() ? true : peer["relaytxes"].get_bool()};

luke-jr commented at 12:11 pm on May 25, 2022:

Isn’t this wrong?

jonatack commented at 12:21 pm on May 25, 2022:

Can you elaborate?

luke-jr commented at 6:00 pm on May 25, 2022:

If relaytxes is missing, it’s not necessarily true - could just as well be false.

jonatack commented at 6:13 pm on May 25, 2022:

For -netinfo purposes, that’s fine: if it is missing or true, then nothing is displayed with respect to relaytxes. Only if relaytxes is false then * is printed in the txn column. This pull doesn’t change -netinfo behavior. Per -netinfo help:

0  txn      Time since last novel transaction received from the peer and accepted into our mempool, in minutes
1           "*" - the peer requested we not relay transactions to it (relaytxes is false)

mzumsande commented at 8:30 pm on May 25, 2022:

This discussion made me look into a small quirk I observed while reviewing this PR:

Sometimes I would see a * displayed for a peer that we just connected to, which would vanish with the next refresh of -netinfo. This is because when we make an outbound connection, have sent a version message, but haven’t received an answer, the node hasn’t initialized TxRelay because it doesn’t have that info yet, so we’ll get false as an answer (code). So if we’d want to be extremely correct, the wording could be changed to “the peer didn’t request we relay transactions to it (relaytxes is false)”.

jonatack commented at 9:22 pm on May 25, 2022:

@mzumsande ACK

jonatack commented at 7:11 pm on August 24, 2022:

This discussion made me look into a small quirk I observed while reviewing this PR:

Sometimes I would see a * displayed for a peer that we just connected to, which would vanish with the next refresh of -netinfo. This is because when we make an outbound connection, have sent a version message, but haven’t received an answer, the node hasn’t initialized TxRelay because it doesn’t have that info yet, so we’ll get false as an answer (code). So if we’d want to be extremely correct, the wording could be changed to “the peer didn’t request we relay transactions to it (relaytxes is false)”. @mzumsande added a commit authored by you in #25923

luke-jr commented at 12:13 pm on May 25, 2022: member

This change made getpeerinfo “relaytxes” an optional field that can return UniValue IsNull().

Hypothetically, but shouldn’t it still be an edge case? Why would it be frequently missing?

jonatack commented at 12:25 pm on May 25, 2022: contributor

Hypothetically, but shouldn’t it still be an edge case? Why would it be frequently missing?

It can happen during each new peer connection. I run watch -t ./src/bitcoin-cli -rpcwait -netinfo 4 for a live dashboard that refreshes by default every couple seconds. When doing this, it could be several times a minute or sometimes not for a while.

sidhujag referenced this in commit 21ede5a53a on May 28, 2022

bitcoin locked this on Nov 5, 2023

Fix frequent -netinfo JSON errors from missing getpeerinfo#relaytxes #25176