From cb1893a424d407f18a009e682e902d8ea525ea59 Mon Sep 17 00:00:00 2001 From: Colin LeMahieu Date: Tue, 9 Apr 2024 15:51:12 +0100 Subject: [PATCH 1/2] Fix race condition from TSAN where block_sideband is updated by different nodes. --- nano/core_test/node.cpp | 4 ++-- nano/lib/blockbuilders.cpp | 15 +++++++++++++++ nano/lib/blockbuilders.hpp | 2 ++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 0b7e049bad..15b0788592 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -647,14 +647,14 @@ TEST (node, fork_keep) .work (*system.work.generate (nano::dev::genesis->hash ())) .build (); node1.process_active (send1); - node2.process_active (send1); + node2.process_active (builder.make_block ().from (*send1).build ()); ASSERT_TIMELY_EQ (5s, 1, node1.active.size ()); ASSERT_TIMELY_EQ (5s, 1, node2.active.size ()); system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv); // Fill node with forked blocks node1.process_active (send2); ASSERT_TIMELY (5s, node1.active.active (*send2)); - node2.process_active (send2); + node2.process_active (builder.make_block ().from (*send2).build ()); ASSERT_TIMELY (5s, node2.active.active (*send2)); auto election1 (node2.active.election (nano::qualified_root (nano::dev::genesis->hash (), nano::dev::genesis->hash ()))); ASSERT_NE (nullptr, election1); diff --git a/nano/lib/blockbuilders.cpp b/nano/lib/blockbuilders.cpp index d8fa600f8f..034abbe3af 100644 --- a/nano/lib/blockbuilders.cpp +++ b/nano/lib/blockbuilders.cpp @@ -516,6 +516,21 @@ nano::send_block_builder::send_block_builder () make_block (); } +nano::send_block_builder & nano::send_block_builder::from (nano::send_block const & other_block) +{ + block->work = other_block.work; + build_state |= build_flags::work_present; + block->signature = other_block.signature; + build_state |= build_flags::signature_present; + block->hashables.balance = other_block.hashables.balance; + build_state |= build_flags::balance_present; + block->hashables.destination = other_block.hashables.destination; + build_state |= build_flags::link_present; + block->hashables.previous = other_block.hashables.previous; + build_state |= build_flags::previous_present; + return *this; +} + nano::send_block_builder & nano::send_block_builder::make_block () { construct_block (); diff --git a/nano/lib/blockbuilders.hpp b/nano/lib/blockbuilders.hpp index 1471c9bb20..6f625c330b 100644 --- a/nano/lib/blockbuilders.hpp +++ b/nano/lib/blockbuilders.hpp @@ -199,6 +199,8 @@ class send_block_builder : public abstract_builder Date: Tue, 9 Apr 2024 17:27:44 +0100 Subject: [PATCH 2/2] Fix peer_container.tcp_channel_cleanup_works test that was changing config values after node construction. --- nano/core_test/peer_container.cpp | 11 +++++++---- nano/test_common/network.cpp | 10 +++++++++- nano/test_common/network.hpp | 3 +++ 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/nano/core_test/peer_container.cpp b/nano/core_test/peer_container.cpp index c0f04d9d91..b55150d583 100644 --- a/nano/core_test/peer_container.cpp +++ b/nano/core_test/peer_container.cpp @@ -68,10 +68,13 @@ TEST (peer_container, tcp_channel_cleanup_works) // Disable the confirm_req messages avoiding them to affect the last_packet_set time node_flags.disable_rep_crawler = true; auto & node1 = *system.add_node (node_config, node_flags); - auto outer_node1 = nano::test::add_outer_node (system, node_flags); - outer_node1->config.network_params.network.keepalive_period = std::chrono::minutes (10); - auto outer_node2 = nano::test::add_outer_node (system, node_flags); - outer_node2->config.network_params.network.keepalive_period = std::chrono::minutes (10); + + auto config1 = node_config; + config1.network_params.network.keepalive_period = std::chrono::minutes (10); + auto outer_node1 = nano::test::add_outer_node (system, config1, node_flags); + auto config2 = config1; + config2.network_params.network.keepalive_period = std::chrono::minutes (10); + auto outer_node2 = nano::test::add_outer_node (system, config2, node_flags); auto now = std::chrono::steady_clock::now (); auto channel1 = nano::test::establish_tcp (system, node1, outer_node1->network.endpoint ()); ASSERT_NE (nullptr, channel1); diff --git a/nano/test_common/network.cpp b/nano/test_common/network.cpp index 80ec2065c0..5ca9bcf41e 100644 --- a/nano/test_common/network.cpp +++ b/nano/test_common/network.cpp @@ -24,6 +24,14 @@ std::shared_ptr nano::test::establish_tcp (nano::t return result; } +std::shared_ptr nano::test::add_outer_node (nano::test::system & system_a, nano::node_config const & config_a, nano::node_flags flags_a) +{ + auto outer_node = std::make_shared (system_a.io_ctx, nano::unique_path (), config_a, system_a.work, flags_a); + outer_node->start (); + system_a.nodes.push_back (outer_node); + return outer_node; +} + std::shared_ptr nano::test::add_outer_node (nano::test::system & system_a, nano::node_flags flags_a) { auto outer_node = std::make_shared (system_a.io_ctx, system_a.get_available_port (), nano::unique_path (), system_a.work, flags_a); @@ -55,4 +63,4 @@ uint16_t nano::test::speculatively_choose_a_free_tcp_bind_port () acceptor.close (); return port; -} \ No newline at end of file +} diff --git a/nano/test_common/network.hpp b/nano/test_common/network.hpp index c8835139fe..db90344f56 100644 --- a/nano/test_common/network.hpp +++ b/nano/test_common/network.hpp @@ -19,6 +19,9 @@ namespace test /** Waits until a TCP connection is established and returns the TCP channel on success*/ std::shared_ptr establish_tcp (nano::test::system &, nano::node &, nano::endpoint const &); + /** Adds a node to the system without establishing connections */ + std::shared_ptr add_outer_node (nano::test::system & system, nano::node_config const & config_a, nano::node_flags = nano::node_flags ()); + /** Adds a node to the system without establishing connections */ std::shared_ptr add_outer_node (nano::test::system & system, nano::node_flags = nano::node_flags ());