From 7248c816541d83ad33528dd650513c2a0a44e95c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 6 Jul 2024 20:39:39 +0200 Subject: [PATCH 1/2] Revert "Fix bootstrap <-> blockprocessor notifications (#4663)" This reverts commit 0615aca556b8abeb3d485073c94a24e30d934e33. --- nano/core_test/bootstrap_ascending.cpp | 137 +++++------------- nano/core_test/toml.cpp | 29 ---- nano/node/blockprocessor.cpp | 4 +- .../node/bootstrap_ascending/account_sets.cpp | 17 +-- .../node/bootstrap_ascending/account_sets.hpp | 4 +- nano/node/bootstrap_ascending/service.cpp | 51 +++---- nano/node/bootstrap_ascending/service.hpp | 27 +++- nano/node/fwd.hpp | 2 - 8 files changed, 81 insertions(+), 190 deletions(-) diff --git a/nano/core_test/bootstrap_ascending.cpp b/nano/core_test/bootstrap_ascending.cpp index 43da04ba1f..be946cb209 100644 --- a/nano/core_test/bootstrap_ascending.cpp +++ b/nano/core_test/bootstrap_ascending.cpp @@ -78,7 +78,7 @@ TEST (account_sets, priority_base) auto store = nano::make_store (system.logger, nano::unique_path (), nano::dev::constants); ASSERT_FALSE (store->init_error ()); nano::bootstrap_ascending::account_sets sets{ system.stats }; - ASSERT_EQ (0.0f, sets.priority (account)); + ASSERT_EQ (1.0f, sets.priority (account)); } TEST (account_sets, priority_blocked) @@ -120,13 +120,13 @@ TEST (account_sets, priority_up_down) auto store = nano::make_store (system.logger, nano::unique_path (), nano::dev::constants); ASSERT_FALSE (store->init_error ()); nano::bootstrap_ascending::account_sets sets{ system.stats }; - ASSERT_EQ (sets.priority_up (account), sets.priority_initial); - ASSERT_EQ (sets.priority (account), sets.priority_initial); - ASSERT_EQ (sets.priority_down (account), sets.priority_initial - sets.priority_decrease); - ASSERT_EQ (sets.priority (account), sets.priority_initial - sets.priority_decrease); + sets.priority_up (account); + ASSERT_EQ (sets.priority (account), nano::bootstrap_ascending::account_sets::priority_initial); + sets.priority_down (account); + ASSERT_EQ (sets.priority (account), nano::bootstrap_ascending::account_sets::priority_initial - nano::bootstrap_ascending::account_sets::priority_decrease); } -// Check that priority downward saturates to 0.0f +// Check that priority downward saturates to 1.0f TEST (account_sets, priority_down_sat) { nano::test::system system; @@ -135,14 +135,8 @@ TEST (account_sets, priority_down_sat) auto store = nano::make_store (system.logger, nano::unique_path (), nano::dev::constants); ASSERT_FALSE (store->init_error ()); nano::bootstrap_ascending::account_sets sets{ system.stats }; - ASSERT_EQ (sets.priority_up (account), sets.priority_initial); - ASSERT_GT (sets.priority (account), 1.0f); - for (int n = 0; n < 1000; ++n) - { - sets.priority_down (account); - } - ASSERT_EQ (sets.priority_down (account), 0.0f); - ASSERT_EQ (0.0f, sets.priority (account)); + sets.priority_down (account); + ASSERT_EQ (1.0f, sets.priority (account)); } // Ensure priority value is bounded @@ -158,8 +152,7 @@ TEST (account_sets, saturate_priority) { sets.priority_up (account); } - ASSERT_EQ (sets.priority_up (account), sets.priority_max); - ASSERT_EQ (sets.priority (account), sets.priority_max); + ASSERT_EQ (sets.priority (account), nano::bootstrap_ascending::account_sets::priority_max); } /** @@ -265,91 +258,31 @@ TEST (bootstrap_ascending, trace_base) ASSERT_TIMELY (10s, node1.block (receive1->hash ()) != nullptr); } -/* - * Tests that bootstrap won't try to immediately bootstrap chains that have active live traffic - */ -TEST (bootstrap_ascending, ignore_live) +TEST (bootstrap_ascending, config_serialization) { - nano::node_flags flags; - flags.disable_legacy_bootstrap = true; - nano::test::system system; - auto & node = *system.add_node (flags); - nano::keypair key; - nano::state_block_builder builder; - auto send1 = builder.make_block () - .account (nano::dev::genesis_key.pub) - .previous (nano::dev::genesis->hash ()) - .representative (nano::dev::genesis_key.pub) - .link (key.pub) - .balance (nano::dev::constants.genesis_amount - 1) - .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) - .work (*system.work.generate (nano::dev::genesis->hash ())) - .build (); - auto receive1 = builder.make_block () - .account (key.pub) - .previous (0) - .representative (nano::dev::genesis_key.pub) - .link (send1->hash ()) - .balance (1) - .sign (key.prv, key.pub) - .work (*system.work.generate (key.pub)) - .build (); - auto send2 = builder.make_block () - .account (key.pub) - .previous (receive1->hash ()) - .representative (nano::dev::genesis_key.pub) - .link (key.pub) - .balance (0) - .sign (key.prv, key.pub) - .work (*system.work.generate (receive1->hash ())) - .build (); - - // Genesis is inserted into the priority set by default, test with another chain - ASSERT_EQ (nano::block_status::progress, node.block_processor.add_blocking (send1, nano::block_source::live)); - ASSERT_EQ (node.ascendboot.priority (key.pub), 0.0f); - - ASSERT_EQ (nano::block_status::progress, node.block_processor.add_blocking (receive1, nano::block_source::live)); - ASSERT_EQ (node.ascendboot.priority (key.pub), 0.0f); - - // This should trigger insertion of the account into the priority set - ASSERT_EQ (nano::block_status::progress, node.block_processor.add_blocking (send2, nano::block_source::bootstrap)); - ASSERT_GE (node.ascendboot.priority (key.pub), 1.0f); + nano::bootstrap_ascending_config config1; + config1.requests_limit = 0x101; + config1.database_requests_limit = 0x102; + config1.pull_count = 0x103; + config1.timeout = 0x104; + config1.throttle_coefficient = 0x105; + config1.throttle_wait = 0x106; + config1.block_wait_count = 0x107; + nano::tomlconfig toml1; + ASSERT_FALSE (config1.serialize (toml1)); + std::stringstream stream1; + toml1.write (stream1); + auto string = stream1.str (); + std::stringstream stream2{ string }; + nano::tomlconfig toml2; + toml2.read (stream2); + nano::bootstrap_ascending_config config2; + ASSERT_FALSE (config2.deserialize (toml2)); + ASSERT_EQ (config1.requests_limit, config2.requests_limit); + ASSERT_EQ (config1.database_requests_limit, config2.database_requests_limit); + ASSERT_EQ (config1.pull_count, config2.pull_count); + ASSERT_EQ (config1.timeout, config2.timeout); + ASSERT_EQ (config1.throttle_coefficient, config2.throttle_coefficient); + ASSERT_EQ (config1.throttle_wait, config2.throttle_wait); + ASSERT_EQ (config1.block_wait_count, config2.block_wait_count); } - -/* - * Tests that source blocked account can be unblocked by live traffic (or any other sources) - */ -TEST (bootstrap_ascending, unblock_live) -{ - nano::node_flags flags; - flags.disable_legacy_bootstrap = true; - nano::test::system system; - auto & node = *system.add_node (flags); - nano::keypair key; - nano::state_block_builder builder; - auto send1 = builder.make_block () - .account (nano::dev::genesis_key.pub) - .previous (nano::dev::genesis->hash ()) - .representative (nano::dev::genesis_key.pub) - .link (key.pub) - .balance (nano::dev::constants.genesis_amount - 1) - .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) - .work (*system.work.generate (nano::dev::genesis->hash ())) - .build (); - auto receive1 = builder.make_block () - .account (key.pub) - .previous (0) - .representative (nano::dev::genesis_key.pub) - .link (send1->hash ()) - .balance (1) - .sign (key.prv, key.pub) - .work (*system.work.generate (key.pub)) - .build (); - - ASSERT_EQ (nano::block_status::gap_source, node.block_processor.add_blocking (receive1, nano::block_source::bootstrap)); - ASSERT_TRUE (node.ascendboot.blocked (key.pub)); - - ASSERT_EQ (nano::block_status::progress, node.block_processor.add_blocking (send1, nano::block_source::live)); - ASSERT_FALSE (node.ascendboot.blocked (key.pub)); - ASSERT_TRUE (node.ascendboot.priority (key.pub) > 0); -} \ No newline at end of file diff --git a/nano/core_test/toml.cpp b/nano/core_test/toml.cpp index 93f224ad98..43a950e7d2 100644 --- a/nano/core_test/toml.cpp +++ b/nano/core_test/toml.cpp @@ -1073,33 +1073,4 @@ TEST (toml, merge_config_files) ASSERT_NE (merged_config.node.backlog_scan_batch_size, 7777); ASSERT_EQ (merged_config.node.bootstrap_ascending.block_wait_count, 33333); ASSERT_TRUE (merged_config_string.find ("old_entry") == std::string::npos); -} - -TEST (toml, bootstrap_ascending_config) -{ - nano::bootstrap_ascending_config config1; - config1.requests_limit = 0x101; - config1.database_requests_limit = 0x102; - config1.pull_count = 0x103; - config1.timeout = 0x104; - config1.throttle_coefficient = 0x105; - config1.throttle_wait = 0x106; - config1.block_wait_count = 0x107; - nano::tomlconfig toml1; - ASSERT_FALSE (config1.serialize (toml1)); - std::stringstream stream1; - toml1.write (stream1); - auto string = stream1.str (); - std::stringstream stream2{ string }; - nano::tomlconfig toml2; - toml2.read (stream2); - nano::bootstrap_ascending_config config2; - ASSERT_FALSE (config2.deserialize (toml2)); - ASSERT_EQ (config1.requests_limit, config2.requests_limit); - ASSERT_EQ (config1.database_requests_limit, config2.database_requests_limit); - ASSERT_EQ (config1.pull_count, config2.pull_count); - ASSERT_EQ (config1.timeout, config2.timeout); - ASSERT_EQ (config1.throttle_coefficient, config2.throttle_coefficient); - ASSERT_EQ (config1.throttle_wait, config2.throttle_wait); - ASSERT_EQ (config1.block_wait_count, config2.block_wait_count); } \ No newline at end of file diff --git a/nano/node/blockprocessor.cpp b/nano/node/blockprocessor.cpp index 5ff33eace3..4c36cd60d0 100644 --- a/nano/node/blockprocessor.cpp +++ b/nano/node/blockprocessor.cpp @@ -242,14 +242,14 @@ void nano::block_processor::run () auto processed = process_batch (lock); debug_assert (!lock.owns_lock ()); - batch_processed.notify (processed); - // Set results for futures when not holding the lock for (auto & [result, context] : processed) { context.set_result (result); } + batch_processed.notify (processed); + lock.lock (); } else diff --git a/nano/node/bootstrap_ascending/account_sets.cpp b/nano/node/bootstrap_ascending/account_sets.cpp index 6c79d6516b..1348c5bebc 100644 --- a/nano/node/bootstrap_ascending/account_sets.cpp +++ b/nano/node/bootstrap_ascending/account_sets.cpp @@ -17,7 +17,7 @@ nano::bootstrap_ascending::account_sets::account_sets (nano::stats & stats_a, na { } -float nano::bootstrap_ascending::account_sets::priority_up (nano::account const & account) +void nano::bootstrap_ascending::account_sets::priority_up (nano::account const & account) { if (!blocked (account)) { @@ -26,11 +26,9 @@ float nano::bootstrap_ascending::account_sets::priority_up (nano::account const auto iter = priorities.get ().find (account); if (iter != priorities.get ().end ()) { - auto priority_new = std::min ((iter->priority * account_sets::priority_increase), account_sets::priority_max); - priorities.get ().modify (iter, [priority_new] (auto & val) { - val.priority = priority_new; + priorities.get ().modify (iter, [] (auto & val) { + val.priority = std::min ((val.priority * account_sets::priority_increase), account_sets::priority_max); }); - return priority_new; } else { @@ -38,18 +36,15 @@ float nano::bootstrap_ascending::account_sets::priority_up (nano::account const stats.inc (nano::stat::type::bootstrap_ascending_accounts, nano::stat::detail::priority_insert); trim_overflow (); - - return account_sets::priority_initial; } } else { stats.inc (nano::stat::type::bootstrap_ascending_accounts, nano::stat::detail::prioritize_failed); } - return 0.0f; } -float nano::bootstrap_ascending::account_sets::priority_down (nano::account const & account) +void nano::bootstrap_ascending::account_sets::priority_down (nano::account const & account) { auto iter = priorities.get ().find (account); if (iter != priorities.get ().end ()) @@ -68,13 +63,11 @@ float nano::bootstrap_ascending::account_sets::priority_down (nano::account cons val.priority = priority_new; }); } - return priority_new; } else { stats.inc (nano::stat::type::bootstrap_ascending_accounts, nano::stat::detail::deprioritize_failed); } - return 0.0f; } void nano::bootstrap_ascending::account_sets::block (nano::account const & account, nano::block_hash const & dependency) @@ -233,7 +226,7 @@ float nano::bootstrap_ascending::account_sets::priority (nano::account const & a { return existing->priority; } - return 0.0f; + return account_sets::priority_cutoff; } auto nano::bootstrap_ascending::account_sets::info () const -> nano::bootstrap_ascending::account_sets::info_t diff --git a/nano/node/bootstrap_ascending/account_sets.hpp b/nano/node/bootstrap_ascending/account_sets.hpp index f45531c6ca..ebed04a465 100644 --- a/nano/node/bootstrap_ascending/account_sets.hpp +++ b/nano/node/bootstrap_ascending/account_sets.hpp @@ -33,12 +33,12 @@ namespace bootstrap_ascending * If the account does not exist in priority set and is not blocked, inserts a new entry. * Current implementation increases priority by 1.0f each increment */ - float priority_up (nano::account const & account); + void priority_up (nano::account const & account); /** * Decreases account priority * Current implementation divides priority by 2.0f and saturates down to 1.0f. */ - float priority_down (nano::account const & account); + void priority_down (nano::account const & account); void block (nano::account const & account, nano::block_hash const & dependency); void unblock (nano::account const & account, std::optional const & hash = std::nullopt); void timestamp (nano::account const & account, bool reset = false); diff --git a/nano/node/bootstrap_ascending/service.cpp b/nano/node/bootstrap_ascending/service.cpp index cbf1b8928f..7d295b3215 100644 --- a/nano/node/bootstrap_ascending/service.cpp +++ b/nano/node/bootstrap_ascending/service.cpp @@ -32,17 +32,26 @@ nano::bootstrap_ascending::service::service (nano::node_config & config_a, nano: database_limiter{ config.bootstrap_ascending.database_requests_limit, 1.0 } { block_processor.batch_processed.add ([this] (auto const & batch) { + bool should_notify = false; { nano::lock_guard lock{ mutex }; auto transaction = ledger.tx_begin_read (); for (auto const & [result, context] : batch) { - release_assert (context.block != nullptr); - inspect (transaction, result, *context.block, context.source); + // Do not try to unnecessarily bootstrap live traffic chains + if (context.source == nano::block_source::bootstrap) + { + release_assert (context.block != nullptr); + inspect (transaction, result, *context.block); + should_notify = true; + } } } - condition.notify_all (); + if (should_notify) + { + condition.notify_all (); + } }); } @@ -122,23 +131,11 @@ std::size_t nano::bootstrap_ascending::service::score_size () const return scoring.size (); } -bool nano::bootstrap_ascending::service::blocked (nano::account const & account) const -{ - nano::lock_guard lock{ mutex }; - return accounts.blocked (account); -} - -float nano::bootstrap_ascending::service::priority (nano::account const & account) const -{ - nano::lock_guard lock{ mutex }; - return accounts.priority (account); -} - /** Inspects a block that has been processed by the block processor - Marks an account as blocked if the result code is gap source as there is no reason request additional blocks for this account until the dependency is resolved - Marks an account as forwarded if it has been recently referenced by a block that has been inserted. */ -void nano::bootstrap_ascending::service::inspect (secure::transaction const & tx, nano::block_status const & result, nano::block const & block, nano::block_source const & block_source) +void nano::bootstrap_ascending::service::inspect (secure::transaction const & tx, nano::block_status const & result, nano::block const & block) { auto const hash = block.hash (); @@ -150,23 +147,14 @@ void nano::bootstrap_ascending::service::inspect (secure::transaction const & tx // If we've inserted any block in to an account, unmark it as blocked accounts.unblock (account); - - // Do not try to unnecessarily bootstrap live traffic chains - if (block_source == nano::block_source::bootstrap) - { - accounts.priority_up (account); - accounts.timestamp (account, /* reset timestamp */ true); - } + accounts.priority_up (account); + accounts.timestamp (account, /* reset timestamp */ true); if (block.is_send ()) { auto destination = block.destination (); accounts.unblock (destination, hash); // Unblocking automatically inserts account into priority set - - if (block_source == nano::block_source::bootstrap) - { - accounts.priority_up (destination); - } + accounts.priority_up (destination); } } break; @@ -176,10 +164,7 @@ void nano::bootstrap_ascending::service::inspect (secure::transaction const & tx const auto source = block.source_field ().value_or (block.link_field ().value_or (0).as_block_hash ()); // Mark account as blocked because it is missing the source block - if (block_source == nano::block_source::bootstrap) - { - accounts.block (account, source); - } + accounts.block (account, source); // TODO: Track stats } @@ -204,7 +189,7 @@ void nano::bootstrap_ascending::service::wait_blockprocessor () nano::unique_lock lock{ mutex }; while (!stopped && block_processor.size (nano::block_source::bootstrap) > config.bootstrap_ascending.block_wait_count) { - condition.wait_for (lock, std::chrono::milliseconds{ config.bootstrap_ascending.throttle_wait }, [this] () { return stopped; }); + condition.wait_for (lock, std::chrono::milliseconds{ config.bootstrap_ascending.throttle_wait }, [this] () { return stopped; }); // Blockprocessor is relatively slow, sleeping here instead of using conditions } } diff --git a/nano/node/bootstrap_ascending/service.hpp b/nano/node/bootstrap_ascending/service.hpp index 6738d810c0..ccb2ea66cd 100644 --- a/nano/node/bootstrap_ascending/service.hpp +++ b/nano/node/bootstrap_ascending/service.hpp @@ -11,7 +11,6 @@ #include #include #include -#include #include #include @@ -22,8 +21,23 @@ namespace mi = boost::multi_index; +namespace nano::secure +{ +class transaction; +} + namespace nano { +class block_processor; +class ledger; +class network; +class node_config; + +namespace transport +{ + class channel; +} + namespace bootstrap_ascending { class service @@ -38,17 +52,14 @@ namespace bootstrap_ascending /** * Process `asc_pull_ack` message coming from network */ - void process (nano::asc_pull_ack const & message, std::shared_ptr); + void process (nano::asc_pull_ack const & message, std::shared_ptr channel); + public: // Container info + std::unique_ptr collect_container_info (std::string const & name); std::size_t blocked_size () const; std::size_t priority_size () const; std::size_t score_size () const; - bool blocked (nano::account const &) const; - float priority (nano::account const &) const; - - std::unique_ptr collect_container_info (std::string const & name); - private: // Dependencies nano::node_config & config; nano::network_constants & network_consts; @@ -82,7 +93,7 @@ namespace bootstrap_ascending private: /* Inspects a block that has been processed by the block processor */ - void inspect (nano::secure::transaction const &, nano::block_status const &, nano::block const &, nano::block_source const &); + void inspect (secure::transaction const &, nano::block_status const & result, nano::block const & block); void throttle_if_needed (nano::unique_lock & lock); void run (); diff --git a/nano/node/fwd.hpp b/nano/node/fwd.hpp index fb1ba3316b..4a0f903a8a 100644 --- a/nano/node/fwd.hpp +++ b/nano/node/fwd.hpp @@ -7,7 +7,6 @@ namespace nano { class active_elections; -class block_processor; class confirming_set; class ledger; class local_block_broadcaster; @@ -29,6 +28,5 @@ class vote_processor; class vote_router; class wallets; -enum class block_source; enum class vote_code; } \ No newline at end of file From c314893f3fc9bf81b78ff2cc6fc4c9427f185974 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 6 Jul 2024 20:40:06 +0200 Subject: [PATCH 2/2] Revert "Avoid bootstrap notifications from live traffic (#4463)" This reverts commit ebbc881ebbd9a1599afee35aeaf8893486c32563. --- nano/node/bootstrap_ascending/service.cpp | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/nano/node/bootstrap_ascending/service.cpp b/nano/node/bootstrap_ascending/service.cpp index 7d295b3215..8bceda97d3 100644 --- a/nano/node/bootstrap_ascending/service.cpp +++ b/nano/node/bootstrap_ascending/service.cpp @@ -31,27 +31,20 @@ nano::bootstrap_ascending::service::service (nano::node_config & config_a, nano: scoring{ config.bootstrap_ascending, config.network_params.network }, database_limiter{ config.bootstrap_ascending.database_requests_limit, 1.0 } { + // TODO: This is called from a very congested blockprocessor thread. Offload this work to a dedicated processing thread block_processor.batch_processed.add ([this] (auto const & batch) { - bool should_notify = false; { nano::lock_guard lock{ mutex }; auto transaction = ledger.tx_begin_read (); for (auto const & [result, context] : batch) { - // Do not try to unnecessarily bootstrap live traffic chains - if (context.source == nano::block_source::bootstrap) - { - release_assert (context.block != nullptr); - inspect (transaction, result, *context.block); - should_notify = true; - } + debug_assert (context.block != nullptr); + inspect (transaction, result, *context.block); } } - if (should_notify) - { - condition.notify_all (); - } + + condition.notify_all (); }); }