p2p: always provide CNodeStateStats and getpeerinfo/netinfo/gui updates

jonatack commented at 7:10 pm on August 24, 2022: contributor

update CNodeStateStats members to be std::optional or member-initialized with default values
return CNodeStateStats and NodeStats directly rather than a bool
make some getpeerinfo and gui peers fields always provided rather than optional: addr_processed, addr_rate_limited, addr_relay_enabled, inflight
remove NodesStats type which causes more confusion than benefit

jonatack renamed this:
~~p2p, rpc: always provide CNodeStateStats and v23 getpeerinfo fields~~
p2p, rpc: always provide CNodeStateStats and v23 getpeerinfo API for v24
on Aug 24, 2022

jonatack force-pushed on Aug 24, 2022

jonatack renamed this:
~~p2p, rpc: always provide CNodeStateStats and v23 getpeerinfo API for v24~~
p2p, bugfix: always provide CNodeStateStats, ensure v23 getpeerinfo API for v24
on Aug 25, 2022

maflcko commented at 6:58 am on August 25, 2022: member

Can you explain why this is a “bugfix”, considering that the API break was done on purpose at that time?

vasild commented at 1:06 pm on August 25, 2022: contributor

Concept ACK @MarcoFalke, a bug is a bug, even if it was done on purpose. @jnewbery can clarify here, but I do not think this was even purposeful. My guess is that this breakage was unwanted, but acceptable in order to get the refactor from #21160 done. It later required fixup attempts like #24691 and #25176.

So, what happened? The relaytxes and minfeefilter fields were always present, and documented as such, in the output of the getpeerinfo RPC. #21160 changed it so that they could sometimes be missing. I guess the issues (both for GUI and RPC) were neglected during the review with a comment like will almost always be true, so this isn’t a big issue. Well, it turned out our own tool bitcoin-cli broke due to the missing but expected fields. Some user apps will break too (if we don’t fix it in 24.0).

#24691 documented the fields as optional, but that is not appropriate because in the previous version they were not-optional. When users wrote their apps they assumed the fields will always be present. I think the “optional” flag should only be added when a field is initially introduced. Adding it later is the same as dropping the field - it breaks the “contract” with the user that the field will be present.

On this PR: I think it is ok to display the values relaytxes=false and minfeefilter=0 in those corner cases. This is what the code did before #21160. If this is stripped to the bare minimum needed for the fix, I think it will have higher chance of making it to 24.0

jnewbery commented at 2:49 pm on August 25, 2022: contributor

So, what happened? The relaytxes and minfeefilter fields were always present, and documented as such, in the output of the getpeerinfo RPC.

This isn’t true. None of the fields in getpeerinfo were documented as always present until the ‘optional’ fields were explicitly documented in #22798. Prior to that PR, there was no explicit guarantee that any of the fields would be present.

PR 22798 was opened after #21160, then merged, and resulted in the silent conflict where the fields were not marked as optional. That could be caught by better testing of optional fields.

with a comment like “will almost always be true, so this isn’t a big issue”

Stop misrepresenting other people’s words. This was in response to a review comment about the GUI, not the RPC. You’ve also clipped the quote to misrepresent it further. Here’s the full sentence: “will almost always be true, so this isn’t a big issue (if it was, then the other stats in nodeStateStats not being available would also be an issue).” I stand by that - if it’s a problem that one of the fields is not visible in the GUI for a very short window during peer connection, then it’d be a problem that the other fields like “startingheight” or “minping” are not present. It’s clearly not a problem, since that’s always been the case.

At the time of the comment, it was also totally consistent with the other fields that were optional but not marked as such, since #22798 had not yet been opened.

in the previous version they were not-optional. When users wrote their apps they assumed the fields will always be present. I think the “optional” flag should only be added when a field is initially introduced. Adding it later is the same as dropping the field - it breaks the “contract” with the user that the field will be present.

Again, this is false. The ‘optional’ documentation was added in #22798, which was only included in v0.23. Until then, there were many fields could be considered optional but not documented as such. Users writing apps that interacted with the field (including the bitcoin-cli netinfo app) should have made those apps tolerant to the fields not being present.

As can be seen in #22798, there were 10s of fields across many different RPCs that were ‘optional’ before being explicitly documented as such. Both the PR description and @vasild’s comment make it sound like this was some nefarious intentional API break, or some egregious bug. It’s neither.

Frankly, it’s utterly tedious to have to spend time rebutting false statements and misrepresentations made about PRs that were merged months ago. Unfortunately, this ‘gotcha’ attitude of trying to shame code authors seems to be becoming more common in this project. It’s extremely harmful for productive collaboration, and is certainly not something I’d expect to see from someone putting themselves forward as a potential project maintainer.

jonatack commented at 4:15 pm on August 25, 2022: contributor

My goodness, that’s saddening to read. The only goal should be to improve Bitcoin. Let’s please remember to assume good intentions, focus on the code and improving Bitcoin and leave the other stuff behind. Thank you!

vasild commented at 4:17 pm on August 25, 2022: contributor

So, what happened? The relaytxes and minfeefilter fields were always present, and documented as such, in the output of the getpeerinfo RPC.

This isn’t true. None of the fields in getpeerinfo were documented as always present until the ‘optional’ fields were explicitly documented in #22798. Prior to that PR, there was no explicit guarantee that any of the fields would be present.

