Merged
Conversation
Allow to connect to multiple nodes behind the same IP
If the user specified -port, he very likely intends to connect to nodes with the same port.
UdjinM6
requested changes
Jul 5, 2018
src/test/net_tests.cpp
Outdated
| CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted); | ||
| bool exceptionThrown = false; | ||
| CAddrMan addrman1; | ||
| CAddrMan addrman1(false); |
There was a problem hiding this comment.
Not needed, false is the default one (same for all changes below in this file).
src/net.cpp
Outdated
| { | ||
| if (pszDest == NULL) { | ||
| if (IsLocal(addrConnect)) | ||
| if (IsLocal(addrConnect) && (!Params().AllowMultiplePorts() || addrConnect.GetPort() == GetListenPort())) { |
There was a problem hiding this comment.
It's too hard to read, maybe
bool fAllowLocal = Params().AllowMultiplePorts() && addrConnect.GetPort() != GetListenPort();
if (!fAllowLocal && IsLocal(addrConnect)) {
?
src/net.cpp
Outdated
| break; | ||
|
|
||
| // if we selected a local address, restart (local addresses are allowed in regtest and devnet) | ||
| if (IsLocal(addr) && (!Params().AllowMultiplePorts() || addr.GetPort() == GetListenPort())) |
src/net.cpp
Outdated
| if (IsBanned(addrConnect) || | ||
| FindNode(addrConnect.ToStringIPPort())) | ||
| return false; | ||
| if (IsLocal(addrConnect) && (!Params().AllowMultiplePorts() || addrConnect.GetPort() == GetListenPort())) |
There was a problem hiding this comment.
Same here
bool fAllowLocal = Params().AllowMultiplePorts() && addrConnect.GetPort() != GetListenPort();
if ((!fAllowLocal && IsLocal(addrConnect)) ||
(!Params().AllowMultiplePorts() && FindNode((CNetAddr)addrConnect)) ||
(Params().AllowMultiplePorts() && FindNode((CService)addrConnect)) ||
IsBanned(addrConnect) ||
FindNode(addrConnect.ToStringIPPort()))
return false;
Collaborator
|
As discussed on Slack, non-standard ports seem to cause re-connection issues. If a node starts on non-default ports, it will not connect automatically to a remote peer running on that port unless To summarize:
Also, a little off-topic for the PR probably, but why does |
Author
|
Added suggested changes. I however left the multiple ifs in OpenNetworkConnection intact, as I always found the original if extremely hard to read. I added a few comments to make it easier to read. |
|
Tested and it seems to work as expected. utACK -> ACK |
andvgal
pushed a commit
to energicryptocurrency/gen2-energi
that referenced
this pull request
Jan 6, 2019
* Remove testnet seeds from devnet * Lift multiple ports restriction on devnet when considering new nodes Allow to connect to multiple nodes behind the same IP * Don't skip addresses with non-default port if it matches -port If the user specified -port, he very likely intends to connect to nodes with the same port. * Don't pass false to CAddrMan constructor as it is already the default * Make if statements easier to read
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Jun 11, 2024
, bitcoin#22734, bitcoin#22950, bitcoin#23053, bitcoin#22839, bitcoin#23140, bitcoin#23306, bitcoin#23354, bitcoin#23380 (addrman backports: part 2) a93fec6 merge bitcoin#23380: Fix AddrMan::Add() return semantics and logging (Kittywhiskers Van Gogh) d1a4b14 merge bitcoin#23354: Introduce new V4 format addrman (Kittywhiskers Van Gogh) 7a97aab test: restore pre-bitcoin#23306 tests to validate port distinguishment (Kittywhiskers Van Gogh) 1a050d6 merge bitcoin#23306: Make AddrMan support multiple ports per IP (Kittywhiskers Van Gogh) d56702a merge bitcoin#23140: Make CAddrman::Select_ select buckets, not positions, first (Kittywhiskers Van Gogh) 19b0145 merge bitcoin#22839: improve addrman logging (Kittywhiskers Van Gogh) 3910c68 merge bitcoin#23053: Use public methods in addrman fuzz tests (Kittywhiskers Van Gogh) b6ec8ab merge bitcoin#22950: Pimpl AddrMan to abstract implementation details (Kittywhiskers Van Gogh) 236cf36 merge bitcoin#22734: Avoid crash on corrupt data, Force Check after deserialize (Kittywhiskers Van Gogh) 2420ac9 merge bitcoin#23041: Add addrman deserialization error tests (Kittywhiskers Van Gogh) 8aefa9b merge bitcoin#22762: Raise InitError when peers.dat is invalid or corrupted (Kittywhiskers Van Gogh) c9a645f merge bitcoin#22879: Fix format string in deserialize error (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6040. * Dash already introduced support for storage of address-port pairs (referred to as "port discrimination") and allowed the usage of non-default ports in P2P with [dash#2168](#2168). * Albeit this was only permitted for networks with `fAllowMultiplePorts` enabled (which at the time was `devnet` and as it stands currently, on every network except `mainnet`). * Keeping in line with the above policy (discussion on lifting such restrictions on `mainnet` is outside the scope of this PR), when backporting [bitcoin#23306](bitcoin#23306), changes have been made to retain the effects of `discriminate_ports`. * This involves appending placing a `!m_discriminate_ports` condition to behaviour that otherwise would be _removed_ entirely. * Additionally, in line with upstream backports, changes have been made that render port distinguishment _enabled_ as the new default in `addrman_tests` (the old default was to keep it _disabled_, to mirror `mainnet` and pre-change upstream behaviour). * To ensure distinguishment _disabled_ works as expected, affected pre-backport tests were reintroduced with the `_nondiscriminate` suffix. --- I would propose at some point to rename the flag to `ignore_port`/`suppress_port` (if not remove it altogether should the `mainnet` restriction be lifted) as discriminate (or distinguish) isn't immediately clear with if address entries will be discriminated/distinguished _using_ ports (i.e. considered) or ports will be discriminated _against_ (i.e. ignored). ## Breaking Changes It's unclear if these backports result in serialization issues for older versions, as Dash Core technically supported address-port pairs since 0.12 and suppressed it on `mainnet` by setting zero-ing out the port ([source](https://github.com/dashpay/dash/blob/19512988c6e6e8641245bd9c5fab21dd737561f0/src/addrman.cpp#L135-L138)), meaning even with port discrimination _disabled_, the serialization format should remain the same. Regardless, following upstream backports, a new version has been introduced (v4) that marks the AddrMan format incompatible with older versions of Dash Core. ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK a93fec6 PastaPastaPasta: utACK a93fec6 Tree-SHA512: 49b35af3e4eb660249ce9a65d5a539205d852e9c728da22dc88779f6b3b15c13cf91522896a313bfe2a91889fedf3b6b2cebdea12cc2bbe865ec3b85b6a5dfa8
PastaPastaPasta
added a commit
that referenced
this pull request
Jan 15, 2025
, bitcoin#24468, bitcoin#25119, bitcoin#25176, bitcoin#25421, bitcoin#26248, bitcoin#26199, bitcoin#27036, bitcoin#27270, partial bitcoin#27106 (networking backports: part 10) 3260f2c partial bitcoin#27106: remove orphaned CSubNet::SanityCheck() (Kittywhiskers Van Gogh) 75cc94e merge bitcoin#27270: Avoid CNode::m_relays_txs usage (Kittywhiskers Van Gogh) f961903 merge bitcoin#27036: Remove last uses of snprintf and simplify (Kittywhiskers Van Gogh) d80bbe9 merge bitcoin#26199: Don't self-advertise during version processing (Kittywhiskers Van Gogh) 4b17baf merge bitcoin#26248: Set relay in version msg to peers with relay permission in -blocksonly mode (Kittywhiskers Van Gogh) 18738f5 merge bitcoin#25421: convert standalone IsSelectableSocket() and SetSocketNonBlocking() to Sock methods (Kittywhiskers Van Gogh) 8782575 merge bitcoin#25176: Fix frequent -netinfo JSON errors from missing getpeerinfo#relaytxes (Kittywhiskers Van Gogh) 1d96a47 merge bitcoin#25119: move StartExtraBlockRelayPeers() from header to implementation (Kittywhiskers Van Gogh) 37e0c58 merge bitcoin#24468: improve -onlynet help and related tor/i2p documentation (Kittywhiskers Van Gogh) 52c3b03 merge bitcoin#23542: open p2p connections to nodes that listen on non-default ports (Kittywhiskers Van Gogh) 8e2a12a merge bitcoin#22732: use m_client_interface rather than uiInterface (Kittywhiskers Van Gogh) 06a8e9c merge bitcoin#21845: Don't require locking cs_main before calling RelayTransactions() (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Due to changes introduced in [bitcoin#21845](bitcoin#21845), a `LOCK(cs_main)` had to be added to `PeerManagerImpl::ReattemptInitialBroadcast()` ([source](06a8e9c#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1634)). This addition is in line with upstream ([source](https://github.com/bitcoin/bitcoin/blob/39e19713cd6594f93db835e8ef7eef5824a9ba02/src/net_processing.cpp#L1021)). * While nodes have been allowed to connect to non-default ports since [dash#2168](#2168), [bitcoin#23542](bitcoin#23542) also adds a list of ports considered "bad" that while not outright prohibited, are heavily discouraged from use as they are considered _de facto_ prohibited. In combination with port restrictions for masternodes (connections only permitted if matching listening port), port validation logic was better served by implementing it in a lambda block that's immediately executed (see `is_prohibited_port` for more information, [source](https://github.com/dashpay/dash/blob/01975fba32b8fcd8bccb0ce293b217c07b522c53/src/net.cpp#L3551-L3567)). * In [bitcoin#25176](bitcoin#25176), `is_block_relay` is renamed to `is_tx_relay` as the block relay-only = not transaction-relay assumption no longer holds true (see [dash#6365](#6365) for more information). One use of `is_block_relay` which relied on this now-obsolete assumption is incrementing `m_block_relay_peers_count`. It has been replaced with checking for a `block-relay-only` `conn_type`, matching upstream ([source](https://github.com/bitcoin/bitcoin/blob/a17c5e96b602fed65166037b78d98605e915206b/src/bitcoin-cli.cpp#L486)). ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 3260f2c PastaPastaPasta: utACK 3260f2c Tree-SHA512: 2bb50deb77ffaf7f7c4b396a8efe03f8bbfa605b49b75eace5fbc9d9813d4de8b72b086c50fbb0c23b97237c1f4c1b30b68f4b456bb74875308a4fcaa82deb08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These fixes are originally from the DIP3 branch but should be included into develop ASAP, as otherwise devnet users will have issues connecting to a devnet. See individual commits.