[24.x] backport rpc: Require NodeStateStats object in getpeerinfo

jonatack commented at 3:03 am on November 5, 2022: contributor

Backports #26515.

DrahtBot added the label Backport on Nov 5, 2022

hebasto commented at 10:54 am on November 5, 2022: member

I doubt whether changes in translatable strings should be backported, as they will be unnoticed by translators.

in src/bitcoin-cli.cpp:645 in 0e4b959e0a 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-        "           \"*\" - the peer requested we not relay transactions to it (relaytxes is false)\n"
646+        "           \"*\" - we do not relay transactions to this peer (relaytxes is false)\n"

luke-jr commented at 3:37 pm on November 5, 2022:

Sorry for the late comment, but where did this behaviour change? Or was the previous description always wrong?

I can only find (in 24.x) that the data moved around, possibly making it sometimes unavailable, but no behaviour changes…

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

More the latter, I think: #25923 (review), #25176 (review).

luke-jr commented at 3:38 pm on November 5, 2022: member

I doubt whether changes in translatable strings should be backported, as they will be unnoticed by translators.

Better to have an correct English string, than an incorrect native string. Assuming it is a fix.

mzumsande commented at 10:36 pm on November 15, 2022: contributor

I was trying to understand how it is possible that relaytxes or minfeefilter of getpeerinfo can be non-existent (see also related IRC discussion): During connection setup (both inbound or outbound), we always first call InitializeNode() before we add the new node to m_nodes. InitializeNode() adds a CNodeState entry to m_node_states and a PeerRef to m_peer_map. In the getpeerinfo RPC, we call connman.GetNodeStats() which loops over m_nodes and returns copies of its entries. So if there is a m_nodes entry, there should always also be a CNodeState and a PeerRef for this peer at this exact point in time.

The only situation I can think of in which peerman.GetNodeStateStats() could return false is if there is a race, such that a peer was returned from the GetNodeStats() call of getpeerinfo, but has been disconnected and removed when we call peerman.GetNodeStateStats() a bit later. I could verify this by adding a sleep after GetNodeStats() here, and manually disconnecting a peer before the RPC could continue. Maybe the best way to deal with that is just not reporting the already disconnected peer in getpeerinfo, and making the RPC result fields non-optional again. Thoughts on this?

As for the impact: I think that the chance of hitting this without an artificially added sleep is quite low, so unless you fire up a getpeerinfo call multiple times in a second, you probably will never get into the situation where relaytxes or minfeefilter are missing.

maflcko commented at 8:33 am on November 16, 2022: member

Maybe the best way to deal with that is just not reporting the already disconnected peer in getpeerinfo, and making the RPC result fields non-optional again. Thoughts on this?

I always assumed that this may also happen when the CNode is spun up. Good to know it only happens on disconnect. However, I don’t think this will help us, as the relaytxes change was done on purpose, because you don’t know when the peer is going to send you their version message and what it contains. So I think the changes here are correct.

maflcko approved

maflcko commented at 8:33 am on November 16, 2022: member

lgtm

in src/rpc/net.cpp:117 in 4b5f99e926 outdated

113@@ -114,7 +114,7 @@ static RPCHelpMan getpeerinfo()
114                     {
115                         {RPCResult::Type::STR, "SERVICE_NAME", "the service name if it is recognised"}
116                     }},
117-                    {RPCResult::Type::BOOL, "relaytxes", /*optional=*/true, "Whether we relay transactions to this peer"},
118+                    {RPCResult::Type::BOOL, "relaytxes", /*optional=*/true, "Whether we relay transactions to this peer (not available while peer connection is being set up)"},

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

As soon as getpeerinfo reports a new peer, we also have CNodeStateStats entry - but no TxRelay object yet. This means, we report "relaytxes": false in getpeerinfo (code) when the connection is being set up, but the field should be available.

jonatack commented at 6:38 pm on November 17, 2022:

Agree, the intermittently absent state of the field was occurring at disconnection rather than at connection.

mzumsande commented at 3:36 pm on November 16, 2022: contributor

I don’t think this will help us, as the relaytxes change was done on purpose, because you don’t know when the peer is going to send you their version message and what it contains. So I think the changes here are correct.

This PR targeted for backport only includes the doc changes wrt relaytxes, the functional change b54d8de243b6f04e6a328e9c0cbff7a17dd7fa1e of #26328 to report 3 states is not included here - also see comment below.

DrahtBot commented at 3:36 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	fanquake

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

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

This PR targeted for backport only includes the doc changes wrt relaytxes, the functional change b54d8de of #26328 to report 3 states is not included here - also see comment below.

Yes, I didn’t propose to backport that change unless it was requested, as it is somewhat of a feature.

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

As for the impact: I think that the chance of hitting this without an artificially added sleep is quite low, so unless you fire up a getpeerinfo call multiple times in a second, you probably will never get into the situation where relaytxes or minfeefilter are missing.

I do hit this for relaytxes more than rarely, but you may be correct that it happens only during peer deconnection. Edit: yes, peer disconnection.

maflcko referenced this in commit 82fe672ea0 on Nov 17, 2022

bitcoin deleted a comment on Nov 17, 2022

maflcko commented at 9:28 am on November 17, 2022: member

