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

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:fix-netinfo-json-errors-from-null-getpeerinfo-relaytxes-field changing 1 files +4 −4

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

jonatack cross-referenced this on May 20, 2022 from issue net/net processing: Move tx inventory into net_processing by jnewbery

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:

txn Time since last novel transaction received from the peer and accepted into our mempool, in minutes "*" - 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
jnewbery cross-referenced this on Jul 4, 2022 from issue net processing: Move CNode::nServices and CNode::nLocalServices to Peer by dergoegge
jonatack cross-referenced this on Aug 24, 2022 from issue p2p: always provide CNodeStateStats and getpeerinfo/netinfo/gui updates by jonatack
jonatack cross-referenced this on Sep 22, 2022 from issue rpc, doc: getpeerinfo updates by jonatack
jonatack cross-referenced this on Nov 5, 2022 from issue [24.x] backport rpc: Require NodeStateStats object in getpeerinfo by jonatack
jonatack cross-referenced this on Nov 17, 2022 from issue rpc: Always return getpeerinfo "relaytxes" field by jonatack
bitcoin locked this on Nov 5, 2023

Contributors

Labels

Linked (view graph)

#21160 net/net processing: Move tx inventory into net_processing #24691 Fix getpeerinfo doc #25514 net processing: Move CNode::nServices and CNode::nLocalServices to Peer #25923 p2p: always provide CNodeStateStats and getpeerinfo/netinfo/gui updates #26109 rpc, doc: getpeerinfo updates #26457 [24.x] backport rpc: Require NodeStateStats object in getpeerinfo #26516 rpc: Always return getpeerinfo "relaytxes" field