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

jonatack · 2022-11-05T03:03:47Z

Backports #26515.

hebasto · 2022-11-05T10:54:50Z

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

luke-jr · 2022-11-05T15:37:35Z

src/bitcoin-cli.cpp

@@ -642,7 +642,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
        "  send     Time since last message sent to the peer, in seconds\n"
        "  recv     Time since last message received from the peer, in seconds\n"
        "  txn      Time since last novel transaction received from the peer and accepted into our mempool, in minutes\n"
-        "           \"*\" - the peer requested we not relay transactions to it (relaytxes is false)\n"
+        "           \"*\" - we do not relay transactions to this peer (relaytxes is false)\n"


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...

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

luke-jr · 2022-11-05T15:38:34Z

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

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 · 2022-11-16T08:33:45Z

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

lgtm

mzumsande

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 b54d8de of #26328 to report 3 states is not included here - also see comment below.

mzumsande · 2022-11-16T15:29:03Z

src/rpc/net.cpp

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


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.

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

DrahtBot · 2022-11-16T15:36:14Z

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 · 2022-11-17T04:18:05Z

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 · 2022-11-17T04:21:27Z

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.

7a53033 Fix Transaction Relay tooltip text in Peers details window (Jon Atack) Pull request description: as a value of N/A could occur due to a lock or a disconnection race but not during connection setup -- see bitcoin/bitcoin#26457 (review). Credit to Martin Zumsande for finding this. ACKs for top commit: jarolrod: ACK 7a53033 Tree-SHA512: 031779567e927f05f6fae02394a8c97ba5c45ba9fffd7f1e2c006e152df5f724d92a06f18a4c2540436476eca6b40a3a5cbc4421666cd576439b823668acfcfb

maflcko · 2022-11-17T09:28:57Z

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

jonatack · 2022-11-17T18:02:27Z

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.

…rs details window 7a53033 Fix Transaction Relay tooltip text in Peers details window (Jon Atack) Pull request description: as a value of N/A could occur due to a lock or a disconnection race but not during connection setup -- see bitcoin#26457 (review). Credit to Martin Zumsande for finding this. ACKs for top commit: jarolrod: ACK 7a53033 Tree-SHA512: 031779567e927f05f6fae02394a8c97ba5c45ba9fffd7f1e2c006e152df5f724d92a06f18a4c2540436476eca6b40a3a5cbc4421666cd576439b823668acfcfb