Alright, by “documented” I meant that since it is not documented as optional, then it must be mandatory, but I was not aware that optional fields were introduced later (#22798). Thanks for bringing it up. This changes the perspective here.

This was in response to a review comment about the GUI, not the RPC.

In the RPC case you referred to the GUI comment: As above for the gui peer console.

Anyway, I was just trying to clarify the situation here, not misinterpret, gotcha or shame. I am having coronavirus and should probably not be at the computer today at all, upsetting people. Will be more careful with my wording in the future.

Thank you!

ghost commented at 4:37 pm on August 25, 2022: none

Concept ACK

jonatack commented at 4:43 pm on August 25, 2022: contributor

(Am considering closing this draft and opening a new one. If there are any helpful, friendly suggestions in the interim to improve the change, that would be great ☺️)

DrahtBot commented at 5:25 am on August 26, 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
Concept ACK	ghost
Approach ACK	vasild
Stale ACK	brunoerg

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/505 (RPCConsole: add hidePeersDetailButton and connect to RPCConsole::clearSelectedNode by RandyMcMillan)
#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
#26516 (rpc: Always return getpeerinfo “relaytxes” field by jonatack)
#26515 (rpc: skip getpeerinfo for a peer without CNodeStateStats by mzumsande)
#25619 (net: avoid overriding non-virtual ToString() in CService and use better naming by vasild)
#24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)
#24008 (assumeutxo: net_processing changes by jamesob)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/bitcoin-cli.cpp:645 in 7ee6f76653 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+        "           \"*\" - the peer did not request that we relay transactions to it (relaytxes is false)\n"

ajtowns commented at 7:25 am on August 26, 2022:

I don’t think this change is more accurate. relaytxes is false only when either a peer explicitly requests tx relay to be disabled (via setting the fRelay flag added in BIP 37 to false), or when we decide that an outgoing connection should be block-relay-only, in which case we ignore any request from it to relay transactions. Saying “whether we will relay txs with this peer” might be more accurate though.

jonatack commented at 9:30 pm on August 30, 2022:

Thanks, “whether we relay transactions to this peer” SGTM.

jonatack commented at 10:48 am on August 31, 2022:

Updated both the getpeerinfo and the -netinfo help docs.

in src/bitcoin-cli.cpp:481 in 068cd21e92 outdated

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

ajtowns commented at 7:30 am on August 26, 2022:

bitcoin-cli could be being used with an older version of bitcoind, no? I think get_bool treats null as false, but I think getInt will throw. Probably not an issue if only unreleased versions of bitcoind treated these values as optional though.

maflcko commented at 7:53 am on August 26, 2022:

I don’t think we support arbitrary version combinations of bitcoin-cli and the rpc server? The only official supported combination is the one where both are built from the exact same commit hash.

jonatack commented at 9:27 pm on August 30, 2022:

Right, for the moment the newly frequently missing fields are not in a release.

get_bool and getInt both throw on null with error: JSON value of type null is not of expected type {bool|number}, which can be seen with a snippet like this added to a CLI or RPC.

0const UniValue x = UniValue();
1assert (x.isNull());
2// auto y = x.get_bool();        // uncomment to throw
3// auto z = x.getInt<int64_t>(); // uncomment to throw

jonatack commented at 10:05 am on September 1, 2022:

bitcoin-cli could be being used with an older version of bitcoind, no?

Circling back to this, as I have heard in the past from people using a recent -netinfo with nodes that have been up since an earlier release. Updated the minimum server check in -netinfo from v21 to v22, the last version that should be backward compatible (due to bitcoin core adding the addr_* fields in v22 used in -netinfo) and kept the null checks to maintain backward compatibility.

ajtowns commented at 7:47 am on August 26, 2022: contributor

I guess I’m not convinced either way about what the right behaviour is here – if a connection is still being setup, it seems more useful to indicate that via getpeerinfo or -netinfo than just treating it as if it will be like most connections are. On the other hand, that’s a bunch of effort for unusual special cases, so :shrug: …

maflcko commented at 8:40 am on August 26, 2022: member

While there are some fields that can obviously be made non-optional (such as the fee filter), because they can be absent on the wire even long after the connection has been spun up (thus require a default value anyway), there are other fields where I think it would be problematic to make them non-optional. For example, the service flags can not change after the handshake, so returning different service flags for the same peer id in different RPC invocations might cause issues in callers that assume they can’t change. I see that you didn’t change service flags in this pull, but the same should hold for txrelay, unless the user has bloom filters enabled.

So overall I don’t think this can be “sold” as a bugfix of some kind, but instead should be done by evaluating each field on a case-by-case basis whether it makes more sense to be optional or not. The behaviour change should then be motivated based on that rationale and not to restore some underdocumented/misdocumented interface of a past release.

in src/net_processing.cpp:1576 in 7ee6f76653 outdated

1492-    if (peer == nullptr) return false;
1493-    stats.their_services = peer->m_their_services;
1494-    stats.m_starting_height = peer->m_starting_height;
1495+    const PeerRef peer = GetPeerRef(nodeid);
1496+    if (peer == nullptr) {
1497+        return stats;

luke-jr commented at 11:21 pm on August 26, 2022:

Concept ACK to returning partial data rather than none at all.

in src/qt/rpcconsole.cpp:1196 in 7ee6f76653 outdated

1216-        ui->peerAddrProcessed->setText(QString::number(stats->nodeStateStats.m_addr_processed));
1217-        ui->peerAddrRateLimited->setText(QString::number(stats->nodeStateStats.m_addr_rate_limited));
1218-        ui->peerRelayTxes->setText(stats->nodeStateStats.m_relay_txs ? ts.yes : ts.no);
1219-    }
1220-
1221+    ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStateStats.m_services));

luke-jr commented at 11:22 pm on August 26, 2022:

Concept NACK to displaying incorrect data when data is not available.

jonatack commented at 9:22 pm on August 30, 2022:

Done for the GUI

0--- a/src/qt/guiutil.cpp
1+++ b/src/qt/guiutil.cpp
2@@ -748,7 +748,7 @@ QString formatServicesStr(quint64 mask)
3     if (strList.size())
4         return strList.join(", ");
5     else
6-        return QObject::tr("None");
7+        return QObject::tr("Unknown");
8 }

and left (for now) the RPC getpeerinfo behavior as it is currently, i.e. unchanged

0    "services": "0000000000000000",
1    "servicesnames": [
2    ],

vasild commented at 1:16 pm on September 6, 2022:

What about making stats->nodeStateStats.m_services std::optional and here do x.has_value() ? ... : "Unknown")? Otherwise we seem to be abusing a real value NODE_NONE (no services) as a dummy “we don’t know/has no value”.

jonatack commented at 11:10 am on September 12, 2022:

Done

luke-jr changes_requested

DrahtBot added the label Needs rebase on Aug 30, 2022

jonatack renamed this:
~~p2p, bugfix: always provide CNodeStateStats, ensure v23 getpeerinfo API for v24~~
p2p, refactor: always provide CNodeStateStats, maintain v23 getpeerinfo API for v24
on Aug 30, 2022

