From 9a3abd973c328f964dcaf6be030da192c96fa69d Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 10 Dec 2020 18:37:50 +0100 Subject: [PATCH 1/5] Merge #20613: test: Use Popen.wait instead of RPC in assert_start_raises_init_error fa918dd537fea775c19a590e5f9161bf51a5839b test: Use Popen.wait instead of RPC in assert_start_raises_init_error (MarcoFalke) Pull request description: Using RPC (`wait_for_rpc_connection`) has several issue: * It polls in a loop, which might be slow * It tries to read the RPC cookie file, which might not be present, thus leading to intermittent issues Fix both by using `Popen.wait` ACKs for top commit: laanwj: Code review ACK ~~faf7b05be9c86ee61c39e5314511fe2410128a6b~~ fa918dd537fea775c19a590e5f9161bf51a5839b darosior: ACK fa918dd537fea775c19a590e5f9161bf51a5839b Tree-SHA512: 5368ad0d0ea2deb0af9582a42667c9290efe8f2705f37a236afc2c7908b04265ab342e2dd356a57156e99389f4a27ab6da9fa7bf9161fb7568240aa005e693b9 --- test/functional/test_framework/test_node.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 1f7123e446..26cc2a2bb1 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -490,11 +490,8 @@ class TestNode(): tempfile.NamedTemporaryFile(dir=self.stdout_dir, delete=False) as log_stdout: try: self.start(extra_args, stdout=log_stdout, stderr=log_stderr, *args, **kwargs) - self.wait_for_rpc_connection() - self.stop_node() - self.wait_until_stopped() - except FailedToStartError as e: - self.log.debug('dashd failed to start: %s', e) + ret = self.process.wait(timeout=self.rpc_timeout) + self.log.debug(self._node_msg(f'dashd exited with status {ret} during initialization')) self.running = False self.process = None # Check stderr for expected message @@ -513,11 +510,15 @@ class TestNode(): if expected_msg != stderr: self._raise_assertion_error( 'Expected message "{}" does not fully match stderr:\n"{}"'.format(expected_msg, stderr)) - else: + except subprocess.TimeoutExpired: + self.process.kill() + self.running = False + self.process = None + assert_msg = f'dashd should have exited within {self.rpc_timeout}s ' if expected_msg is None: - assert_msg = "dashd should have exited with an error" + assert_msg += "with an error" else: - assert_msg = "dashd should have exited with expected error " + expected_msg + assert_msg += "with expected error " + expected_msg self._raise_assertion_error(assert_msg) def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs): From 2111c7727b7f2c95ebb5b3364142b89372e7b0f8 Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 14 Dec 2020 09:56:04 +0800 Subject: [PATCH 2/5] Merge #20617: p2p: Remove m_is_manual_connection from CNodeState a33442fdc73eabd1c5596ab92954344edc9517e6 Remove m_is_manual_connection from CNodeState (Antoine Riard) Pull request description: Currently, this member is only used to exclude MANUAL peers from discouragement in MaybePunishNodeForBlock(). Manual connections are already protected in MaybeDiscourageAndDisconnect(), independently from their network processing behaviors. ACKs for top commit: MarcoFalke: cr ACK a33442fdc73eabd1c5596ab92954344edc9517e6 promag: Code review ACK a33442fdc73eabd1c5596ab92954344edc9517e6. jnewbery: utACK a33442fdc73eabd1c5596ab92954344edc9517e6 amitiuttarwar: code review ACK a33442fdc73eabd1c5596ab92954344edc9517e6 Tree-SHA512: cfe3f3dfa131373e3299002d34ae9e22ca6e1a966831bab32fcf06ff1d08f06095b4ab020cc4d267f3ec05ae23fbdc22373382ab828b999c0db11b8c842a4f0c --- src/net_processing.cpp | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1a26996d90..7f2918bea0 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -670,11 +670,8 @@ struct CNodeState { //! Whether this peer is an inbound connection bool m_is_inbound; - //! Whether this peer is a manual connection - bool m_is_manual_connection; - - CNodeState(CAddress addrIn, bool is_inbound, bool is_manual) : - address(addrIn), m_is_inbound(is_inbound), m_is_manual_connection(is_manual) + CNodeState(CAddress addrIn, bool is_inbound) : + address(addrIn), m_is_inbound(is_inbound) { pindexBestKnownBlock = nullptr; hashLastUnknownBlock.SetNull(); @@ -1159,7 +1156,7 @@ void PeerManagerImpl::InitializeNode(CNode *pnode) { NodeId nodeid = pnode->GetId(); { LOCK(cs_main); - mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, pnode->fInbound, pnode->m_manual_connection)); + mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, pnode->fInbound)); } { PeerRef peer = std::make_shared(nodeid); @@ -1475,8 +1472,8 @@ bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati } // Discourage outbound (but not inbound) peers if on an invalid chain. - // Exempt HB compact block peers and manual connections. - if (!via_compact_block && !node_state->m_is_inbound && !node_state->m_is_manual_connection) { + // Exempt HB compact block peers. Manual connections are always protected from discouragement. + if (!via_compact_block && !node_state->m_is_inbound) { Misbehaving(nodeid, 100, message); return true; } From 45399b96a798a46871a6394197787927f49a8289 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 16 Dec 2020 15:47:15 +0100 Subject: [PATCH 3/5] Merge #20569: test: Fix intermittent wallet_multiwallet issue with got_loading_error fab48da908f3f81135b9163edf5011d1e5f6ef6e test: Fix intermittent wallet_multiwallet issue with got_loading_error (MarcoFalke) fa8e15f7b75e35846b86e8627a3612e31eb22dcb test: pep8 wallet_multiwallet.py (MarcoFalke) Pull request description: Failing the test after 10 iterations without a loading error is problematic because it may take 11 iterations to get a loading error. Fix that by running until a loading error occurs, which should happen in almost all runs within the first 10 iterations. ACKs for top commit: ryanofsky: Code review ACK fab48da908f3f81135b9163edf5011d1e5f6ef6e. This seems like a good workaround. I think more ideally think load and unload RPCs would not have racy status reporting (suggested previously https://github.com/bitcoin/bitcoin/pull/19300#pullrequestreview-435362710 and Tree-SHA512: 6b80b26d916276efe2a01af93bca7dbf71a3e67db9d3deb15175070719bf7d1325a1410d93e74c0316942e388faa2ba185dc9d3759c82d1c73c3c509b9997f05 --- test/functional/wallet_multiwallet.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index f9c98caba5..ca06d07f8c 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -25,9 +25,10 @@ FEATURE_LATEST = 120200 got_loading_error = False + def test_load_unload(node, name): global got_loading_error - for i in range(10): + while True: if got_loading_error: return try: @@ -67,7 +68,7 @@ class MultiWalletTest(BitcoinTestFramework): return wallet_dir(name, self.wallet_data_filename) return wallet_dir(name) - assert_equal(self.nodes[0].listwalletdir(), { 'wallets': [{ 'name': self.default_wallet_name }] }) + assert_equal(self.nodes[0].listwalletdir(), {'wallets': [{'name': self.default_wallet_name}]}) # check wallet.dat is created self.stop_nodes() @@ -261,7 +262,7 @@ class MultiWalletTest(BitcoinTestFramework): threads = [] for _ in range(3): n = node.cli if self.options.usecli else get_rpc_proxy(node.url, 1, timeout=600, coveragedir=node.coverage_dir) - t = Thread(target=test_load_unload, args=(n, wallet_names[2], )) + t = Thread(target=test_load_unload, args=(n, wallet_names[2])) t.start() threads.append(t) for t in threads: From b1e0b6c1f585306d6ea974ac2789073654e32934 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 17 Dec 2020 12:05:55 +0100 Subject: [PATCH 4/5] Merge #20668: doc: warn that incoming conns are unlikely when not using default ports 010eed3ce03cf4fc622a48f40fc4d589383f7a44 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes #5150. This was mostly copied from #5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3ce03cf4fc622a48f40fc4d589383f7a44 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f --- src/init.cpp | 2 +- src/net.cpp | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 8fe3fa9828..87e55a47b2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -596,7 +596,7 @@ void SetupServerArgs(NodeContext& node) argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-peertimeout=", strprintf("Specify p2p connection timeout in seconds. This option determines the amount of time a peer may be inactive before the connection to it is dropped. (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); - argsman.AddArg("-port=", strprintf("Listen for connections on (default: %u, testnet: %u, regtest: %u). Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); + argsman.AddArg("-port=", strprintf("Listen for connections on . Nodes not using the default ports (default: %u, testnet: %u, regtest: %u) are unlikely to get incoming connections. Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); argsman.AddArg("-proxy=", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-proxyrandomize", strprintf("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)", DEFAULT_PROXYRANDOMIZE), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-seednode=", "Connect to a node to retrieve peer addresses, and disconnect. This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); diff --git a/src/net.cpp b/src/net.cpp index 71843e8ded..e3e0e8c15f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2385,7 +2385,11 @@ void CConnman::ThreadOpenConnections(const std::vector connect) continue; } - // do not allow non-default ports, unless after 50 invalid addresses selected already + // Do not allow non-default ports, unless after 50 invalid + // addresses selected already. This is to prevent malicious peers + // from advertising themselves as a service on another host and + // port, causing a DoS attack as nodes around the network attempt + // to connect to it fruitlessly. if ((!isMasternode || !Params().AllowMultiplePorts()) && addr.GetPort() != Params().GetDefaultPort(addr.GetNetwork()) && addr.GetPort() != GetListenPort() && nTries < 50) { continue; } From 2e8800cdc1ecbd60a08c1d359a18d130d8c5d7d7 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 17 Dec 2020 19:47:25 +0100 Subject: [PATCH 5/5] Merge #20686: fuzz, refactor: replace CNode code with fuzz/util.h::ConsumeNode() 23d8f346896c806581189c9eb870c7833c09f5be fuzz: replace CNode code with fuzz/util.h::ConsumeNode() (Jon Atack) Pull request description: Noticed this while updating the CNode fuzzing in #20210. - cc26fab48d76a813d798657b18ae1af08a301150 created `test/fuzz/net.cpp` in May 2020 - 79ef8324d4c85ed16a304e98805724b8a created a CNode factory utility `test/fuzz/util.h::ConsumeNode()` in October 2020 This PR updates `fuzz/net.cpp` from the first commit to use `ConsumeNode()` from the second commit. ACKs for top commit: MarcoFalke: ACK 23d8f346896c806581189c9eb870c7833c09f5be Tree-SHA512: 26f7685395b3d48fcf40dde0d479d5c2fb4e953ec9371940b19eee16bb30aee4840b081e1a918b924a9704c1bef484302ea3e8fe63819a3bba73e7eb805164f1 --- src/test/fuzz/net.cpp | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 56ecab8d51..285f68a913 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -29,27 +29,7 @@ FUZZ_TARGET_INIT(net, initialize_net) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); - const std::optional address = ConsumeDeserializable(fuzzed_data_provider); - if (!address) { - return; - } - const std::optional address_bind = ConsumeDeserializable(fuzzed_data_provider); - if (!address_bind) { - return; - } - - CNode node{fuzzed_data_provider.ConsumeIntegral(), - static_cast(fuzzed_data_provider.ConsumeIntegral()), - INVALID_SOCKET, - *address, - fuzzed_data_provider.ConsumeIntegral(), - fuzzed_data_provider.ConsumeIntegral(), - *address_bind, - fuzzed_data_provider.ConsumeRandomLengthString(32), - fuzzed_data_provider.ConsumeBool(), - fuzzed_data_provider.ConsumeBool(), - fuzzed_data_provider.ConsumeBool() - }; + CNode node{ConsumeNode(fuzzed_data_provider)}; while (fuzzed_data_provider.ConsumeBool()) { CallOneOf( fuzzed_data_provider,