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).
nit: instead of adding extended documentation to the release notes, the documentation can be added to the rpc itself. Affected/interested users can call help rpc_method or --help if needed.
okay, removed the specifics. better?
874 | @@ -874,6 +875,25 @@ class CNode 875 | assert(false); 876 | } 877 | 878 | + std::string ConnectionTypeToString() const {
For new code it would make sense to properly clang-format it. See e.g. https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy
thanks, think I've updated all the commits with proper clang formatting. moving forward, will try to incorporate this into my workflow.
This seems strangely implementation-specific to me. I think we should at least keep a simple connection direction somewhere?
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";
nit: Would it make sense to replace the spaces with - 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"
done
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"
{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?
done
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
field.
Probably can remove the extended documentation
done
Approach ACK, just some style-nits.
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
I think it's premature to be deprecating these fields. It would be preferable to let the new code mature for a release or so and to implement how it might be presented in the GUI, before considering removing access to these via the RPC.
Agree that we don't need to be overly aggressive with deprecation. If deprecation/removal is too controversial for now, it can be split up into another follow-up pull for later. I think adding the field should be uncontroversial and conceptually easier to get in.
I don't think there is a reason to remove "inbound", ever. It's very useful to filter RPC output by on its own.
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 -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 :)
Ok, I checked the code and indeed, (a) I misread you and (b) getpeerinfo addnode and conn_type manual are the same.
Concept ACK on adding the connection type to getpeerinfo and the logging.
thanks for the reviews! I've removed the "deprecate getpeerinfo inbound" commit & addressed all other review comments.
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 | + }
Please add // 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)
added c99b26010eaf4d446eb5118e38dbcc03fabba11c in #19724
thanks for the review @laanwj. addressed all review comments
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.
ah thanks, forgot to update after actually opening the PR
done
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.
Could say that connection_type=manual when addnode=true.
done
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;
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.
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
resolving this convo for now, feel free to reopen/comment if you'd like me consider an alternative approach :)
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 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?
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.
Concept ACK.
113 | @@ -114,6 +114,15 @@ struct CSerializedNetMsg 114 | std::string m_type; 115 | }; 116 | 117 | +const std::vector<std::string> CONNECTION_TYPE_DOC{
Is it possible to add an assert that the size of this vector is the same as the size of ConnectionType, so it's not possible to update one without the other?
I like the idea, but it looks like there's no straightforward way to get the length of the enum? :(
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:
enum class en {
thing1,
thing2,
thing3, // added a new thing
MAX,
};
int main()
{
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
static_assert(a.size() == (int)en::MAX); // always passes
for (const auto& x : a) {
std::cout << x << std::endl;
}
}
Would you replace the value in the static_assert with a hard-coded value?
or maybe hardcode the length of the array?
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:
cat src/util/enum_class.h
// Copyright (c) 2020 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#ifndef BITCOIN_UTIL_ENUM_CLASS_H
#define BITCOIN_UTIL_ENUM_CLASS_H
#include <boost/preprocessor/variadic/size.hpp>
#define ENUM_CLASS(name, ...) \
enum class name { __VA_ARGS__ }; \
constexpr size_t name##_##SIZE { BOOST_PP_VARIADIC_SIZE(__VA_ARGS__) }
#endif // BITCOIN_UTIL_ENUM_CLASS_H
This could be used to write
static_assert(CONNECTION_TYPE_DOC.size() == ConnectionType_SIZE, "inconsistent docs");
Probably not worth the effort. Just saying that it is possible.
bah. Adding a 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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
ACK f7f3ef4f099d1f44b25c255baa0d25de448f3bdf (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.
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).
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 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.
This is a very minor change to the RPC interface. I don't think we need any special treatment for deprecating 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.
While these are release notes, I still think it's worth it to at least list those 6 options to give a hint at what they are, even if describing each may be too verbose.
In general, I think we try to avoid duplicating documentation in this way. Release notes don't need to include all the details, and pointing where to go for the source of truth is appropriate here.
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.
ok, seems like the dominant reviewer preference is to leave as is? so, resolving this conversation
486 | @@ -487,6 +487,26 @@ void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNet 487 | } 488 | } 489 | 490 | +std::string CNode::ConnectionTypeAsString() const
μNit, feel free to ignore: the As feels redundant. (I.e. ConnectionTypeString())
Nah, I propsed the current naming (it was already changed once). Lets not bikeshed it too much.
ISTM this is a presentation concern, and could either be moved out to the RPC code or removed completely if it wasn't needed for the RPC help, as API clients will in any case need to re-implement this logic.
did you see the commit that introduced this function? (now https://github.com/bitcoin/bitcoin/pull/19725/commits/53059f9476b00f83a9f8161c5d03d857ce0cb164). It uses the string for logging in net_processing.cpp, so it wouldn't make sense to move to the RPC code.
I think it's an unneeded abstraction.
Concept ACK
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:
oops, looks like I have some test failures. will investigate / fix tomorrow.
tests fixed! ready for review.
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/
done, thanks!
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)"
s/short lived/short-lived/ in this line and the preceding one
done
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);
Would suggest placing it on the line immediately after 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")
s/inbound/addnode/
oops. fixed, thanks!
utACK 2499b120cca63a5c13ea6e782d8ff53caae5d3dd
@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.
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.
Same, almost exclusively use the command line too. I hope we can have the uint8_t ids but if people want this then ACK from me.
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?
Concept ACK. Seems fine to return the type as a human readable string that can also be logged. If the strings ever need to change and a client needs a stable interface, e.g. to collect statistics on connection types, then a separate PR could add integral ids at that time (or earlier, when needed).
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.
utACK a512925e19a70d7f6b80ac530a169f45ffaafa1c
Tested and code review ACK a512925e.
cr ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c 🌇
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
cr ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c 🌇
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjoEQv/dMdhuBqd2oAqHggaa2Yb972bRNVWlue2JmORCDOpmItlSBjiwo5hcJyV
W4ZcGG732EVI/jZDbr2oa7QfYOCcR8JVWVqNJLz637l6R1SbquwXl39uuMdgp03Q
LYutCkf361QIIzHfVy4ayGjjNH9UVeGVmh+u0vBx8kWivTzxF0mlFcilc6vaVFnp
luviz3qJN/TtuSkvWYtIPJ6mqRxJLMkIRfRFl43puzXLojKipJI7vHQHAO07BFj/
W25yCxGBIJ9lMxofAgJS3fNQQ9YwbVw+CsHInR8fvuAOjLTkSEmEsKk97yvVqvrF
Ok7Nw67CyxRJ4b7NF7E6tu10iJDnirwbqkEiz/a7apIq7/V+SRYzTFXy/BMUcOx4
ZYzUUgc1hOYXcgaIf+ormDmuar+JXFGEIFRxugNMgrz2nKZD++kgg+/22kue/gkf
WEdi7ZPwfu23HXPKectDeNldcWlZFofGvdfoNMbTpeRnJuisS6tys6bkNrKLOfiw
H0WlvBpz
=rQUg
-----END PGP SIGNATURE-----
Timestamp of file with hash e3ad231c501be41233b2be1a9001e466947870576a8514e0d67035a2f2a5e785 -
</details>
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):
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 the
Just tested locally, and the size does decrease by about 10kB:
--- a/tmp/old/d_size
+++ b/tmp/new/d_size
@@ -1 +1 @@
-10044320
+10027432
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! https://github.com/bitcoin/bitcoin/pull/20090