jonatack force-pushed on Aug 31, 2022

jonatack renamed this:
~~p2p, refactor: always provide CNodeStateStats, maintain v23 getpeerinfo API for v24~~
p2p: always provide CNodeStateStats, maintain v23 getpeerinfo API for v24
on Aug 31, 2022

DrahtBot removed the label Needs rebase on Aug 31, 2022

jonatack commented at 11:42 am on August 31, 2022: contributor

Rebased and updated to take review feedback by @ajtowns and @luke-jr and for the merge of #25717, maintaining the tristate presynced_headers behavior. The Clang-tidy CI errors are unrelated (https://github.com/bitcoin/bitcoin/pull/25963 should fix the Clang tidy one).

Thanks for the input, everyone. I’m agnostic to how we describe this change, as can be seen in the PR title and description history. I’ve re-updated both. I agree with evaluating each field on a case-by-case basis and it’s what I’ve tried to do.

History:

the getpeerinfo rpc was added in 2012 in 1006f0707e34f8903f247195dabd86243ae61f05 by Jeff Garzik
the relaytxes field was added to getpeerinfo in 2015 in 08843ed99843078 by Peter Todd
the minfeefilter field was added to getpeerinfo in 2018 in 5778bf95d94f4d1 by @ajtowns

Before adding the “optional” mentions to the RPC help docs, I believe we would often mention that fields were optional in the description, see 5ba829e12e99f11 for an example.

IIUC the only behavior change here where there may be varying opinions is to keep relaytxes always present.

My thinking is that this is the 8th year it has always been present, e.g. in a majority of our releases (~15) until now. That long-established behavior is a contract I would prefer to maintain, absent a compelling reason not to do so, in which case it might be good to warn our user space about the change in a release note.

It is true that both (a) the getpeerinfo fields order fixup and (b) keeping relaytxes and minfeefilter always present could be done separately for v24 with a small patch in src/rpc/net.cpp rather than in this larger change.

Regarding these last two points, I’ll leave it to reviewers to weigh in on which way to go.

jnewbery commented at 12:27 pm on August 31, 2022: contributor

Before adding the “optional” mentions to the RPC help docs, I believe we would often mention that fields were optional in the description, see https://github.com/bitcoin/bitcoin/commit/5ba829e12e99f119df56cab422f827b9be03fe57 for an example.

You’re linking to documentation that you added to a single RPC. By comparison look at #22798, which adds over 120 optional type arguments to existing rpcs. A large majority of “optional” type arguments were not documented as such until September 2021/v0.23. The optional type itself was not added until #17809 in 2020, many years after these return fields were included in the RPC (and not documented as optional).

That long-established behavior is a contract

No. If it’s not documented, it’s not a contract. Anyone writing clients using the RPC interface would have needed to make the clients accepting of fields being missing, since that’s always been possible for a large number of the results fields in a large number of the RPCs.

I agree with Marco that it makes sense for some fields to be non-optional, so some of the changes here some beneficial, but please stick to the facts in describing this change.

jonatack commented at 12:49 pm on August 31, 2022: contributor

Thanks @jnewbery. I’m agnostic to what word is used to describe the 8 years of relaytxes behavior. I think the question is whether reflecting that the peer’s behavior is still being set up is a sufficient reason to change it for v24. When in doubt I would maintain the long-standing behavior but ok to update depending on consensus.

DrahtBot added the label P2P on Aug 31, 2022

in src/rpc/net.cpp:117 in cdad951abf 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 peer has asked us to relay transactions to it"},
118+                    {RPCResult::Type::BOOL, "relaytxes", "Whether we relay transactions to this peer"},

maflcko commented at 1:25 pm on August 31, 2022:

in cdad951abf73650a9b49dd038ecbcc96ebf9bc0f: I think the doxygen for m_relay_txs should also be updated for the same reason. Otherwise it is not clear why you are changing this.

jonatack commented at 3:47 pm on August 31, 2022:

Good idea, done.

maflcko commented at 7:31 am on September 1, 2022:

done

No?

jonatack commented at 9:59 am on September 1, 2022:

Thanks, pushed

ajtowns commented at 3:12 pm on August 31, 2022: contributor

maintain getpeerinfo API stability by continuing to provide the relaytxes and minfeefilter fields that have always been present since 2015 and 2018, respectively, and do so with the same default values

FWIW, I think you’d be better off justifying this on its own terms regardless of recent/ancient historical behaviour.

I think relaytxes: false meaning “we currently won’t relay txs to/from this peer” is clear, even if that might change after version negotiation. I could see -1 or CAmount::max() being better defaults for minfeefilter if relaying isn’t happening though, since that way you could interpret it correctly without needing to also look at relaytxes…

jonatack renamed this:
~~p2p: always provide CNodeStateStats, maintain v23 getpeerinfo API for v24~~
p2p: always provide CNodeStateStats and getpeerinfo/netinfo/gui updates
on Aug 31, 2022

jonatack force-pushed on Sep 1, 2022

jonatack commented at 10:16 am on September 1, 2022: contributor

Rebased for the CI fixes and updated per @MarcoFalke and @ajtowns feedback (thanks).

FWIW, I think you’d be better off justifying this on its own terms regardless of recent/ancient historical behaviour.

Updated the pull title/description and the commit messages.

I think relaytxes: false meaning “we currently won’t relay txs to/from this peer” is clear, even if that might change after version negotiation. I could see -1 or CAmount::max() being better defaults for minfeefilter if relaying isn’t happening though, since that way you could interpret it correctly without needing to also look at relaytxes…

Makes sense, could see a -1 minfeefilter default for the reason you gave. Unsure with respect to API stability so deferring for a follow-up unless there is a preference that it be done here.

jonatack marked this as ready for review on Sep 1, 2022

in src/node/interfaces.cpp:168 in f9fb72e07e outdated

167@@ -168,29 +168,25 @@ class NodeImpl : public Node
168     bool getNodesStats(NodesStats& stats) override

brunoerg commented at 5:27 pm on September 1, 2022:

Couldn’t we change this function to return NodeStats instead of bool. Seems odd the first thing we do here is clear the stats.

