net: option to disallow v1 connection on ipv4 and ipv6 peers#30951
net: option to disallow v1 connection on ipv4 and ipv6 peers#30951stratospher wants to merge 5 commits into
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30951. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste 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. LLM Linter (✨ experimental)Possible typos and grammar issues:
Possible places where named args for integral literals may be used (e.g.
2026-04-29 08:49:42 |
cd4f811 to
0cfc9fc
Compare
7c9e5a1 to
ab5f875
Compare
|
In #29618 and here, I don't see the motivation to forbid v1 connections. In other words, what is the purpose of this? In addition to the concerns expressed in #29618, there is one more - if V2-only is widespread, it may be hard to eclipse V2-only node, but then it becomes easy to eclipse a V1 node which has a problem of finding peers to connect to, so an attacker starts a lot of nodes that do accept V1 connections. I see possible downsides and 0 benefit of a v2only option. |
|
Concept ACK
Well, to gain the benefits of BIP324. For example if you are concerned about someone inspecting the traffic to your node it really doesn't matter how many BIP324 peers you have if you have just one unencrypted connection. You could also run tor-only in this case, but that has its own disadvantages and
that would be a problem, but |
Our docs address this to an extent here and here. As those docs point out,
Agree. My node, for instance, runs over tor/i2p/cjdns and makes manual connections to trusted clearnet peers.
Agree. With this change, clearnet node operators may need to accept v2, whether they wish to or not. We generally try to avoid forcing users to do things. I suppose my concept ack on this would depend on that tradeoff. |
True. Lets dive a bit deeper - what are the reasons to be concerned about somebody inspecting the traffic to my node? |
| @@ -550,6 +550,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) | |||
| argsman.AddArg("-i2pacceptincoming", strprintf("Whether to accept inbound I2P connections (default: %i). Ignored if -i2psam is not set. Listening for inbound I2P connections is done through the SAM proxy, not by binding to a local address and port.", DEFAULT_I2P_ACCEPT_INCOMING), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | |||
| argsman.AddArg("-onlynet=<net>", "Make automatic outbound connections only to network <net> (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. It can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | |||
| argsman.AddArg("-v2transport", strprintf("Support v2 transport (default: %u)", DEFAULT_V2_TRANSPORT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | |||
| argsman.AddArg("-v2onlyclearnet", strprintf("Disallow v1 connections on IPV4/IPV6 (default: %u). Enabling this is not recommended unless absolutely necessary, as it may risk network partitions if all users enable it.", false), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | |||
There was a problem hiding this comment.
Enabling this is not recommended unless absolutely necessary, as it may risk network partitions
This is unfortunately only one brigading campaign by one or more prominent bitcoiners on social media away from being potentially adopted by many nodes.
With -onlynet, the network partition risk is in practice opt-in, i.e. chosen by a minority subset for themselves. Whereas here, the network partitioning and the lack of peers risks could be imposed on a future minority of users that are running earlier versions of Bitcoin Core. This seems at odds with maintaining backward compatibility for that minority.
Some thoughts off the top my head:
In both of these cases, if you accept inbounds they could just connect to you as an alternative (but they would need to have a suspicion, whereas detection of bitcoin-related network data could be automated). So another alternative would be to only disallow v1 connections for outbound peers. In that case, if you don't want any v1 connections at all you'd have to run with |
This is what is worrying me - V2-only will not hide the fact that I am running a bitcoin node, but may give the false impression that it does. This is dangerous. My node will connect to publicly known bitcoin nodes and the mere fact that I have a TCP connection to an Another possible misunderstanding is that V2-only makes the traffic impossible to spy. I can imagine a blatant MITM, which is detectable because the session keys would differ, should somebody bother to check them. Once I detect this, whom am I going to complain to? My oppressive gov/ISP/whatever, same one that is doing the spying? Or the ISP can outright redirect my connections to
V2-only may give the false impression that it breaks the link between transaction origin and IP address / geolocation. Even with unspyable p2p encryption, I may have a connection to a spy bitcoin node which then sees my traffic. Like you said "it really doesn't matter how many BIP324 peers you have if you have just one unencrypted connection" I think this is the same as "it does not matter even if all your connections are encrypted, if you have just one (encrypted) connection to a spy node". |
|
@vasild V2 only doesn't protect against active attacks like the ones you've mentioned which require resources from the other party to run a node to spy the traffic. V2 only protects against a cheaper attack vectors possible because of passive inspection of network traffic flowing through different medium. They don't need the other party to run a node to spy.
Yes, for the same reason as wanting to encrypt network traffic in the first place and this just offers a stronger guarantee when running a node with v2 support that unencrypted clearnet bitcoin traffic is not being collected and sold by other parties. Deep packet inspection, keyword filtering are cheaper for ISPs/firewalls to pull off and v2 only would protect against this. |
|
A V2-only option will:
but it will give the false impression that it does those two things. Further, it will make it more difficult for old nodes to find peers to connect to. Concept NACK because of that. |
|
Concept ACK This option seems useful for users that want to:
Have you checked how diverse these ~60% of v2 supporting nodes are? An extreme example: if they are all on running on AWS then I don't think we should add the option at this time. |
How? Given that the spy does not need to inspect the traffic at all:
|
|
@vasild That assumes the attacker has sufficient monitoring infrastructure in place to maintain an accurate list of Bitcoin P2P ip:ports. That's obviously not impossible, but it is a significantly larger effort than stateless packet inspection looking for the string "\xf9\xbe\xb4\xd9version\x00\x00\x00\x00\x00". And yes, very little in BIP324 protects against an attacker deliberately targetting specific individuals - its goal is increasing costs for large-scale monitoring. From that perspective I agree there is a risk for a false sense of security when introducing options like these, but I think it's a stretch to say that that makes it pointless. I can very well imagine this makes it possible to run a node at all for some users. However, as I pointed out in the linked issue, I am somewhat concerned about a potential future where it becomes harder for other/older software to connect to the network if large swaths move to be v2-only. Because of that, I wonder if it isn't better to make this only apply to outbound connections, and advise people who want more private operation to not listen for inbound connections. |
Yes. I may be potentially concept/approach ack here if this proposal adopts the suggestion above / in #29618 (comment) or #29618 (comment). (Along with perhaps additional user documentation for node runners, maybe in the relevant config option helps and/or in |
This comment was marked as abuse.
This comment was marked as abuse.
|
A mutation testing report for this PR is available at: https://brunoerg.xyz/mutation-core-front |
14ee29d to
6b2796a
Compare
|
Concept ACK Afaict the project has communicated clearly that v2 encryption aims to protect against passive adversaries. It seems desirable to make good on that. Adding this to the 32 milestone. |
|
Concept ACK |
6b2796a to
f7027bd
Compare
f7027bd to
1e61206
Compare
fjahr
left a comment
There was a problem hiding this comment.
However this feels like lots of users setting listen=0 problem, and we're showing someone a reason for setting listen=0 + making that future slightly more probable. Will need to think more about this and curious about other people's thoughts on #30951 (review) as well.
Re-reading the code and @mzumsande's argument, I think I am now slightly leaning towards not tying it to listen=0 but rather only disallowing v1 connections. But I would still be fine with the current approach as well, especially if we can find a bit better wording.
|
|
||
| if (args.GetBoolArg("-v2onlyclearnet", DEFAULT_V2_ONLY_CLEARNET)) { | ||
| if (args.GetBoolArg("-listen", DEFAULT_LISTEN)) { | ||
| return InitError(_("Cannot set -v2onlyclearnet=1 with listen=0. Current listen=1 provides valuable network capacity. Use -v2onlyclearnet only if you truly need encrypted-only outbound traffic.")); |
There was a problem hiding this comment.
This error message is a bit confusing, first "Cannot set -v2onlyclearnet=1 with listen=0" seems to be wrong, it should say "listen=1" there (or s/with/without/). And then maybe switch the order of the current second and third sentence, I think that would read a bit better. For my taste the second sentence is a bit much because I don't think it's going to change anyone's mind but I would be fine to keep it. But we could also ask people to read the help text of the option carefully where we are more detailed.
There was a problem hiding this comment.
oh whoops, thank you for the great catch!
maybe also makes sense to keep the error message short + keep the context only in the help text - it also prevents duplication. I've changed the error message to : "Cannot set -v2onlyclearnet=1 with listen=1. See -help for details on -v2onlyclearnet." open to better wordings/suggestions as well or if people feel we should give slightly more context.
| argsman.AddArg("-i2pacceptincoming", strprintf("Whether to accept inbound I2P connections (default: %i). Ignored if -i2psam is not set. Listening for inbound I2P connections is done through the SAM proxy, not by binding to a local address and port.", DEFAULT_I2P_ACCEPT_INCOMING), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | ||
| argsman.AddArg("-onlynet=<net>", "Make automatic outbound connections only to network <net> (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. It can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | ||
| argsman.AddArg("-v2transport", strprintf("Support v2 transport (default: %u)", DEFAULT_V2_TRANSPORT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | ||
| argsman.AddArg("-v2onlyclearnet", strprintf("Ensure all outbound IPv4/IPv6 peers use encrypted network traffic (default: %u). Using this option requires -listen=0 and takes valuable listening capacity away from the network. Enable this option only if passive network observers like ISPs, firewalls, etc. pose a threat and unencrypted network traffic must be avoided. Note: Encryption protects message contents but does not obscure that you are running a Bitcoin node. Peers can still connect to you, observers can identify Bitcoin activity from connection attempts on the default port (8333) or from analysing Bitcoin network traffic patterns.", DEFAULT_V2_ONLY_CLEARNET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); |
There was a problem hiding this comment.
Not a strong opinion but as it is worded now, probably everyone who runs a bitcoin node will say, yes, these network observers are a problem for my privacy. It might be better to say something like: "if you are at risk of your traffic getting blocked or service shut down". That would be a bit more strongly steer people away from this if they don't know what they are doing/don't really need it. But it's just an idea, feel free to ignore.
There was a problem hiding this comment.
It might be better to say something like: "if you are at risk of your traffic getting blocked or service shut down"
If you are at such a risk, then -v2onlyclearnet=1 will not help. Mentioning "passive" is good because -v2onlyclearnet=1 protects only against "passive" observers and not against "active" ones who can MITM themselves and spy on the encrypted traffic. Then the user has no way to ensure that a possible adversary will stay "passive" and will not switch to "active" at any moment.
only-clearnet condition is used in a few other places as well.
a boolean option `m_v2only_clearnet` is introduced in CConman which will (in a later commit) store if the user wishes to establish only outbound v2 connections on IPV4 and IPV6 networks since they are unencrypted. this option is accessible outside CConman using `RequiresV2ForOutbound()` function with the IP address we're trying to connect to passed as an argument.
if this option is set by the user, v1 connections on unencrypted networks like IPV4/IPV6 will be disallowed. Only users with real need are recommended to turn this on because it could risk network partitioning in the unlikely scenario that everyone turns it on.
if `-v2onlyclearnet` is turned on, - v1 addresses from addrman aren't selected and manual connections aren't attempted for outbound connections if it's from IPV4/IPV6 networks. - v1 downgrade mechanism is not attempted if v2 connection wasn't successful
when `-v2onlyclearnet` is turned on: - v1 connections to clearnet peers don't work - v2 connections to clearnet peers work - v1 connections to tor/i2p/cjdns peer works a proxy is used because otherwise NET_UNROUTABLE is the default network in the tests.
1e61206 to
263c16b
Compare
|
thank you for the great catch @fjahr! I've addressed your comments in this push. I've made the InitError message shorter but kept the help text as it is. also summarising my understanding of the 2 implementation options possible for purpose is assurance that passive observers can't see your encrypted traffic content (it doesn't hides the fact that you're running a Bitcoin node nor protect you from active adversaries)
which implementation option is better?
I personally am ok with both. Maybe slight inclination to B since it gives the user configuration knob to do what they want. And we could document it in the help text to mention using listen=0 as well. Not sure what others prefer though. Would love thoughts on what people prefer before changing approach/pushing an update :) |
Can you elaborate on how v2 without inbounds makes traffic analysis more difficult than v2 with inbounds? I'm not a traffic analysis expert, and to me it would make sense that any analysis happening on 10 v2 connections vs 125 v2 connections would come to the same conclusion about whether or not you were running a bitcoin node? I'm fairly sure I'm missing something though... :) But otherwise I would lean towards requiring |
| std::string GetNetworkName(enum Network net); | ||
| /** Return a vector of publicly routable Network names; optionally append NET_UNROUTABLE. */ | ||
| std::vector<std::string> GetNetworkNames(bool append_unroutable = false); | ||
| bool IsClearnet(enum Network net); |
There was a problem hiding this comment.
nit: A Doxygen comment can be added.
| bool IsClearnet(enum Network net); | |
| /** Returns true if the network is IPv4 or IPv6 (as opposed to a privacy network like Tor/I2P/CJDNS). */ | |
| bool IsClearnet(enum Network net); |
| /** -listen default */ | ||
| static const bool DEFAULT_LISTEN = true; | ||
| /** -v2onlyclearnet default */ | ||
| static const bool DEFAULT_V2_ONLY_CLEARNET = false; |
There was a problem hiding this comment.
nit:
| static const bool DEFAULT_V2_ONLY_CLEARNET = false; | |
| static constexpr bool DEFAULT_V2_ONLY_CLEARNET{false}; |
| * outbound connections on IPV4/IPV6 need to be v2 connections. | ||
| * outbound connections on Tor/I2P/CJDNS can be v1 or v2 connections. | ||
| */ | ||
| bool m_v2only_clearnet; |
There was a problem hiding this comment.
If code ever reads m_v2only_clearnet before Init() is called, it's UB.
| bool m_v2only_clearnet; | |
| bool m_v2only_clearnet{false}; |
| argsman.AddArg("-i2pacceptincoming", strprintf("Whether to accept inbound I2P connections (default: %i). Ignored if -i2psam is not set. Listening for inbound I2P connections is done through the SAM proxy, not by binding to a local address and port.", DEFAULT_I2P_ACCEPT_INCOMING), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | ||
| argsman.AddArg("-onlynet=<net>", "Make automatic outbound connections only to network <net> (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. It can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | ||
| argsman.AddArg("-v2transport", strprintf("Support v2 transport (default: %u)", DEFAULT_V2_TRANSPORT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | ||
| argsman.AddArg("-v2onlyclearnet", strprintf("Ensure all outbound IPv4/IPv6 peers use encrypted network traffic (default: %u). Using this option requires -listen=0 and takes valuable listening capacity away from the network. Enable this option only if passive network observers like ISPs, firewalls, etc. pose a threat and unencrypted network traffic must be avoided. Note: Encryption protects message contents but does not obscure that you are running a Bitcoin node. Peers can still connect to you, observers can identify Bitcoin activity from connection attempts on the default port (8333) or from analysing Bitcoin network traffic patterns.", DEFAULT_V2_ONLY_CLEARNET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); |
There was a problem hiding this comment.
| argsman.AddArg("-v2onlyclearnet", strprintf("Ensure all outbound IPv4/IPv6 peers use encrypted network traffic (default: %u). Using this option requires -listen=0 and takes valuable listening capacity away from the network. Enable this option only if passive network observers like ISPs, firewalls, etc. pose a threat and unencrypted network traffic must be avoided. Note: Encryption protects message contents but does not obscure that you are running a Bitcoin node. Peers can still connect to you, observers can identify Bitcoin activity from connection attempts on the default port (8333) or from analysing Bitcoin network traffic patterns.", DEFAULT_V2_ONLY_CLEARNET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | |
| argsman.AddArg("-v2onlyclearnet", strprintf("Ensure all outbound IPv4/IPv6 peers use encrypted network traffic (default: %u). Using this option requires -listen=0 and takes valuable listening capacity away from the network. Enable this option only if passive network observers like ISPs, firewalls, etc. pose a threat and unencrypted network traffic must be avoided. Note: Encryption protects message contents but does not obscure that you are running a Bitcoin node. Peers can still connect to you, observers can still identify Bitcoin activity from your outbound connection attempts to the default port (8333) or by analysing traffic patterns.", DEFAULT_V2_ONLY_CLEARNET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); |
| std::unique_ptr<i2p::sam::Session> i2p_transient_session; | ||
|
|
||
| for (auto& target_addr : connect_to) { | ||
| if (RequiresV2ForOutbound(target_addr) && !use_v2transport) { |
There was a problem hiding this comment.
If a user enables this option and wonders why connections to certain peers aren't happening, there's nothing in the debug log to explain it.
| if (RequiresV2ForOutbound(target_addr) && !use_v2transport) { | |
| if (RequiresV2ForOutbound(target_addr) && !use_v2transport) { | |
| LogDebug(BCLog::NET, "skipping v1 connection to %s (-v2onlyclearnet)\n", target_addr.ToStringAddrPort()); |
| std::unique_ptr<i2p::sam::Session> i2p_transient_session; | ||
|
|
||
| for (auto& target_addr : connect_to) { | ||
| if (RequiresV2ForOutbound(target_addr) && !use_v2transport) { |
There was a problem hiding this comment.
When pszDest is unresolved because DNS is delegated to a name proxy, it might be better to treat it as possibly IPv4/IPv6 and block v1 under -v2onlyclearnet.
| if (RequiresV2ForOutbound(target_addr) && !use_v2transport) { | |
| const bool possibly_clearnet_name{pszDest && !target_addr.IsValid()}; | |
| if ((RequiresV2ForOutbound(target_addr) || | |
| (m_v2only_clearnet && possibly_clearnet_name)) && | |
| !use_v2transport) { |
P.S.: Same logic can be applied to DisconnectNodes().
// and we don't want to hold up the socket handler thread for that long.
- if (network_active && !RequiresV2ForOutbound(pnode->addr) && pnode->m_transport->ShouldReconnectV1()) {
+ const bool possibly_clearnet_name{
+ m_v2only_clearnet && !pnode->addr.IsValid() && !pnode->m_dest.empty()};
+ if (network_active && !RequiresV2ForOutbound(pnode->addr) &&
+ !possibly_clearnet_name && pnode->m_transport->ShouldReconnectV1()) {
reconnections_to_add.push_back({
.addr_connect = pnode->addr,
I think there are two issues if you don't want to be discovered that you are running a node and have
That being said, I still lean toward option B, of not tying I am Concept +0 leaning towards Concept ACK on this, I agree that given all of the other privacy issues that exist in both Bitcoin P2P and Bitcoin Core's implementation there won't be a step change in privacy from doing this; but I am very skeptical of this species of argument since the same arguments are used circularly to persist the other practices that degrade privacy persist, e.g. port 8333 (why change the port if we are detectable anyways via v1 connections?) Yes there are other problems, and they ought to be addressed, but raising the cost seems a good direction. The only concern I feel hasn't been clearly addressed is the depletion of inbound v1 slots on the network; but maybe this isn't an issue needing discussing yet since so few will run with this option enabled. |
resolves #29618.
This PR adds a config flag option
-v2onlyclearnetwhich disallows outbound v1 connections on ipv4 and ipv6 networks since they're unencrypted. outbound v1 connections are still possible from tor/i2p/cjdns peers since they're encrypted networks.-v2onlyclearnetis switched on)Currently 70% of reachable nodes in network supporting v2 (according to https://21.ninja/reachable-nodes/node-service-share/ and https://bitnodes.io/nodes). Also found 81% of addresses supporting v2 in the addrman of nodes I checked - personal node and the nodes in peer.observer. (You can use this branch to check the % of v2 addresses in a node's addrman.)