Better to have an correct English string, than an incorrect native string. Assuming it is a fix.

On a second thought, I don’t think this should be backported, because it is not a regression, thus not a blocker at this stage of the release cycle.

No objection to a 24.1 backport, assuming it is a fix, but right now we should probably focus on real regressions/blockers/bugs for 24.0

maflcko added this to the milestone 24.1 on Nov 17, 2022

maflcko removed this from the milestone 24.1 on Nov 17, 2022

jonatack commented at 6:02 pm on November 17, 2022: contributor

It seems doubtful now as to whether making the relaytxes and minfeefilter fields intermittently absent brought any benefit (other than developers looking more into the code). However, I don’t think here is any doubt that it is a potential regression for users. One that is perhaps minor, but without a compelling reason to have done so.

sidhujag referenced this in commit 1b969ba2d0 on Nov 18, 2022

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

Maybe mark this as a draft for now? This is dependant on multiple other PRs (26328, which also has an alternative in 26516). It also needs updating to remove changes that are incorrect (4b5f99e9268e18748d0146250d3d403f80e409e0 - #26457 (review)), and to remove commits which are backporting changes that have since been reverted in master, i.e 0e4b959e0a3874233b1549f9e7a257f47d40539f (reverted in https://github.com/bitcoin-core/gui/pull/681).

maflcko referenced this in commit 3d974960d3 on Dec 19, 2022

sidhujag referenced this in commit bbc8a636d4 on Dec 19, 2022

jonatack marked this as a draft on Jan 10, 2023

jonatack force-pushed on Jan 10, 2023

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

Done, updated and marked as draft as requested pending merge of #26328.

fanquake commented at 3:24 pm on January 11, 2023: member

Concept ~0 on backporting 24cbea4274b68823eccce8507ac348ed161f4aa9 and 5c03df1fce744fbc357e32f66f9a50b8e609c386 (which partially reverts the change in the prior commit). In master, we now only return this data if we have it (#26515), and if anything, it might make more sense to just backport that. We also haven’t had any reports of this being an issue post release.

995cdbf210a3a05fe1f991b9772f0b4e12bafe76 seems ok, but is then also partially reverted in the following commit: a34312b3b7231f5e8fc97c2b3746f1e3c1d29206.

Same in the last two commits. fe335508b9d9645d119c75fc06b7eb93473db538 is backported, only to be partially reverted it in the following commit: ba28fa73d65a96581961bdf088c82d37181c7e91.

If we are going to do this, it’d be nice to make the changes in a way where half the commits aren’t just (partially) reverting changes from the prior commit.

jonatack commented at 4:54 pm on January 11, 2023: contributor

Thanks, agree we should backport https://github.com/bitcoin/bitcoin/commit/6fefd49527fa0ed9535e54f2a3e76fe2599b2350. Is it fine to squash the order-dependent commits here and list their metadata in the same backport commit?. Edit: agree it’s enough to only backport #26515.

jonatack renamed this:
~~[24.x] backports for tx relay changes in getpeerinfo/netinfo/gui~~
[24.x] backport rpc: skip getpeerinfo for a peer without CNodeStateStats
on Jan 11, 2023

jonatack force-pushed on Jan 11, 2023

rpc: Require NodeStateStats object in getpeerinfo

There is no situation in which CNodeStateStats could be
missing for a legitimate reason - this can only happen if
there is a race condition between peer disconnection and
the getpeerinfo call, in which case the disconnected peer
doesn't need to be included in the response.

Github-Pull: bitcoin#26515
Rebased-From: 6fefd49

e72313e6b3

jonatack force-pushed on Jan 11, 2023

fanquake commented at 12:21 pm on January 18, 2023: member

agree it’s enough to only backport #26515.

Should this be undrafted then, given that PR is what it currently backports?

cc @mzumsande

maflcko added this to the milestone 24.1 on Jan 19, 2023

jonatack marked this as ready for review on Jan 19, 2023

jonatack commented at 7:10 pm on January 19, 2023: contributor

Should this be undrafted

Done. Should we also add #26838 and #26727 here?

fanquake commented at 11:20 am on January 20, 2023: member

Rather than adding more changes here, and having multiple PRs open for backporting to the same branch, I’ll roll whatever is needed into #26878. Not completely sure we need to backport either of those in either case.

fanquake approved

fanquake commented at 11:21 am on January 20, 2023: member

ACK e72313e6b3fbf865e0eaa9aee0a555b7a7fe6850

fanquake merged this on Jan 20, 2023

fanquake closed this on Jan 20, 2023

jonatack deleted the branch on Jan 20, 2023

fanquake renamed this:
~~[24.x] backport rpc: skip getpeerinfo for a peer without CNodeStateStats~~
[24.x] backport rpc: Require NodeStateStats object in getpeerinfo
on Mar 12, 2023

DrahtBot renamed this:
~~[24.x] backport rpc: Require NodeStateStats object in getpeerinfo~~
[24.x] backport rpc: Require NodeStateStats object in getpeerinfo
on Mar 12, 2023

fanquake referenced this in commit 9e05de1d70 on Mar 13, 2023

bitcoin locked this on Mar 11, 2024

[24.x] backport rpc: Require NodeStateStats object in getpeerinfo #26457

Reviews