fanquake · 2022-12-05T10:53:25Z

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 (4b5f99e - #26457 (comment)), and to remove commits which are backporting changes that have since been reverted in master, i.e 0e4b959 (reverted in bitcoin-core/gui#681).

6fefd49 rpc: Require NodeStateStats object in getpeerinfo (Martin Zumsande) Pull request description: The objects `CNode`, `CNodeState` and `Peer` store different info about a peer - `InitializeNode()` and `FinalizeNode()` make sure that for the duration of a connection, we should always have one of each for a peer. Therefore, there is no situation in which, as part of getpeerinfo RPC, `GetNodeStateStats()` (which requires a `CNodeState` and a `Peer` entry for a `NodeId` to succeed) could fail for a legitimate reason while the peer is connected - this can only happen if there is a race condition between peer disconnection and the `getpeerinfo` processing (see also a more detailed description of this in #26457 (review)). But in this case I think it's better to just not include the newly disconnected peer in the response instead of returning just parts of its data. An earlier version of this PR also made the affected `CNodeStateStats` fields non-optional (see mzumsande@5f900e2). Since this conflicts with #25923 and should be a separate discussion, I removed that commit from this PR. ACKs for top commit: dergoegge: Approach ACK 6fefd49 MarcoFalke: review ACK 6fefd49 👒 Tree-SHA512: 89c8f7318df4634c1630415de9c8350e6dc2d14d9d07e039e5b180c51bfd3ee2ce99eeac4f9f858af7de846f7a6b48fcae96ebac08495b30e431a5d2d4660532

…ateStats 6fefd49 rpc: Require NodeStateStats object in getpeerinfo (Martin Zumsande) Pull request description: The objects `CNode`, `CNodeState` and `Peer` store different info about a peer - `InitializeNode()` and `FinalizeNode()` make sure that for the duration of a connection, we should always have one of each for a peer. Therefore, there is no situation in which, as part of getpeerinfo RPC, `GetNodeStateStats()` (which requires a `CNodeState` and a `Peer` entry for a `NodeId` to succeed) could fail for a legitimate reason while the peer is connected - this can only happen if there is a race condition between peer disconnection and the `getpeerinfo` processing (see also a more detailed description of this in bitcoin#26457 (review)). But in this case I think it's better to just not include the newly disconnected peer in the response instead of returning just parts of its data. An earlier version of this PR also made the affected `CNodeStateStats` fields non-optional (see mzumsande@5f900e2). Since this conflicts with bitcoin#25923 and should be a separate discussion, I removed that commit from this PR. ACKs for top commit: dergoegge: Approach ACK 6fefd49 MarcoFalke: review ACK 6fefd49 👒 Tree-SHA512: 89c8f7318df4634c1630415de9c8350e6dc2d14d9d07e039e5b180c51bfd3ee2ce99eeac4f9f858af7de846f7a6b48fcae96ebac08495b30e431a5d2d4660532

jonatack · 2023-01-10T21:50:38Z

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

fanquake · 2023-01-11T15:24:28Z

Concept ~0 on backporting 24cbea4 and 5c03df1 (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.

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

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

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 · 2023-01-11T16:54:04Z

Thanks, agree we should backport 6fefd49. ~~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.

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

fanquake · 2023-01-18T12:21:21Z

agree it's enough to only backport #26515.

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

cc @mzumsande

jonatack · 2023-01-19T19:10:34Z

Should this be undrafted

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

fanquake · 2023-01-20T11:20:59Z

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

ACK e72313e

932a609 doc: add initial release notes for v24.1 (fanquake) cc4e315 doc: update manual pages for v24.1rc1 (fanquake) 787affb doc: update version in bips.md to v24.1 (fanquake) 5077e02 build: bump version to v24.1rc1 (fanquake) Pull request description: Bump the version number to v24.1rc1. Regenerate the man pages. Update the version number in bips.md. Move the v24.0.1 release notes to doc/release-notes. Add initial release notes for v24.1. Merged changes to the 24.x branch since v24.0.1: - #26457 - #26735 - #26878 - #26880 ACKs for top commit: achow101: ACK 932a609 Tree-SHA512: b90fd7c8f22c8fb096864e47cb79eaf5878524739a3b5c1d495c8c196b70d08c7b95fbfb1dfcdddf507bd8a72a5d133ecbe6ae898bbe70931f404afd0807b707

DrahtBot added the Backport label Nov 5, 2022

luke-jr reviewed Nov 5, 2022

View reviewed changes

luke-jr mentioned this pull request Nov 15, 2022

[24.x] bump version to v24.0 #26503

Merged

mzumsande reviewed Nov 15, 2022

View reviewed changes

maflcko approved these changes Nov 16, 2022

View reviewed changes

mzumsande reviewed Nov 16, 2022

View reviewed changes

aureleoules mentioned this pull request Nov 16, 2022

Webhook server and framework for bot features with ACK tracker maflcko/DrahtBot#1

Merged

This was referenced Nov 16, 2022

doc: fix -netinfo relaytxes help #26328

Closed

rpc: skip getpeerinfo for a peer without CNodeStateStats #26515

Closed

jonatack mentioned this pull request Nov 17, 2022

rpc: Always return getpeerinfo "relaytxes" field #26516

Closed

jonatack mentioned this pull request Nov 17, 2022

Fix Transaction Relay tooltip text in Peers details window bitcoin-core/gui#681

Merged

bitcoin deleted a comment from DrahtBot Nov 17, 2022

maflcko modified the milestone: 24.1 Nov 17, 2022

jonatack marked this pull request as draft January 10, 2023 21:40

jonatack force-pushed the 24_x_backports branch from 0e4b959 to ba28fa7 Compare January 10, 2023 21:41

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

jonatack force-pushed the 24_x_backports branch from ba28fa7 to bd7b8ef Compare January 11, 2023 17:15

jonatack force-pushed the 24_x_backports branch from bd7b8ef to e72313e Compare January 11, 2023 17:18

maflcko added this to the 24.1 milestone Jan 19, 2023

jonatack marked this pull request as ready for review January 19, 2023 19:06

fanquake approved these changes Jan 20, 2023

View reviewed changes

fanquake merged commit 2b87e90 into bitcoin:24.x Jan 20, 2023

jonatack deleted the 24_x_backports branch January 20, 2023 13:23

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

DrahtBot changed the title ~~[24.x] backport rpc: Require NodeStateStats object in getpeerinfo~~ [24.x] backport rpc: Require NodeStateStats object in getpeerinfo Mar 12, 2023

fanquake mentioned this pull request Mar 12, 2023

[24.x] Bump version to v24.1rc1 #27247

Merged

bitcoin locked and limited conversation to collaborators Mar 11, 2024

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

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

Uh oh!

Conversation

jonatack commented Nov 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Nov 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr Nov 5, 2022

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 5, 2022

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Nov 5, 2022

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 16, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

jonatack commented Nov 17, 2022

Uh oh!

jonatack commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 17, 2022

Uh oh!

jonatack commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Dec 5, 2022

Uh oh!

jonatack commented Jan 10, 2023

Uh oh!

fanquake commented Jan 11, 2023

Uh oh!

jonatack commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jan 18, 2023

Uh oh!

jonatack commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jan 20, 2023

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Nov 5, 2022 •

edited

Loading

hebasto commented Nov 5, 2022 •

edited

Loading

jonatack Nov 17, 2022 •

edited

Loading

DrahtBot commented Nov 16, 2022 •

edited

Loading

jonatack commented Nov 17, 2022 •

edited

Loading

jonatack commented Nov 17, 2022 •

edited

Loading

jonatack commented Jan 11, 2023 •

edited

Loading

jonatack commented Jan 19, 2023 •

edited

Loading