jonatack commented at 6:19 pm on September 1, 2022:

Good observation! Done.

jonatack force-pushed on Sep 1, 2022

DrahtBot added the label Needs rebase on Sep 2, 2022

jonatack commented at 11:18 am on September 2, 2022: contributor

Rebased and ready for review.

jonatack force-pushed on Sep 2, 2022

DrahtBot removed the label Needs rebase on Sep 2, 2022

brunoerg approved

brunoerg commented at 8:10 pm on September 2, 2022: contributor

crACK 4b10c63dc2f5c894c07adc12926862783cda99b1

Code seems good for me and noticed my suggestion has been addressed in one of the latest pushes.

naumenkogs commented at 9:42 am on September 6, 2022: member

I will halt a thorough code review for now while it seems like there is a non-trivial high-level (“concept”) discussion of the subject. This probably also means we should not rush it into the immediate release. There is certainly a risk of breaking some software, but the risks of merging something poor now could be worse (the amount of code here is non-trivial too).

As for my personal high-level opinion, I’m not yet convinced following the former behavior (“bugfix”) is preferential (especially while not everyone argees it is a bugfix).

Please consider this my humble opinion as I haven’t reviewed the history of these changes as close as other devs in the PR.

in src/net_processing.cpp:1600 in 4b10c63dc2 outdated

