After #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- #19316 (review) & #19316 (review)
114+ the type of connection established with the peer. It will return one of six
115+ options: 1. outbound full relay (default automatic connections) 2. block
116+ relay only (connections that only relay blocks, not transactions or
117+ addresses) 3. inbound (connection initiated by the peer) 4. manual (peers
118+ added via addnode or -connect configuration param) 5. feeler / 6. addrfetch
119+ (both short lived automatic connections used for robustness).
help rpc_method or --help if needed.
874@@ -874,6 +875,25 @@ class CNode
875 assert(false);
876 }
877
878+ std::string ConnectionTypeToString() const {
884+ case ConnectionType::FEELER:
885+ return "feeler";
886+ case ConnectionType::OUTBOUND:
887+ return "outbound full relay";
888+ case ConnectionType::BLOCK_RELAY:
889+ return "block relay only";
- to preserve the previous format of the log message? Also, if this was used in an arbitrary sentence, it might not be clear that this is a single word/“operator”
133@@ -133,6 +134,10 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
134 "When a message type is not listed in this json object, the bytes received are 0.\n"
135 "Only known message types can appear as keys in the object and all bytes received of unknown message types are listed under '"+NET_MESSAGE_COMMAND_OTHER+"'."}
136 }},
137+ {RPCResult::Type::STR, "connection_type", "Type of connection: outbound full relay (default automatic connections),\n"
0 {RPCResult::Type::STR, "connection_type", "Type of connection:\n " + Join(foobar, ",\n") + "."},
What about moving the connection type documentation into a global like NET_PERMISSIONS_DOC?
101@@ -102,6 +102,22 @@ will trigger BIP 125 (replace-by-fee) opt-in. (#11413)
102 option `-deprecatedrpc=banscore` is used. The `banscore` field will be fully
103 removed in the next major release. (#19469)
104
105+- The `getpeerinfo` RPC no longer returns the `inbound` or `addnode` fields by
106+ default. These two fields will be fully removed in the next major release.
107+ They can be accessed with the configuration options
108+ `-deprecatedrpc=getpeerinfo_inbound` and `-deprecatedrpc=getpeerinfo_addnode`
109+ respectively. However, it is recommended to instead use the `connection_type`
110+ field, which will indicate whether the peer is inbound, addnode(manual), or
0 field.
Probably can remove the extended documentation
101@@ -102,6 +102,22 @@ will trigger BIP 125 (replace-by-fee) opt-in. (#11413)
102 option `-deprecatedrpc=banscore` is used. The `banscore` field will be fully
103 removed in the next major release. (#19469)
104
105+- The `getpeerinfo` RPC no longer returns the `inbound` or `addnode` fields by
addnode (as is in master) misleading because it also returns -connect peers. But if having this information separately is desirable, I’m happy to remove that commit as well. I just don’t want the default to be the RPC bloating over time with redundant information.
makes two of the existing fields (inbound & addnode booleans) redundant
sounds good! I removed the “deprecate inbound” commit. I currently have left in the “deprecate getpeerinfo addnode” commit, but can also remove that one if reviewers would like. I do find the current way of calling it
addnode(as is in master) misleading because it also returns-connectpeers. But if having this information separately is desirable, I’m happy to remove that commit as well.
If addnode has different behavior, which users may depend on, is it redundant?
(Edit: I apologize if I seem keen on addnode; I use both the rpc/cli/config option and the getpeerinfo field frequently)
it’s not different behavior. its the same.
getpeerinfo field addnode returns a bool that indicates if the connection was manually added, either via addnode RPC or -connect command line argument.
with this PR, getpeerinfo field connection_type will return manual for these connections.
I’m saying that # 2 is more clear and accurate. I think your confusion around possible difference of behavior supports my point that it is unclear :)
addnode and conn_type manual are the same.
getpeerinfo and the logging.
883@@ -874,6 +884,26 @@ class CNode
884 assert(false);
885 }
886
887+ std::string ConnectionTypeToString() const
I’m not sure it’s better to have ConnectionTypeToString as a method on CNode instead of a free function that simply takes a ConnectionType.
But if so, ConnectionTypeAsString might be a better name.
Edit: Also, I’d prefer to move this function to the implementation file instead of the .h. Compact header files are better for readability, and there’s imo no performance reason to have this inlinable.
hm, I got a lot of review on #19316 expressing concerns around publicly exposing m_conn_type, which led to the current design of wrapper classes. since I need the return value in net_processing, I’ve opted to keep the function on CNode & updated the name to better reflect behavior, as per your suggestion. happy to reconsider if you still feel it would be better as a free function.
also moved into net.cpp. I’m still trying to understand when inlining is worth it. so far my heuristic has mainly been “is this function computationally very simple”. but I hear your point of header files being readable. I’m interested in learning more if you have any other heuristics you consider / resources to share.
897+ return "outbound-full-relay";
898+ case ConnectionType::BLOCK_RELAY:
899+ return "block-relay-only";
900+ case ConnectionType::ADDR_FETCH:
901+ return "addr-fetch";
902+ }
// no default case, so the compiler can warn about missing cases comment (https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures)
101@@ -102,6 +102,15 @@ will trigger BIP 125 (replace-by-fee) opt-in. (#11413)
102 option `-deprecatedrpc=banscore` is used. The `banscore` field will be fully
103 removed in the next major release. (#19469)
104
105+- The `getpeerinfo` RPC no longer returns the `addnode` field by default. This
106+ field will be fully removed in the next major release. It can be accessed
107+ with the configuration option `-deprecatedrpc=getpeerinfo_addnode`. However,
108+ it is recommended to instead use the `connection_type` field.
109+
110+- The `getpeerinfo` RPC now returns a `connection_type` field. This indicates
I’d put this first.
Side note, here and above, could add PR number? It’s a quick way to get context from the release notes.
101@@ -102,6 +102,15 @@ will trigger BIP 125 (replace-by-fee) opt-in. (#11413)
102 option `-deprecatedrpc=banscore` is used. The `banscore` field will be fully
103 removed in the next major release. (#19469)
104
105+- The `getpeerinfo` RPC no longer returns the `addnode` field by default. This
106+ field will be fully removed in the next major release. It can be accessed
107+ with the configuration option `-deprecatedrpc=getpeerinfo_addnode`. However,
108+ it is recommended to instead use the `connection_type` field.
652@@ -644,6 +653,7 @@ class CNodeStats
653 // Bind address of our side of the connection
654 CAddress addrBind;
655 uint32_t m_mapped_as;
656+ std::string m_connection_type;
ConnectionType m_connection_type and a std::string ConnectionTypeToString(ConnectionType) function? Couldn’t find past discussion after a quick look.
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
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
ConnectionType, and maybe even use the same name; this unifies the two and no one has to ever wonder what the difference is between the m_conn_type and the m_connection_type.m_conn_string.Feel free to ignore, though.
I’m not sure this is a problem:
std::string members in this struct
m_conn_string is a better variable name for the current state, have incorporated into the latest pushCNodeStats store a ConnectionType instead of a string. I haven’t incorporated into this patch yet, but happy to if this solution looks reasonable / preferable: https://github.com/amitiuttarwar/bitcoin/commit/93a56e2880ee2a1513447c1c8807ebfa70be9270. 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.
113@@ -114,6 +114,15 @@ struct CSerializedNetMsg
114 std::string m_type;
115 };
116
117+const std::vector<std::string> CONNECTION_TYPE_DOC{
I believe you can’t in general.
But you can add a “MAX” element to the enum, which is the length.
If you make the list an std::array instead of std::vector, you can make it a static_assert with that even.
@sipa what would you static assert on? When you declare the std_array type, you need to specify the length of the array, and the array is always going to be that size. eg this compiles:
0enum class en {
1 thing1,
2 thing2,
3 thing3, // added a new thing
4 MAX,
5};
6
7int main()
8{
9 std::array<std::string, (int)en::MAX> a {"thing1 description", "thing2 description"}; // oops, didn't add a thing3 description. a[2] is default initialized to empty string
10 static_assert(a.size() == (int)en::MAX); // always passes
11 for (const auto& x : a) {
12 std::cout << x << std::endl;
13 }
14}
Would you replace the value in the static_assert with a hard-coded value?
I’m unclear on what’s being recommended here.
Should I update to make CONNECTION_TYPE_DOC an array, allocate size based on MAX from enum, then hardcode the MAX value into the static assert to compare it to the size of array?
Just in theory it would be possible to get the number of enum values via the preprocessor:
0cat src/util/enum_class.h
1// Copyright (c) 2020 The Bitcoin Core developers
2// Distributed under the MIT software license, see the accompanying
3// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4
5#ifndef BITCOIN_UTIL_ENUM_CLASS_H
6#define BITCOIN_UTIL_ENUM_CLASS_H
7
8#include <boost/preprocessor/variadic/size.hpp>
9
10#define ENUM_CLASS(name, ...) \
11 enum class name { __VA_ARGS__ }; \
12 constexpr size_t name##_##SIZE { BOOST_PP_VARIADIC_SIZE(__VA_ARGS__) }
13
14#endif // BITCOIN_UTIL_ENUM_CLASS_H
This could be used to write
0static_assert(CONNECTION_TYPE_DOC.size() == ConnectionType_SIZE, "inconsistent docs");
Probably not worth the effort. Just saying that it is possible.
MAX value breaks all the (important) “value not handled in switch statement” warnings. I don’t think this is worth it. Sorry for the noise!
also meh
I agree!
Tested ACK f7f3ef4f099d1f44b25c255baa0d25de448f3bdf
One suggestion inline.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
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.
Doing so also makes two of the existing fields (inbound & addnode booleans) redundant, so this PR begins deprecating those fields as well.
Inbound isn’t being deprecated IIUC. Mind updating the PR description?
Inbound isn’t being deprecated IIUC. Mind updating the PR description?
done, thanks!
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.
I don’t quite follow your logic. In this patch, users can support the old behavior with the configuration option -deprecatedrpc=getpeerinfo_addnode, as with any other time we deprecate a field. What is the difference between doing it in this version versus a future version?
For the case of the inbound field, reviewers made a case that having the isolated boolean of connection direction is useful, so I removed the deprecation commit and left the field. I’m open to leaving the addnode field if there is a good reason. I believe it makes sense to remove because getpeerinfo is already huge, and I don’t want to bloat it with redundant information unless we have a specific reason to do so.
What is the difference between doing it in this version versus a future version?
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 -deprecatedrpc hassle.
-deprecatedrpc or upgrade their script to use the new feature right away.)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:
existing master: getpeerinfo#addnode returns a boolean based on stats.m_manual_connection which checks m_conn_type == ConnectionType::MANUAL
proposed patch: getpeerinfo#connection_type returns manual if m_conn_type is ConnectionType::MANUAL based on a switch statement.
what this means for the user is replacing any use of getpeerinfo.addnode with getpeerinfo.connection_type == "manual".
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 getpeerinfo#addnode field with this PR unless we have a reason & want to leave it in for the long run (as with the inbound field).
getunconfirmedbalance (superceded a few releases ago by getbalances yet still not deprecated (and per reviewers, there is no rush to do so), or the getwalletinfo balance fields (idem). Stability for users seems to be generally seen as more important than the maintenance for us–which really might mean just making a draft PR with the relevant commit and tagging it for 0.22/0.23.
addnode. Lets just go through the normal deprecation process in this version and remove in the next.
101@@ -102,6 +102,17 @@ will trigger BIP 125 (replace-by-fee) opt-in. (#11413)
102 option `-deprecatedrpc=banscore` is used. The `banscore` field will be fully
103 removed in the next major release. (#19469)
104
105+- The `getpeerinfo` RPC now returns a `connection_type` field. This indicates
106+ the type of connection established with the peer. It will return one of six
107+ options. For more information, see the `getpeerinfo` help documentation.
Only reading the release notes you are left wanting, I suspect; explicitly saying there’s six of them makes you start to wonder “so many? I can think of maybe 3 at most… curious!”.
Half-joke aside, simply “one of six options: a, b, c, d, e, or f” would be useful, but no strong opinion on the matter.
486@@ -487,6 +487,26 @@ void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNet
487 }
488 }
489
490+std::string CNode::ConnectionTypeAsString() const
As feels redundant. (I.e. ConnectionTypeString())
net_processing.cpp, so it wouldn’t make sense to move to the RPC code.
tACK
Tested f7f3ef4f099d1f44b25c255baa0d25de448f3bdf locally on regtest.
Was able to verify that the connection types inbound, manual, addr-fetch work as expected.
The -deprecatedrpc=getpeerinfo_addnode flag also has the desired effect of adding the deprecated addnode back to the output of getpeerinfo.
Thank you for these reviews! 🙌🏽
I had to rebase and I took the opportunity to change the variable name from CNodeStats.m_connection_type -> CNodeStats.m_conn_type_string
Current status of this PR:
113@@ -114,6 +114,15 @@ struct CSerializedNetMsg
114 std::string m_type;
115 };
116
117+const std::vector<std::string> CONNECTION_TYPE_DOC{
118+ "outbound-full-relay (default automatic connections)",
119+ "block-relay-only (does not relay transactions or addresses)",
120+ "inbound (initiated by the peer)",
121+ "manual (added via addnode RPC or -connect command line argument)",
addnode is also a configuration option
s/command line argument/configuration option/
118+ "outbound-full-relay (default automatic connections)",
119+ "block-relay-only (does not relay transactions or addresses)",
120+ "inbound (initiated by the peer)",
121+ "manual (added via addnode RPC or -connect command line argument)",
122+ "addr-fetch (short lived automatic connection for soliciting addresses)",
123+ "feeler (short lived automatic connection for testing addresses)"
232@@ -228,6 +233,7 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
233 recvPerMsgCmd.pushKV(i.first, i.second);
234 }
235 obj.pushKV("bytesrecv_per_msg", recvPerMsgCmd);
236+ obj.pushKV("connection_type", stats.m_conn_type_string);
addnode.
agreed on placing after addnode, updated in latest push. thanks!
having an integer value seems like a reasonable proposal, I’ll leave it for review via 19883.
29+ connect_nodes(self.nodes[0], 1)
30+
31+ self.log.info("Test getpeerinfo by default no longer returns an addnode field")
32+ assert "addnode" not in self.nodes[0].getpeerinfo()[0].keys()
33+
34+ self.log.info("Test getpeerinfo returns inbound with -deprecatedrpc=addnode")
getpeerinfo(). I personally wouldn’t use it but I could see how it’d be useful for a client application. No reason not to return both I think.
jonatack - how do you feel about having a string and a type number for connection type returned by
getpeerinfo(). I personally wouldn’t use it but I could see how it’d be useful for a client application. No reason not to return both I think.
Indeed, the first version of #19883 proposed both an integer id and the long-format string id, before removing the string after the first round of review to take advantage of the improvement that brings in performance, maintenance, code, complexity, memory, bandwidth, etc.
I’ve been dogfooding the conn type for the -netinfo dashboard and prefer to parse integer ids for the reasons described in #19883 (comment).
Client-side, you can bounds check an integer and then use it to call a data structure element. No need to match against each of the possible long format string values to error check it.
okay @jonatack, I think you have made your stance sufficiently clear :)
for other reviewers- I believe these proposed changes make sense. The logging & RPC exposure were proposed by @sdaftuar during review of 19316 & I agree with them. As a node operator, I would like to be able to easily see information about the connection types to peers, through the RPC interface and logs. As a code contributor I like cleaning up along the way so our codebase complexity doesn’t just grow over time.
the PR is in a ready-for-review state. tip currently has 1 utACK from @jnewbery (thanks!)
several other reviewers have expressed support of these changes from different levels of ACKs, am curious to hear if they still think the patch makes sense.
thanks!
tACK 2499b120.
I personally also prefer this PR over #19883.
Numeric types still requires your code to do a lookup of some sort to present a human-readable value. If the meaning of a numeric value changes, you cannot catch this easily. Assuming that a string based enum value would be renamed if its meaning changed, a user of the API would notice that with a higher probability IMO.
When considering replacing integer ids with long-format string ids for API clients, remember that the long-format naming is:
Consider why we use integers for BIP numbers. Once one is assigned, it can be stable, for it is decoupled from the changing naming and fashion of the day.
I wouldn’t be surprised if within a year we refer to conn_types by number.
The same observation goes for enum Network / CNetAddr::GetNetClass, another enum we’ll need to have getpeerinfo expose in order to reliably know whether a peer is ipv4, ipv6, onion, i2p, cjdns, etc.
I suggest that we may as well begin with integer ids from the start and be done with it.
your code to do a lookup of some sort to present a human-readable value
Yes, that is what client-side software does: presentation. I suggest that there’s no need to burden the net code with client-side presentation concerns that can go in /rpc or be left to the API clients that consume the API.
I don’t disagree with your arguments. Naming is hard and choosing the same representation for the same thing everywhere is not always easy.
Though as someone that’s almost exclusively using the command line, I like getting a (rough) idea what a value means without needing a lookup table. But likely I’m not the target user group this API was designed for so maybe my opinion is a bit skewed and should be weighted accordingly.
I however would not object to adding a numeric type in addition to the string value.
Concept ACK on returning a meaningful but terse string representation (inbound, manual, feeler, etc.) instead of leaking implementation details (0, 1, 2, etc.). (What happens when the enum values are re-arranged?)
Empirical study: I’ve used the -netinfo feature a lot recently but the magic numbers are driving me crazy. I’ve lost count on how many times I’ve asked myself: “was it bitcoin-cli -netinfo 3 or bitcoin-cli -netinfo 4 that corresponds to the layout I like?” :)
I don’t know of any precedent of that style in Unix land TBH.
I’d personally feel more comfortable if more people weighed in on Jon’s comment in the alternative PR.
By any objective technical criteria, ersatz long-format string ids in the place of integer ones seem a substantially worse choice
code complexity robustness API stability API flexibility for clients memory speed network bandwidth maintainability in every way maybe an order of magnitude worse.
JSON-RPC expects integer ids.
An API client can bounds check an integer id, then call a vector element with it.
With a long format string, the client has to match against every possible expected value first in order to error check the value.
I have no strong opposition to bundling changes into a PR but I would like to check whether people (especially experienced reviewers) agree, disagree, or are indifferent to Jon’s above comment.
Concept ACK on returning a meaningful but terse string representation (
inbound, manual, feeler, etc.) instead of leaking implementation details (0, 1, 2, etc.). (What happens when the enum values are re-arranged?)
Thanks for clarifying regarding re-arrangement.
Do you know of any precedent of this style in Unix land, or in Bitcoin Core?
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.
Simple rename/restructure to allow for upcoming test additions.
This field is now redundant since the connection type field will indicate
MANUAL for addnode connections.
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 getpeerinfo fields services and servicesnames is an example of where we return separate values for machines vs humans.
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 a512925e19a70d7f6b80ac530a169f45ffaafa1c.
I certainly don’t think we should rush through merges
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.
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
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 getpeerinfo() quite a bit while debugging, and not having to (eg) infer whether a peer is a block-relay-only outbound by using the inbound and relaytxes variables, and being able to grep for a human readable string, would save me time.
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 ! @sdaftuar
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?
good idea. In the interest of preserving ACKs I’ve added https://github.com/amitiuttarwar/bitcoin/commit/099e38724adb2c002f0b9ec4271f9247067844d6 on another branch. I’ll either incorporate here or can do in a follow-up. @michaelfolkson
Approach NACK. I prefer the approach in #19883 (comment).
can you please provide the rationale for your NACK? as I stated in #19725 (comment) (and others have also expressed)
I standby my view that these changes are not in conflict with #19883- the getpeerinfo fields services and servicesnames is an example of where we return separate values for machines vs humans.
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:
When considering replacing integer ids with long-format string ids for API clients, remember that the long-format naming is:
(a) in flux (e.g., some people write “block-relay”, others “block-relay-only”, some now write “b-r-o”, etc.)
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.
(b) will change as new and additional types are conceived
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 getblockchaininfo RPC that the "type" field can only ever be "buried" or "bip9", and then coming to the painful observation that this means we’re unable to adopt new activation mechanisms…
(c) the GUI, CLI -netinfo, and client software will all have to translate the naming anyway for their human-facing needs and > display constraints
Agree, but I don’t think this is any worse or better for numbers vs names.
Consider why we use integers for BIP numbers. Once one is assigned, it can be stable, for it is decoupled from the changing naming and fashion of the day.
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.
cr ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c 🌇
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3cr ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c 🌇
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjoEQv/dMdhuBqd2oAqHggaa2Yb972bRNVWlue2JmORCDOpmItlSBjiwo5hcJyV
8W4ZcGG732EVI/jZDbr2oa7QfYOCcR8JVWVqNJLz637l6R1SbquwXl39uuMdgp03Q
9LYutCkf361QIIzHfVy4ayGjjNH9UVeGVmh+u0vBx8kWivTzxF0mlFcilc6vaVFnp
10luviz3qJN/TtuSkvWYtIPJ6mqRxJLMkIRfRFl43puzXLojKipJI7vHQHAO07BFj/
11W25yCxGBIJ9lMxofAgJS3fNQQ9YwbVw+CsHInR8fvuAOjLTkSEmEsKk97yvVqvrF
12Ok7Nw67CyxRJ4b7NF7E6tu10iJDnirwbqkEiz/a7apIq7/V+SRYzTFXy/BMUcOx4
13ZYzUUgc1hOYXcgaIf+ormDmuar+JXFGEIFRxugNMgrz2nKZD++kgg+/22kue/gkf
14WEdi7ZPwfu23HXPKectDeNldcWlZFofGvdfoNMbTpeRnJuisS6tys6bkNrKLOfiw
15H0WlvBpz
16=rQUg
17-----END PGP SIGNATURE-----
Timestamp of file with hash e3ad231c501be41233b2be1a9001e466947870576a8514e0d67035a2f2a5e785 -
118+ "outbound-full-relay (default automatic connections)",
119+ "block-relay-only (does not relay transactions or addresses)",
120+ "inbound (initiated by the peer)",
121+ "manual (added via addnode RPC or -addnode/-connect configuration options)",
122+ "addr-fetch (short-lived automatic connection for soliciting addresses)",
123+ "feeler (short-lived automatic connection for testing addresses)"};
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):
0diff --git a/src/net.cpp b/src/net.cpp
1index 5b533d7d17..166fc8233d 100644
2--- a/src/net.cpp
3+++ b/src/net.cpp
4@@ -488,6 +488,15 @@ void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNet
5 }
6 }
7
8+extern const std::vector<std::string> CONNECTION_TYPE_DOC{
9+ "outbound-full-relay (default automatic connections)",
10+ "block-relay-only (does not relay transactions or addresses)",
11+ "inbound (initiated by the peer)",
12+ "manual (added via addnode RPC or -addnode/-connect configuration options)",
13+ "addr-fetch (short-lived automatic connection for soliciting addresses)",
14+ "feeler (short-lived automatic connection for testing addresses)",
15+};
16+
17 std::string CNode::ConnectionTypeAsString() const
18 {
19 switch (m_conn_type) {
20diff --git a/src/net.h b/src/net.h
21index 5a8e57b68b..3c6c45c161 100644
22--- a/src/net.h
23+++ b/src/net.h
24@@ -114,13 +114,7 @@ struct CSerializedNetMsg
25 std::string m_type;
26 };
27
28-const std::vector<std::string> CONNECTION_TYPE_DOC{
29- "outbound-full-relay (default automatic connections)",
30- "block-relay-only (does not relay transactions or addresses)",
31- "inbound (initiated by the peer)",
32- "manual (added via addnode RPC or -addnode/-connect configuration options)",
33- "addr-fetch (short-lived automatic connection for soliciting addresses)",
34- "feeler (short-lived automatic connection for testing addresses)"};
35+extern const std::vector<std::string> CONNECTION_TYPE_DOC;
36
37 /** Different types of connections to a peer. This enum encapsulates the
38 * information we have available at the time of opening or accepting the
Just tested locally, and the size does decrease by about 10kB:
0--- a/tmp/old/d_size
1+++ b/tmp/new/d_size
2@@ -1 +1 @@
3-10044320
4+10027432