Fix frequent -netinfo JSON errors from missing getpeerinfo#relaytxes #25176
Conversation
"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().
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.
14c5f3a to
a17c5e9
Compare
| @@ -477,7 +477,7 @@ class NetinfoRequestHandler : public BaseRequestHandler | |||
| const int8_t network_id{NetworkStringToId(network)}; | |||
| if (network_id == UNKNOWN_NETWORK) continue; | |||
| const bool is_outbound{!peer["inbound"].get_bool()}; | |||
| const bool is_block_relay{!peer["relaytxes"].get_bool()}; | |||
| const bool is_tx_relay{peer["relaytxes"].isNull() ? true : peer["relaytxes"].get_bool()}; | |||
There was a problem hiding this comment.
If relaytxes is missing, it's not necessarily true - could just as well be false.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)".
There was a problem hiding this comment.
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
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 |
…etpeerinfo#relaytxes a17c5e9 Rename NetinfoRequestHandler::is_block_relay data member to is_tx_relay (Jon Atack) f0bb7db Fix frequent -netinfo JSON errors from null getpeerinfo#relaytxes (Jon Atack) Pull request description: CLI -netinfo frequently returns "error: JSON value is not a boolean as expected" since the merge of bitcoin#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 bitcoin#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.) ACKs for top commit: mzumsande: Code review ACK a17c5e9 Tree-SHA512: dc54ce80b78122874a6794555f99e5b328a1574b52bb3e7f974c699c2b759a60ea0807a6483c5bc0414a950d853c0eeeb13dcc1b790d3917c6ee4c9c99fe159f
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.)