1617     {
1618         LOCK(peer->m_headers_sync_mutex);
1619-        if (peer->m_headers_sync) {
1620-            stats.presync_height = peer->m_headers_sync->GetPresyncHeight();
1621-        }
1622+        stats.m_presync_height = peer->m_headers_sync ? peer->m_headers_sync->GetPresyncHeight() : -1;

vasild commented at 12:33 pm on September 6, 2022:

m_presync_height is std::optional but here we unconditionally assign a value to it and also use the dummy -1. Maybe revert this hunk?

jonatack commented at 8:43 am on September 12, 2022:

I believe this branch maintains the current behavior for this field (correct me if I’m wrong), as it was member-initialized to -1 in the CNodeStateStats struct but only returned in the RPC if the peer was set up.

in src/net_processing.cpp:1590 in 4b10c63dc2 outdated

1598     // the caller can immediately detect that this is happening.
1599-    auto ping_wait{0us};
1600     if ((0 != peer->m_ping_nonce_sent) && (0 != peer->m_ping_start.load().count())) {
1601-        ping_wait = GetTime<std::chrono::microseconds>() - peer->m_ping_start.load();
1602+        stats.m_ping_wait = GetTime<std::chrono::microseconds>() - peer->m_ping_start.load();
1603     }

vasild commented at 12:40 pm on September 6, 2022:

If we don’t assign to m_ping_wait here it would remain the default-constructed 0µs. Is it not better to change it to be std::optional and avoid the dummy default? That is, if we never sent a ping, the ping wait metric is meaningless. Also, the dummy value of 0 could in theory, I guess, overlap with a real value.

jonatack commented at 8:50 am on September 12, 2022:

SGTM, done

jonatack commented at 1:26 pm on September 14, 2022:

Actually, we are only interested in non-zero values for pingwait, so it would be simpler not to do this. What do you think?

jonatack commented at 3:49 pm on September 14, 2022:

Reverted this change in the latest push at b5255ca75f.

vasild commented at 1:25 pm on November 14, 2022:

Since the trend in this PR is to use std::optional it is odd to use the magic value 0 for m_ping_wait. Consider this change, which I think should be possible to be amended in bbd680d23e p2p, rpc, qt: always provide CNodeStateStats, update getpeerinfo and gui (not tested):

 0diff --git i/src/net_processing.h w/src/net_processing.h
 1index 2d7068f4c8..50fbe679a2 100644
 2--- i/src/net_processing.h
 3+++ w/src/net_processing.h
 4@@ -26,13 +26,13 @@ static const bool DEFAULT_PEERBLOCKFILTERS = false;
 5 static const int DISCOURAGEMENT_THRESHOLD{100};
 6
 7 struct CNodeStateStats {
 8     std::optional<int> m_sync_height;
 9     std::optional<int> m_common_height;
10     std::optional<int> m_starting_height;
11-    std::chrono::microseconds m_ping_wait{0us};
12+    std::optional<std::chrono::microseconds> m_ping_wait;
13     std::vector<int> m_height_in_flight;
14     std::optional<bool> m_relay_txs{false};
15     CAmount m_fee_filter_received{0};
16     uint64_t m_addr_processed = 0;
17     uint64_t m_addr_rate_limited = 0;
18     bool m_addr_relay_enabled{false};
19diff --git i/src/qt/guiutil.cpp w/src/qt/guiutil.cpp
20index 10e1ea169a..6562308052 100644
21--- i/src/qt/guiutil.cpp
22+++ w/src/qt/guiutil.cpp
23@@ -750,13 +750,13 @@ QString formatServicesStr(quint64 mask)
24     else
25         return QObject::tr("Unknown");
26 }
27
28 QString formatPingTime(std::chrono::microseconds ping_time)
29 {
30-    return (ping_time == std::chrono::microseconds::max() || ping_time == 0us) ?
31+    return ping_time == std::chrono::microseconds::max() ?
32         QObject::tr("N/A") :
33         QObject::tr("%1 ms").arg(QString::number((int)(count_microseconds(ping_time) / 1000), 10));
34 }
35
36 QString formatTimeOffset(int64_t nTimeOffset)
37 {
38diff --git i/src/qt/rpcconsole.cpp w/src/qt/rpcconsole.cpp
39index 49402b0264..088608397f 100644
40--- i/src/qt/rpcconsole.cpp
41+++ w/src/qt/rpcconsole.cpp
42@@ -1194,13 +1194,13 @@ void RPCConsole::updateDetailWidget()
43     }
44     ui->peerMappedAS->setText(stats->nodeStats.m_mapped_as != 0 ? QString::number(stats->nodeStats.m_mapped_as) : ts.na);
45     ui->peerServices->setText(stats->nodeStateStats.m_services.has_value() ? GUIUtil::formatServicesStr(stats->nodeStateStats.m_services.value()) : ts.unknown);
46     ui->peerSyncHeight->setText(stats->nodeStateStats.m_sync_height.has_value() ? QString("%1").arg(stats->nodeStateStats.m_sync_height.value()) : ts.unknown);
47     ui->peerCommonHeight->setText(stats->nodeStateStats.m_common_height.has_value() ? QString("%1").arg(stats->nodeStateStats.m_common_height.value()) : ts.unknown);
48     ui->peerHeight->setText(stats->nodeStateStats.m_starting_height.has_value() ? QString::number(stats->nodeStateStats.m_starting_height.value()) : ts.unknown);
49-    ui->peerPingWait->setText(GUIUtil::formatPingTime(stats->nodeStateStats.m_ping_wait));
50+    ui->peerPingWait->setText(stats->nodeStateStats.m_ping_wait.has_value() ? GUIUtil::formatPingTime(stats->nodeStateStats.m_ping_wait.value()) : QObject::tr("N/A"));
51     ui->peerAddrRelayEnabled->setText(stats->nodeStateStats.m_addr_relay_enabled ? ts.yes : ts.no);
52     ui->peerAddrProcessed->setText(QString::number(stats->nodeStateStats.m_addr_processed));
53     ui->peerAddrRateLimited->setText(QString::number(stats->nodeStateStats.m_addr_rate_limited));
54     ui->peerRelayTxes->setText(stats->nodeStateStats.m_relay_txs ? ts.yes : ts.no);
55     ui->peersTabRightPanel->show();
56 }
57diff --git i/src/rpc/net.cpp w/src/rpc/net.cpp
58index c688bd658f..bbfd88980d 100644
59--- i/src/rpc/net.cpp
60+++ w/src/rpc/net.cpp
61@@ -213,14 +213,14 @@ static RPCHelpMan getpeerinfo()
62         if (stats.m_last_ping_time > 0us) {
63             obj.pushKV("pingtime", Ticks<SecondsDouble>(stats.m_last_ping_time));
64         }
65         if (stats.m_min_ping_time < std::chrono::microseconds::max()) {
66             obj.pushKV("minping", Ticks<SecondsDouble>(stats.m_min_ping_time));
67         }
68-        if (statestats.m_ping_wait > 0s) {
69-            obj.pushKV("pingwait", Ticks<SecondsDouble>(statestats.m_ping_wait));
70+        if (statestats.m_ping_wait.has_value()) {
71+            obj.pushKV("pingwait", Ticks<SecondsDouble>(statestats.m_ping_wait.value()));
72         }
73         obj.pushKV("version", stats.nVersion);
74         // Use the sanitized form of subver here, to avoid tricksy remote peers from
75         // corrupting or modifying the JSON output by putting special characters in
76         // their ver message.
77         obj.pushKV("subver", stats.cleanSubVer);

in src/qt/guiutil.cpp:751 in 4b10c63dc2 outdated

747@@ -748,7 +748,7 @@ QString formatServicesStr(quint64 mask)
748     if (strList.size())
749         return strList.join(", ");
750     else
751-        return QObject::tr("None");
752+        return QObject::tr("Unknown");

vasild commented at 1:02 pm on September 6, 2022:

I am not sure about this change. If we are here, then mask is 0 aka ServiceFlags::NODE_NONE. Does not “None” describe it better?

jonatack commented at 11:12 am on September 12, 2022:

Done, back to “None” here and using “Unknown” in src/qt/rpcconsole.cpp per #25923 (review), and which continues to satisfy #25923 (review).

in src/net_processing.cpp:1559 in 4b10c63dc2 outdated

1559-        if (state == nullptr)
1560-            return false;
1561-        stats.nSyncHeight = state->pindexBestKnownBlock ? state->pindexBestKnownBlock->nHeight : -1;
1562-        stats.nCommonHeight = state->pindexLastCommonBlock ? state->pindexLastCommonBlock->nHeight : -1;
1563+        if (state == nullptr) {
1564+            return stats;

vasild commented at 1:39 pm on September 6, 2022:

Maybe this is ok, but somehow this feels wrong - in this case we have no stats at all. We return a CNodeStateStats object that has some fields std::optional without a value which is ok, but the other fields are not optional and have actual value, implying they are actual real stats, not dummies aka “no value/we don’t know”.

Maybe change all fields to std::optional or return std::optional<CNodeStateStats> from here? ~~Same in getNodeStats() if connman is missing.~~ Edit: then it would return empty vector which is ok.

jonatack commented at 9:34 am on September 14, 2022:

I’ve added an explanatory comment. The idea is to return sensible defaults where it makes sense and simplifies the code (both ours and that of API clients). For instance, the “addr_processed” and “addr_rate_limited” fields are member-initialized to 0 and may as well be returned during set up, as the peer has processed and limited no addresses yet. Likewise, returning an “inflight” field with an empty array seems just as logical and makes life simpler for us and for clients, than sometimes not returning the field in the RPC response. For “pingwait” we are only interested in non-zero values so a default of zero makes sense, and so on. Can update if reviewers disagree.

vasild commented at 1:41 pm on September 6, 2022: contributor

Posting review mid-way, I reviewed just the first commit 8c8773dc8744e58150fd09c145edaae80dd5ab4c.

I find it confusing to have NodesStats, CNodeStats, CNodeStateStats and CNodeCombinedStats.

Maybe changing

0using NodesStats = std::vector<std::pair<CNodeStats, CNodeStateStats>>;

to

0using CNodesCombinedStats = std::vector<CNodeCombinedStats>;

would alleviate it a bit?

jonatack force-pushed on Sep 14, 2022

jonatack commented at 9:42 am on September 14, 2022: contributor

Thank you for the review @vasild! I’ve hopefully addressed your feedback.

I find it confusing to have NodesStats, CNodeStats, CNodeStateStats and CNodeCombinedStats … would alleviate it a bit?

Removed NodesStats.

fanquake commented at 10:08 am on September 14, 2022: member

p2p_ping is now failing:

 0222/251 - p2p_ping.py failed, Duration: 1 s
 1stdout:
 22022-09-14T10:06:23.895000Z TestFramework (INFO): Initializing test directory /tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20220914_095106/p2p_ping_26
 32022-09-14T10:06:24.156000Z TestFramework (INFO): Check that ping is sent after connection is established
 42022-09-14T10:06:24.262000Z TestFramework (INFO): Reply without nonce cancels ping
 52022-09-14T10:06:24.315000Z TestFramework (ERROR): Assertion failed
 6Traceback (most recent call last):
 7  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 133, in main
 8    self.run_test()
 9  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_ping.py", line 67, in run_test
10    self.check_peer_info(pingtime=None, minping=None, pingwait=None)
11  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_ping.py", line 48, in check_peer_info
12    assert_equal(stats.pop('pingwait', None), pingwait)
13  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 56, in assert_equal
14    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
15AssertionError: not(0 == None)

jonatack force-pushed on Sep 14, 2022

jonatack commented at 11:24 am on September 14, 2022: contributor

Thank you for reviewing @brunoerg and @vasild! Diff since your reviews: git diff 4b10c63 b5255ca

p2p_ping is now failing

Indeed, fixed.

jonatack force-pushed on Sep 14, 2022

jonatack commented at 2:01 pm on September 16, 2022: contributor

If this is stripped to the bare minimum needed, I think it will have higher chance of making it to 24.0

Done in a separate PR without the refactoring here; the latter can indeed be done later.

in src/net_processing.cpp:1556 in b4bf7611d7 outdated

1555         const CNodeState* state = State(nodeid);
1556-        if (state == nullptr)
1557-            return false;
1558-        stats.nSyncHeight = state->pindexBestKnownBlock ? state->pindexBestKnownBlock->nHeight : -1;
1559-        stats.nCommonHeight = state->pindexLastCommonBlock ? state->pindexLastCommonBlock->nHeight : -1;
1560+        if (state == nullptr) {

naumenkogs commented at 7:11 am on September 20, 2022:

Maybe I’m missing the context, but when would we expect to trigger this? If this is called directly by user, why does it make sense to return defaults, rather than explicitly state the node is not found.

jonatack commented at 8:52 am on September 20, 2022:

Thanks for having a look. This can be seen e.g. in the GUI Peers tab details area when the next three blocks fields display “Unknown” during the peer setup phase.

The idea is to return sensible defaults where it makes sense and potentially simplifies the code (both ours and that of API clients). For instance, the “addr_processed” and “addr_rate_limited” fields are member-initialized to 0 and may as well be returned during set up, as the peer has processed and limited no addresses yet. Likewise, returning an “inflight” field with an empty array seems just as logical and makes life simpler for us and for clients, than sometimes not returning the field in the RPC response. For “pingwait” we are only interested in non-zero values so a default of zero makes sense. For “minfeefilter” we need a default, and so on.

Will update the documentation here as follows:

0         if (state == nullptr) {
1-            // Return member-initialized sensible defaults for the fields where
2-            // it makes sense and simplifies the code.
3+            // Peer is still being set up; return member-initialized sensible defaults
4+            // for the node state fields where it makes sense and simplifies the code.
5             return stats;

in src/net_processing.cpp:1578 in b4bf7611d7 outdated

1583+        // Peer is still being set up; return the stats that we do have.
1584+        return stats;
1585+    }
1586     stats.their_services = peer->m_their_services;
1587-    stats.m_starting_height = peer->m_starting_height;
1588+    if (peer->m_starting_height != -1) {

naumenkogs commented at 7:16 am on September 20, 2022:

This is handling the two ways our code looks at starting_height unknown or something, which we create in b4bf7611d718e361dc7dea849550b3728da79136. Why not having one way? Switch to a new default if you wish, but having two different ones seems suboptimal.

jonatack commented at 9:32 am on September 20, 2022:

I didn’t do that yet as it would extend the refactoring from CNodeStateStats into the Peer class to change m_starting_height from std::atomic to std::optional with a mutex, but might still do it once this PR becomes refactoring-only (the other changes have been unbundled).

jonatack commented at 5:44 pm on October 28, 2022:

I didn’t do that yet as it would extend the refactoring from CNodeStateStats into the Peer class to change m_starting_height from std::atomic to std::optional with a mutex, but might still do it once this PR becomes refactoring-only (the other changes have been unbundled).

Started doing this in latest (WIP) push.

in src/net_processing.cpp:1599 in b4bf7611d7 outdated

1615     {
1616         LOCK(peer->m_headers_sync_mutex);
1617-        if (peer->m_headers_sync) {
1618-            stats.presync_height = peer->m_headers_sync->GetPresyncHeight();
1619-        }
1620+        stats.presync_height = peer->m_headers_sync ? peer->m_headers_sync->GetPresyncHeight() : -1;

naumenkogs commented at 7:19 am on September 20, 2022:

in b4bf7611d718e361dc7dea849550b3728da79136

This looks like a legacy (w.r.t this PR) approach where we don’t rely on defaults. Why not?

naumenkogs commented at 8:15 am on September 27, 2022:

So the new behavior is the following:

nullptr if peer==nullptr
-1 if peer->m_headers_sync == nullptr
a real value otherwise

Is this supposed to be straightforward to the user?

jonatack commented at 10:44 am on September 30, 2022:

It’s not intended to be new behavior; will re-examine :)

in src/net_processing.h:34 in b4bf7611d7 outdated

35+    std::optional<int> m_starting_height;
36+    std::chrono::microseconds m_ping_wait{0us};
37     std::vector<int> vHeightInFlight;
38-    bool m_relay_txs;
39-    CAmount m_fee_filter_received;
40+    bool m_relay_txs{false};

naumenkogs commented at 8:28 am on September 27, 2022:

Why don’t we make this optional? Seeing the peer is not relaying txs (although it just hasn’t claimed it did) yet might be confusing.

naumenkogs commented at 8:38 am on September 27, 2022:

I know you adjust the definition of this flag in later commits. I kinda still think optional before we know for sure would be better.

jonatack commented at 10:44 am on September 30, 2022:

There seems to be agree on making it optional; will do.

jonatack commented at 5:43 pm on October 28, 2022:

Done in latest (WIP) push.

DrahtBot added the label Needs rebase on Oct 13, 2022

p2p, rpc, qt: always provide CNodeStateStats, update getpeerinfo and gui

- update CNodeStateStats members to be either std::optional, or member-initialized
  with default values where it makes sense and simplifies the code

- return CNodeStateStats rather than a bool

- in RPC getpeerinfo, continue to always provide the relaytxes and minfeefilter
  fields with default values of false and 0, respectively

- make getpeerinfo and gui peers fields always provided rather than optional where
  it makes sense: addr_processed, addr_rate_limited, addr_relay_enabled, inflight

bbd680d23e

interfaces, qt: always provide NodeStats 9cd3f53d5a

scripted-diff: rename CNodeStateStats members

-BEGIN VERIFY SCRIPT-
rename() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

rename nSyncHeight      m_sync_height
rename nCommonHeight    m_common_height
rename vHeightInFlight  m_height_in_flight
rename presync_height   m_presync_height
rename their_services   m_services
-END VERIFY SCRIPT-

d1c27a679b

refactor: remove NodesStats type

as it can only be used in a couple of instances rather than in all
of them, thereby providing little benefit and increasing confusion

143ec23bdc

p2p, refactor: make Peer#m_starting_height data member std::optional 25fa0d5a6c

jonatack force-pushed on Oct 28, 2022

jonatack commented at 5:47 pm on October 28, 2022: contributor

Rebased and WIP push (likely still need to add a mutex for m_starting_height), dropping the non-refactoring commits that have been merged in a separate pull.

DrahtBot removed the label Needs rebase on Oct 28, 2022

in src/net_processing.h:34 in 25fa0d5a6c

36+    std::optional<int> m_sync_height;
37+    std::optional<int> m_common_height;
38+    std::optional<int> m_starting_height;
39+    std::chrono::microseconds m_ping_wait{0us};
40+    std::vector<int> m_height_in_flight;
41+    std::optional<bool> m_relay_txs{false};

vasild commented at 1:31 pm on November 14, 2022:

This will initialize the m_relays_txs so that it has a value. Given that we never assign an empty optional to it, this means that calls to m_relay_txs.has_value() will always be true and thus we don’t need std::optional for it. Should it be something like this:

 0diff --git i/src/net_processing.h w/src/net_processing.h
 1index 2d7068f4c8..55ff27c546 100644
 2--- i/src/net_processing.h
 3+++ w/src/net_processing.h
 4@@ -28,13 +28,13 @@ static const int DISCOURAGEMENT_THRESHOLD{100};
 5 struct CNodeStateStats {
 6     std::optional<int> m_sync_height;
 7     std::optional<int> m_common_height;
 8     std::optional<int> m_starting_height;
 9     std::chrono::microseconds m_ping_wait{0us};
10     std::vector<int> m_height_in_flight;
11-    std::optional<bool> m_relay_txs{false};
12+    bool m_relay_txs{false};
13     CAmount m_fee_filter_received{0};
14     uint64_t m_addr_processed = 0;
15     uint64_t m_addr_rate_limited = 0;
16     bool m_addr_relay_enabled{false};
17     std::optional<ServiceFlags> m_services;
18     std::optional<int64_t> m_presync_height;
19diff --git i/src/qt/rpcconsole.cpp w/src/qt/rpcconsole.cpp
20index 49402b0264..270076a780 100644
21--- i/src/qt/rpcconsole.cpp
22+++ w/src/qt/rpcconsole.cpp
23@@ -1198,12 +1198,15 @@ void RPCConsole::updateDetailWidget()
24     ui->peerCommonHeight->setText(stats->nodeStateStats.m_common_height.has_value() ? QString("%1").arg(stats->nodeStateStats.m_common_height.value()) : ts.unknown);
25     ui->peerHeight->setText(stats->nodeStateStats.m_starting_height.has_value() ? QString::number(stats->nodeStateStats.m_starting_height.value()) : ts.unknown);
26     ui->peerPingWait->setText(GUIUtil::formatPingTime(stats->nodeStateStats.m_ping_wait));
27     ui->peerAddrRelayEnabled->setText(stats->nodeStateStats.m_addr_relay_enabled ? ts.yes : ts.no);
28     ui->peerAddrProcessed->setText(QString::number(stats->nodeStateStats.m_addr_processed));
29     ui->peerAddrRateLimited->setText(QString::number(stats->nodeStateStats.m_addr_rate_limited));
30+    // if m_relay_txs is an optional that always has value, then this is equivalent to
31+    // m_relay_txs.has_value() ? ts.yes : ts.no
32+    // which will _always_ print ts.yes
33     ui->peerRelayTxes->setText(stats->nodeStateStats.m_relay_txs ? ts.yes : ts.no);
34     ui->peersTabRightPanel->show();
35 }
36 
37 void RPCConsole::resizeEvent(QResizeEvent *event)
38 {
39diff --git i/src/rpc/net.cpp w/src/rpc/net.cpp
40index c688bd658f..9f76f5e41d 100644
41--- i/src/rpc/net.cpp
42+++ w/src/rpc/net.cpp
43@@ -196,15 +196,13 @@ static RPCHelpMan getpeerinfo()
44         if (stats.m_mapped_as != 0) {
45             obj.pushKV("mapped_as", uint64_t(stats.m_mapped_as));
46         }
47         const ServiceFlags services{statestats.m_services.has_value() ? statestats.m_services.value() : ServiceFlags::NODE_NONE};
48         obj.pushKV("services", strprintf("%016x", services));
49         obj.pushKV("servicesnames", GetServicesNames(services));
50-        if (statestats.m_relay_txs.has_value()) {
51-            obj.pushKV("relaytxes", statestats.m_relay_txs.value());
52-        }
53+        obj.pushKV("relaytxes", statestats.m_relay_txs);
54         obj.pushKV("lastsend", count_seconds(stats.m_last_send));
55         obj.pushKV("lastrecv", count_seconds(stats.m_last_recv));
56         obj.pushKV("last_transaction", count_seconds(stats.m_last_tx_time));
57         obj.pushKV("last_block", count_seconds(stats.m_last_block_time));
58         obj.pushKV("bytessent", stats.nSendBytes);
59         obj.pushKV("bytesrecv", stats.nRecvBytes);

jonatack commented at 11:04 pm on November 16, 2022:

Thanks @vasild! That was an oversight in my last WIP push, will update.

in src/net_processing.cpp:2873 in 25fa0d5a6c

2869@@ -2866,7 +2870,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
2870         // Headers message had its maximum size; the peer may have more headers.
2871         if (MaybeSendGetHeaders(pfrom, GetLocator(pindexLast), peer)) {
2872             LogPrint(BCLog::NET, "more getheaders (%d) to end to peer=%d (startheight:%d)\n",
2873-                    pindexLast->nHeight, pfrom.GetId(), peer.m_starting_height);
2874+                     pindexLast->nHeight, pfrom.GetId(), peer.m_starting_height.value());

vasild commented at 2:11 pm on November 14, 2022:

If m_starting_height has no value the call to value() will throw an exception. I do not feel comfortable either assuming that it will always have value here (also considering future changes) or that throwing an exception is ok. Some of this code is surrounded by try/catch higher up, but we may move pieces of code around, possibly moving this printout elsewhere. Maybe do it more defensively:

0LogPrint(BCLog::NET, "more getheaders (%d) to end to peer=%d%s\n",
1         pindexLast->nHeight,
2         pfrom.GetId(),
3         peer.m_starting_height.has_value() ? strprintf(" (startheight:%d)", peer.m_starting_height.value()) : "");

(here and in the other printouts)

vasild commented at 2:12 pm on November 14, 2022: contributor

Approach ACK 25fa0d5a6c11c79b3a2b60c4bba05a45f4c65d6a

Edit, just not to forget:

likely still need to add a mutex for m_starting_height

or std::atomic<std::optional<int>> m_starting_height

Also consider: #25923 (review)

in src/node/interfaces.cpp:182 in bbd680d23e outdated

192-                if (lockMain) {
193-                    for (auto& node_stats : stats) {
194-                        std::get<1>(node_stats) =
195-                            m_context->peerman->GetNodeStateStats(std::get<0>(node_stats).nodeid, std::get<2>(node_stats));
196-                    }
197+            TRY_LOCK(::cs_main, lockMain);

mzumsande commented at 11:54 pm on November 17, 2022:

Not finished with my review yet, but I think the changes to the GUI have a drawback: TRY_LOCK is used here in transmitting the info to the gui, meaning that we’ll abort (and now return a default-initialized CNodeStateStats) if cs_main is not available. As a result of this, when I run the GUI with this branch and open the Peers window the peer info sometimes changes to the default-initialized values (it’s as if the screen “flickers”), and with the next refresh it’s back to the old (and correct) values. If the node does something that’s locking-intensive like IBD, the flickering becomes a lot more frequent and annoying. I think it might be better not to update the GUI fields if we can’t get the lock, instead of returning a default-initialised object.

vasild commented at 7:48 am on November 18, 2022:

Is it not the same in master?

mzumsande commented at 9:44 am on November 18, 2022:

I think in master, we return fNodeStateStatsAvailable==false and don’t update the fields if we can’t get the lock (code) instead of updating them with default-values. fNodeStateStatsAvailable is being removed here.

jonatack commented at 4:12 pm on February 6, 2023:

Thanks, reproduced the issue you describe.

mzumsande commented at 6:36 pm on November 21, 2022: contributor

Some observations re: the RPC changes (I think the reporting to the GUI should be discussed separately, see my comment above):

As far as I can see, before this PR, the only situation in which CNodeStateStats was not available (fStateStats==false) is a race condition during peer disconnection, because getpeerinfo does not fetch CNodeStats and CNodeStateStats atomically - the node may have removed the peer in between the two calls. But this should not be possible during peer initialization because of the order in which the objects CNode, Peer and CNodeState are initialised.
I think that this behavior during disconnection is unintended, a bug that should be fixed. One way to fix it would be to not report an incomplete peer info in this case (#26515), another possibility might be to prevent the node from disconnecting a peer in between these two calls, i.e. change the way getpeerinfo fetches data - which might also fix the related issue that data from CNodeStateStats might be always present but change after fetching CNodeStats data, leading to an inconsistent response.
As a result of this, there currently seems to be no legitimate reason why we should fail to fetch an entire CNodeStateStats object when we have a CNodeStats object, or why fields from CNodeStateStats need to be any more optional than field from CNodeStats. During the lifetime of a connection, all three internal objects CNode, Peer and CNodeState should always be available.
It is an unrelated issue that some of the getpeerinfo fields (whichever internal object they may belong to) are not known during peer initialization because our peer hasn’t given us enough info yet - the old strategy has been to return default-initialized values, such as -1, but include the field. This PR makes many of these fields optional, by leaving them out of the getpeerinfo response - so it changes getpeerinfo behavior during startup. This should become very clear when this PR gets rebased wrt master and the newly added test in #26519 needs to be adjusted.
I don’t have a strong opinion on whether this is desirable (are there any general recommendation for this, or examples from other RPCs?), however note that some fields (e.g. presynced_headers=-1) are still left default-initialized, while others aren’t. I also think that if we make the fields truly optional, it might be good to mention in the RPC help in which situations they are not reported.

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

DrahtBot added the label Needs rebase on Dec 19, 2022

DrahtBot commented at 1:54 pm on December 19, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

sidhujag referenced this in commit bbc8a636d4 on Dec 19, 2022

fanquake commented at 3:04 pm on February 6, 2023: member

What is the status of this given recently merged changes, and the commentary above?

jonatack commented at 4:19 pm on February 6, 2023: contributor

I’m reviewing other pulls ATM but will evaluate the scope here, starting from simply updating the statestats struct with std::optionals on up to the other changes.

fanquake marked this as a draft on Feb 6, 2023

fanquake commented at 4:21 pm on February 6, 2023: member

I’m evaluating the scope,

Ok. Moved to draft for now, given it’s currently a work in progress. Feel free to undraft when you’ve decided on a scope, rebased, answered outstanding queries etc.

jonatack commented at 11:10 am on April 25, 2023: contributor

Closing temporarily so that I can re-open it – am still interested to finish this.

jonatack closed this on Apr 25, 2023

bitcoin locked this on Apr 24, 2024

p2p: always provide CNodeStateStats and getpeerinfo/netinfo/gui updates #25923

Reviews

Conflicts