[RPC] Add connection type to getpeerinfo, improve logs #19725
Conversation
4b6bc06 to
26baed4
Compare
|
This seems strangely implementation-specific to me. I think we should at least keep a simple connection direction somewhere? |
|
Concept ACK on adding the connection type to |
26baed4 to
c8a5626
Compare
|
thanks for the reviews! I've removed the "deprecate getpeerinfo inbound" commit & addressed all other review comments. |
c8a5626 to
49a5371
Compare
|
thanks for the review @laanwj. addressed all review comments |
src/net.h
Outdated
| @@ -644,6 +653,7 @@ class CNodeStats | |||
| // Bind address of our side of the connection | |||
| CAddress addrBind; | |||
| uint32_t m_mapped_as; | |||
| std::string m_connection_type; | |||
There was a problem hiding this comment.
Why string? What's the problem of ConnectionType m_connection_type and a std::string ConnectionTypeToString(ConnectionType) function? Couldn't find past discussion after a quick look.
There was a problem hiding this comment.
do I understand your suggestion correctly: update CNodeStats.m_connection_type to the raw ConnectionType, then in getpeerinfo implementation, pass through a function ConnectionTypeToString(m_connection_type)?
it's possible, what's the advantage?
the main downside I'm seeing is I'm trying to keep m_connection_type private, and need to access it from net_processing for the log commit. so having ConnectionTypeAsString just return the std::string is more convenient.
open to suggestions
There was a problem hiding this comment.
resolving this convo for now, feel free to reopen/comment if you'd like me consider an alternative approach :)
There was a problem hiding this comment.
I think string is fine, but I think the two variables are now a bit confusing. m_conn_type in CNode is a ConnectionType, and m_connection_type in CNodeStats is a string.
My suggestion is to either
- Switch this to a
ConnectionType, and maybe even use the same name; this unifies the two and no one has to ever wonder what the difference is between them_conn_typeand them_connection_type. - Rename this e.g. to
m_conn_string.
Feel free to ignore, though.
There was a problem hiding this comment.
I would agree that storing a string here is a bit strange. This is a persistent structure in memory, and every string causes an extra heap allocation. Which means an extra heap allocation per connected node. While it stores only one from a limited number of choices, so as I understand, it could just as well be an enum?
There was a problem hiding this comment.
I'm not sure this is a problem:
- in bitcoind, CNodeStats is just instantiated when needed, rather than kept persistently (it looks like the objects are cached in bitcoin-qt though)
- most compilers will use the short string optimization, and most of these strings are < 15 chars long, so won't result in a heap allocation
- there are already 3 other
std::stringmembers in this struct
There was a problem hiding this comment.
- definitely agree that
m_conn_stringis a better variable name for the current state, have incorporated into the latest push - RE memory management- I'm not fully understanding how this is a persistent data structure. I tried to trace the call sites and I'm not seeing where its got anything other than automatic storage duration. But also don't understand the gui caching. any pointers??
- regardless, I've taken a shot at implementing having
CNodeStatsstore aConnectionTypeinstead of a string. I haven't incorporated into this patch yet, but happy to if this solution looks reasonable / preferable: amitiuttarwar@93a56e2. One thing is @laanwj you expressed here a preference for moving the function out of the header file, which this implementation undoes.
I don't have any strong preferences, happy with whatever reviewers prefer.
49a5371 to
f7f3ef4
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
ACK f7f3ef4 (code review, light manual testing on a busy node) |
|
It may be prudent to not aggressively deprecate the addnode field in the same release as the conn_type field addition, as this provides no transition period; users will need to change code or config to avoid breakage.
Inbound isn't being deprecated IIUC. Mind updating the PR description? |
done, thanks!
I don't quite follow your logic. In this patch, users can support the old behavior with the configuration option For the case of the |
There is a slight difference in how urgent a user will need to update scripts. If there is one more version in-between the user can even upgrade without any
No strong opinion, just mentioning that there is a slight difference. |
|
okay fair, I hear the point about having a longer window of time during which to upgrade. that said, the minimum effort required to upgrade would still be the same. having a longer transition time makes sense for more complex features, but this is an extremely simple case and there's not much that would benefit from being manually tested. we can hardly even call it a new feature: what this means for the user is replacing any use of I'm still not seeing what advantages a user would gain from having a ~6 month window to make this simple switch. on the flip side I see a downside of the maintenance burden of the code. We would either have to add a TODO in the code or remember to update in next version. based on these factors, I believe we should begin deprecating the |
|
I think it's generally been considered that for users there is a difference. Historically, Bitcoin Core has been very non-aggressive about deprecations and time periods, and users are used to that, as far I've been able to understand and from what I've seen from deprecating a few fields and the pushback from proposing to deprecate a few others: a few releases, at minimum. Examples: RPC |
|
This is a very minor change to the RPC interface. I don't think we need any special treatment for deprecating |
src/net.h
Outdated
| @@ -644,6 +653,7 @@ class CNodeStats | |||
| // Bind address of our side of the connection | |||
| CAddress addrBind; | |||
| uint32_t m_mapped_as; | |||
| std::string m_connection_type; | |||
There was a problem hiding this comment.
I think string is fine, but I think the two variables are now a bit confusing. m_conn_type in CNode is a ConnectionType, and m_connection_type in CNodeStats is a string.
My suggestion is to either
- Switch this to a
ConnectionType, and maybe even use the same name; this unifies the two and no one has to ever wonder what the difference is between them_conn_typeand them_connection_type. - Rename this e.g. to
m_conn_string.
Feel free to ignore, though.
This field is now redundant since the connection type field will indicate MANUAL for addnode connections.
2499b12 to
a512925
Compare
|
many many thanks for the code reviews @jnewbery & @guggero! I'm very sorry but could I ask for a 3rd round? I had to rebase because of a very trivial conflict in the release notes 😬 but hopefully that means its easy to re-review. and thank you for your recent Concept ACKs @MarcoFalke & @practicalswift! if these changes are supported, I'd love to get them in relatively soon. the proposed patch has been pretty stable recently and it'd be great if I didn't have to ask for a 4th round of review because of rebases 😅 . Also, I'm working on enabling testing of additional connection types in #19315, and this patch would enable me to check the specific connection types (outbound full relay vs block relay only) rather than just checking they are not inbound connections (link). I standby my view that these changes are not in conflict with #19883- the |
|
Approach NACK. I prefer the approach in #19883 (comment). I certainly don't think we should rush through merges either whilst there is still discussion on an alternative approach happening (especially when the author of the alternative approach has ideas on where he'd like to take it downstream). I think the logging refactoring and deprecation are fine. edit: Perhaps we could hammer out this out in a Bitcoin Core PR review club session. I don't want to hold up progress but I also want to make sure we get this right and future plans aren't disrupted. |
|
Code review ACK a512925.
For goodness' sake. This PR has been open for over a month and has ACKs from:
There are also comments supporting a string representation of connection type in #19883:
To say that this PR is being rushed is a total mischaracterization. In fact, the converse is true - this PR has widespread support from the vast majority of regular contributors, and a huge amount of maintainer/contributor time have been wasted on a minor PR. To repeat what has been stated clearly many times before: there doesn't have to be a conflict between this and #19883 and each should be judged on its own merits. |
|
This is a conflict though right? What do you recommend? ACK this PR and then open a new PR changing the connection type to an int? There is a difference between throwing toys out of the pram because you don't get everything you want versus wanting to ensure a decision now doesn't disrupt future work. As far as I understand Concept ACKs don't cover this discussion whatsoever so including them seems misleading. |
There was a problem hiding this comment.
Code review ACK a512925.
Could already ditch m_manual_connection and update copyStats accordingly. Can be done in 0.22 when addnode is removed.
open a new PR changing the connection type to an int
IMO that's fine, I also support that, but for the RPC client this change is good enough.
As far as I understand Concept ACKs don't cover this discussion whatsoever so including them seems misleading.
Right, but it ins't an approach NACK so it counts a bit.
|
Concept ACK putting in human readable connection types in this RPC. I personally use Not a major concern, but I do think it's worth thinking a bit about how stable the API is here. For example, one thing I'm working on is a way to negotiate block-relay-only status on a connection, so that both the inbound and outbound side of the connection are aware of how the connection is being treated. That might mean that the strings we choose to use here could change as we try to improve the documentation to reflect new behavior in the code. So if users are expecting the RPC output to be a stable API, I'm not sure we're there yet. Perhaps that would be something for us to mention in the release notes the first time this appears? |
|
thank you for the reviews @jnewbery, @promag, @sdaftuar !
good idea. In the interest of preserving ACKs I've added amitiuttarwar@099e387 on another branch. I'll either incorporate here or can do in a follow-up.
can you please provide the rationale for your NACK? as I stated in #19725 (comment) (and others have also expressed)
if you disagree and think the two conflict, please provide the reasoning to help me understand the dissonance. thanks! |
|
I don't think this is a good argument for numbers:
I don't think this is true. It is the actual semantics of different connection types that change sometimes, and not just their names. For example, if a particular connection type that currently exists is split into two different types, the old type ceases to exist, and what its number in the enum refers to afterwards will generally be arbitrary. If the semantics change, we'd want the name to change as well, to provide a consistent interface.
If the meaning changes, why would the reported names not change? That even seems desirable. We can't promise to restrict the potential output in RPC of this to any particular set of strings (or numbers, for that matter). That would be like promising in the
Agree, but I don't think this is any worse or better for numbers vs names.
That's because we need a long-term way of referring to an idea. I don't think that's the case here, as the semantics of what types of connections exist is in flux, as you say. A correct analogy would be assigning numbers to the connection types (distinct from the internal enum), and any time the way the policy decisions around various connection types change, pick a new number and retire the old one. I don't think that's preferable to just understandable names that do the same thing. |
|
utACK a512925 |
|
Tested and code review ACK a512925. |
|
cr ACK a512925 🌇 Show signature and timestampSignature: Timestamp of file with hash |
| "inbound (initiated by the peer)", | ||
| "manual (added via addnode RPC or -addnode/-connect configuration options)", | ||
| "addr-fetch (short-lived automatic connection for soliciting addresses)", | ||
| "feeler (short-lived automatic connection for testing addresses)"}; |
There was a problem hiding this comment.
I think for "heavy objects" (non integral), we use extern const in the header and put them in the c++ file. This might reduce the size of the binary minimally.
So if you need to touch this code in the future again, you could apply the following diff (or similar):
diff --git a/src/net.cpp b/src/net.cpp
index 5b533d7d17..166fc8233d 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -488,6 +488,15 @@ void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNet
}
}
+extern const std::vector<std::string> CONNECTION_TYPE_DOC{
+ "outbound-full-relay (default automatic connections)",
+ "block-relay-only (does not relay transactions or addresses)",
+ "inbound (initiated by the peer)",
+ "manual (added via addnode RPC or -addnode/-connect configuration options)",
+ "addr-fetch (short-lived automatic connection for soliciting addresses)",
+ "feeler (short-lived automatic connection for testing addresses)",
+};
+
std::string CNode::ConnectionTypeAsString() const
{
switch (m_conn_type) {
diff --git a/src/net.h b/src/net.h
index 5a8e57b68b..3c6c45c161 100644
--- a/src/net.h
+++ b/src/net.h
@@ -114,13 +114,7 @@ struct CSerializedNetMsg
std::string m_type;
};
-const std::vector<std::string> CONNECTION_TYPE_DOC{
- "outbound-full-relay (default automatic connections)",
- "block-relay-only (does not relay transactions or addresses)",
- "inbound (initiated by the peer)",
- "manual (added via addnode RPC or -addnode/-connect configuration options)",
- "addr-fetch (short-lived automatic connection for soliciting addresses)",
- "feeler (short-lived automatic connection for testing addresses)"};
+extern const std::vector<std::string> CONNECTION_TYPE_DOC;
/** Different types of connections to a peer. This enum encapsulates the
* information we have available at the time of opening or accepting theThere was a problem hiding this comment.
Just tested locally, and the size does decrease by about 10kB:
--- a/tmp/old/d_size
+++ b/tmp/new/d_size
@@ -1 +1 @@
-10044320
+10027432There was a problem hiding this comment.
cool! I made a follow up to clarify expectations around stability in release notes, so I included this improvement too. didn't realize about this extern trick, thanks! #20090
…e logs a512925 [doc] Release notes (Amiti Uttarwar) 50f94b3 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar) df091b9 [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar) 395acfa [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar) 49c10a9 [log] Add connection type to log statement (Amiti Uttarwar) Pull request description: After bitcoin#19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field. This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string. Suggested by sdaftuar- bitcoin#19316 (comment) & bitcoin#19316 (comment) ACKs for top commit: jnewbery: Code review ACK a512925. sipa: utACK a512925 guggero: Tested and code review ACK a512925. MarcoFalke: cr ACK a512925 🌇 promag: Code review ACK a512925. Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e
… field 41dca08 [trivial] Extract connection type doc into file where it is used. (Amiti Uttarwar) 3069b56 [doc] Improve help for getpeerinfo connection_type field. (Amiti Uttarwar) Pull request description: two commits addressing small followups from #19725 * first commit adds a clarification in the release notes that this field shouldn't be expected to be stable (suggested by sdaftuar in #19725 (comment)) * second commit moves the `CONNECTION_TYPE_DOC` object out of the header file to reduce the size of the binary (suggested by MarcoFalke in #19725 (comment), he tested and found a decrease of 10kB) ACKs for top commit: achow101: ACK 41dca08 laanwj: Code review ACK 41dca08 Tree-SHA512: a555df978b4341fbe05deeb40a8a655f0d3c5c1c0adcc1737fd2cf61b204a5a24a301ca0c2b5a3616554d4abf8c57074d22dbda5a50d8450bc22c57679424985
…on type field 41dca08 [trivial] Extract connection type doc into file where it is used. (Amiti Uttarwar) 3069b56 [doc] Improve help for getpeerinfo connection_type field. (Amiti Uttarwar) Pull request description: two commits addressing small followups from bitcoin#19725 * first commit adds a clarification in the release notes that this field shouldn't be expected to be stable (suggested by sdaftuar in bitcoin#19725 (comment)) * second commit moves the `CONNECTION_TYPE_DOC` object out of the header file to reduce the size of the binary (suggested by MarcoFalke in bitcoin#19725 (comment), he tested and found a decrease of 10kB) ACKs for top commit: achow101: ACK 41dca08 laanwj: Code review ACK 41dca08 Tree-SHA512: a555df978b4341fbe05deeb40a8a655f0d3c5c1c0adcc1737fd2cf61b204a5a24a301ca0c2b5a3616554d4abf8c57074d22dbda5a50d8450bc22c57679424985
…tests 47ff509 [test] Clarify setup of node topology. (Amiti Uttarwar) 0672522 [move-only, test]: Match test order with run order (Amiti Uttarwar) Pull request description: small improvements to clarify logic in the functional tests 1. have test logic in `rpc_net.py` match run order of the test 2. remove `connect_nodes` calls that are redundant with the automatic test setup executed by the test framework Noticed when I was trying to debug a test for #19725. Small changes but imo very helpful, because they initially confused me. ACKs for top commit: laanwj: ACK 47ff509 Tree-SHA512: 2843da2c0b4f06b2600b3adb97900a62be7bb2228770abd67d86f2a65c58079af22c7c20957474a98c17da85f40a958a6f05cb8198aa0c56a58adc1c31100492
…tional tests 47ff509 [test] Clarify setup of node topology. (Amiti Uttarwar) 0672522 [move-only, test]: Match test order with run order (Amiti Uttarwar) Pull request description: small improvements to clarify logic in the functional tests 1. have test logic in `rpc_net.py` match run order of the test 2. remove `connect_nodes` calls that are redundant with the automatic test setup executed by the test framework Noticed when I was trying to debug a test for bitcoin#19725. Small changes but imo very helpful, because they initially confused me. ACKs for top commit: laanwj: ACK 47ff509 Tree-SHA512: 2843da2c0b4f06b2600b3adb97900a62be7bb2228770abd67d86f2a65c58079af22c7c20957474a98c17da85f40a958a6f05cb8198aa0c56a58adc1c31100492
Summary: In addition to adding more specificity to the log statement about the type of connection, this change also consolidates two statements into one. Previously, the second one should have never been hit, since block-relay connections would match the "!IsInboundConn()" condition and return early. This is a backport of [[bitcoin/bitcoin#19725 | core#19725]] [1/5] bitcoin/bitcoin@49c10a9 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10371
Summary: This is a backport of [[bitcoin/bitcoin#19725 | core#19725]] [2/5] bitcoin/bitcoin@395acfa Depends on D10371 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10372
After #19316, we can more directly expose information about the connection type on the
getpeerinfoRPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field.This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string.
Suggested by sdaftuar- #19316 (comment) & #19316 (comment)