A few devnet related fixes #2168

codablock · 2018-07-05T13:40:40Z

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.

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

See inline comments

UdjinM6 · 2018-07-05T15:11:48Z

src/test/net_tests.cpp

@@ -92,7 +92,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read)
    // Test that the de-serialization does not throw an exception.
    CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted);
    bool exceptionThrown = false;
-    CAddrMan addrman1;
+    CAddrMan addrman1(false);


Not needed, false is the default one (same for all changes below in this file).

UdjinM6 · 2018-07-05T15:39:41Z

src/net.cpp

@@ -348,8 +348,9 @@ bool CConnman::CheckIncomingNonce(uint64_t nonce)
 CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure)
 {
    if (pszDest == NULL) {
-        if (IsLocal(addrConnect))
+        if (IsLocal(addrConnect) && (!Params().AllowMultiplePorts() || addrConnect.GetPort() == GetListenPort())) {


It's too hard to read, maybe

bool fAllowLocal = Params().AllowMultiplePorts() && addrConnect.GetPort() != GetListenPort(); if (!fAllowLocal && IsLocal(addrConnect)) {

?

UdjinM6 · 2018-07-05T15:42:12Z

src/net.cpp

+                break;
+
+            // if we selected a local address, restart (local addresses are allowed in regtest and devnet)
+            if (IsLocal(addr) && (!Params().AllowMultiplePorts() || addr.GetPort() == GetListenPort()))


same as above

UdjinM6 · 2018-07-05T16:02:29Z

src/net.cpp

            FindNode(addrConnect.ToStringIPPort()))
            return false;
+        if (IsLocal(addrConnect) && (!Params().AllowMultiplePorts() || addrConnect.GetPort() == GetListenPort()))


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;

thephez · 2018-07-06T00:05:57Z

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 addnode is used every time the node starts. It will connect to a remote peer that is running on a non-standard port if it is running on the default.

To summarize:

Local Port	Remote Port	Connection
18999	18999	No automatic re-connect (after initial `addnode`)
19999	18999	Automatic re-connect works (after initial `addnode`)

Also, a little off-topic for the PR probably, but why does -devnet require assigning -port and -rpcport when there are defaults set in chainparams? 🤔

codablock · 2018-07-06T09:47:06Z

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.

UdjinM6

utACK

UdjinM6 · 2018-07-07T21:14:41Z

Tested and it seems to work as expected.

utACK -> ACK

* 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

, 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

, 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

codablock added 3 commits July 5, 2018 15:33

Remove testnet seeds from devnet

a15e906

Lift multiple ports restriction on devnet when considering new nodes

5cfad62

Allow to connect to multiple nodes behind the same IP

Don't skip addresses with non-default port if it matches -port

2d1ecdb

If the user specified -port, he very likely intends to connect to nodes with the same port.

UdjinM6 requested changes Jul 5, 2018

View reviewed changes

UdjinM6 added the enhancement label Jul 5, 2018

UdjinM6 added this to the 12.3.2 milestone Jul 5, 2018

codablock added 2 commits July 6, 2018 11:30

Don't pass false to CAddrMan constructor as it is already the default

4b072c9

Make if statements easier to read

795f1ee

UdjinM6 approved these changes Jul 6, 2018

View reviewed changes

UdjinM6 merged commit 2c303cd into dashpay:develop Jul 7, 2018

codablock deleted the pr_devnet_fixes branch September 14, 2018 12:51

kwvg mentioned this pull request Jun 3, 2024

backport: merge bitcoin#22879, #22762, #23041, #22734, #22950, #23053, #22839, #23140, #23306, #23354, #23380 (addrman backports: part 2) #6043

Merged

5 tasks

kwvg mentioned this pull request Jan 14, 2025

backport: merge bitcoin#21845, #22732, #23542, #24468, #25119, #25176, #25421, #26248, #26199, #27036, #27270, partial bitcoin#27106 (networking backports: part 10) #6532

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A few devnet related fixes #2168

A few devnet related fixes #2168

codablock commented Jul 5, 2018

UdjinM6 left a comment

UdjinM6 Jul 5, 2018

UdjinM6 Jul 5, 2018

UdjinM6 Jul 5, 2018

UdjinM6 Jul 5, 2018

thephez commented Jul 6, 2018 •

edited

Loading

codablock commented Jul 6, 2018

UdjinM6 left a comment

UdjinM6 commented Jul 7, 2018

A few devnet related fixes #2168

A few devnet related fixes #2168

Conversation

codablock commented Jul 5, 2018

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 Jul 5, 2018

Choose a reason for hiding this comment

UdjinM6 Jul 5, 2018

Choose a reason for hiding this comment

UdjinM6 Jul 5, 2018

Choose a reason for hiding this comment

UdjinM6 Jul 5, 2018

Choose a reason for hiding this comment

thephez commented Jul 6, 2018 • edited Loading

codablock commented Jul 6, 2018

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 commented Jul 7, 2018

thephez commented Jul 6, 2018 •

edited

Loading