From 4f7fb384bc204b854a4ef157cb069f552f61dd5a Mon Sep 17 00:00:00 2001 From: gr0vity-dev Date: Tue, 2 Jul 2024 08:21:38 +0200 Subject: [PATCH] Squash merge PR #4663: Fix bootstrap <-> blockprocessor notifications --- nano/core_test/bootstrap_ascending.cpp | 125 +++++++++++++----- 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 | 38 +++++- nano/node/bootstrap_ascending/service.hpp | 27 ++-- nano/node/fwd.hpp | 2 + 8 files changed, 181 insertions(+), 65 deletions(-) diff --git a/nano/core_test/bootstrap_ascending.cpp b/nano/core_test/bootstrap_ascending.cpp index be946cb209..946df51c38 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 (1.0f, sets.priority (account)); + ASSERT_EQ (0.0f, sets.priority (account)); } TEST (account_sets, priority_blocked) @@ -126,7 +126,7 @@ TEST (account_sets, priority_up_down) 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 1.0f +// Check that priority downward saturates to 0.0f TEST (account_sets, priority_down_sat) { nano::test::system system; @@ -135,8 +135,13 @@ 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 }; - sets.priority_down (account); - ASSERT_EQ (1.0f, sets.priority (account)); + sets.priority_up (account); + ASSERT_GT (sets.priority (account), 1.0f); + for (int n = 0; n < 1000; ++n) + { + sets.priority_down (account); + } + ASSERT_EQ (0.0f, sets.priority (account)); } // Ensure priority value is bounded @@ -258,31 +263,91 @@ TEST (bootstrap_ascending, trace_base) ASSERT_TIMELY (10s, node1.block (receive1->hash ()) != nullptr); } -TEST (bootstrap_ascending, config_serialization) +/* + * Tests that bootstrap won't try to immediately bootstrap chains that have active live traffic + */ +TEST (bootstrap_ascending, ignore_live) { - 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); + 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); } + +/* + * 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 43a950e7d2..93f224ad98 100644 --- a/nano/core_test/toml.cpp +++ b/nano/core_test/toml.cpp @@ -1073,4 +1073,33 @@ 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 123ab5aeae..96974f47fd 100644 --- a/nano/node/blockprocessor.cpp +++ b/nano/node/blockprocessor.cpp @@ -248,14 +248,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 1348c5bebc..6c79d6516b 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 { } -void nano::bootstrap_ascending::account_sets::priority_up (nano::account const & account) +float nano::bootstrap_ascending::account_sets::priority_up (nano::account const & account) { if (!blocked (account)) { @@ -26,9 +26,11 @@ void nano::bootstrap_ascending::account_sets::priority_up (nano::account const & auto iter = priorities.get ().find (account); if (iter != priorities.get ().end ()) { - priorities.get ().modify (iter, [] (auto & val) { - val.priority = std::min ((val.priority * account_sets::priority_increase), account_sets::priority_max); + 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; }); + return priority_new; } else { @@ -36,15 +38,18 @@ void 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; } -void nano::bootstrap_ascending::account_sets::priority_down (nano::account const & account) +float nano::bootstrap_ascending::account_sets::priority_down (nano::account const & account) { auto iter = priorities.get ().find (account); if (iter != priorities.get ().end ()) @@ -63,11 +68,13 @@ void nano::bootstrap_ascending::account_sets::priority_down (nano::account const 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) @@ -226,7 +233,7 @@ float nano::bootstrap_ascending::account_sets::priority (nano::account const & a { return existing->priority; } - return account_sets::priority_cutoff; + return 0.0f; } 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 ebed04a465..f45531c6ca 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 */ - void priority_up (nano::account const & account); + float priority_up (nano::account const & account); /** * Decreases account priority * Current implementation divides priority by 2.0f and saturates down to 1.0f. */ - void priority_down (nano::account const & account); + float 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 445c62e3dc..f1ea3933b1 100644 --- a/nano/node/bootstrap_ascending/service.cpp +++ b/nano/node/bootstrap_ascending/service.cpp @@ -42,7 +42,7 @@ nano::bootstrap_ascending::service::service (nano::node_config & config_a, nano: if (context.source == nano::block_source::bootstrap) { release_assert (context.block != nullptr); - inspect (transaction, result, *context.block); + inspect (transaction, result, *context.block, context.source); should_notify = true; } } @@ -130,11 +130,23 @@ 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) +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) { auto const hash = block.hash (); @@ -146,14 +158,23 @@ 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); - accounts.priority_up (account); - accounts.timestamp (account, /* reset timestamp */ true); + + // 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); + } if (block.is_send ()) { auto destination = block.destination (); accounts.unblock (destination, hash); // Unblocking automatically inserts account into priority set - accounts.priority_up (destination); + + if (block_source == nano::block_source::bootstrap) + { + accounts.priority_up (destination); + } } } break; @@ -163,7 +184,10 @@ 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 - accounts.block (account, source); + if (block_source == nano::block_source::bootstrap) + { + accounts.block (account, source); + } // TODO: Track stats } @@ -188,7 +212,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; }); // Blockprocessor is relatively slow, sleeping here instead of using conditions + condition.wait_for (lock, std::chrono::milliseconds{ config.bootstrap_ascending.throttle_wait }, [this] () { return stopped; }); } } diff --git a/nano/node/bootstrap_ascending/service.hpp b/nano/node/bootstrap_ascending/service.hpp index ccb2ea66cd..6738d810c0 100644 --- a/nano/node/bootstrap_ascending/service.hpp +++ b/nano/node/bootstrap_ascending/service.hpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -21,23 +22,8 @@ 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 @@ -52,14 +38,17 @@ namespace bootstrap_ascending /** * Process `asc_pull_ack` message coming from network */ - void process (nano::asc_pull_ack const & message, std::shared_ptr channel); + void process (nano::asc_pull_ack const & message, std::shared_ptr); - 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; @@ -93,7 +82,7 @@ namespace bootstrap_ascending private: /* Inspects a block that has been processed by the block processor */ - void inspect (secure::transaction const &, nano::block_status const & result, nano::block const & block); + void inspect (nano::secure::transaction const &, nano::block_status const &, nano::block const &, nano::block_source const &); void throttle_if_needed (nano::unique_lock & lock); void run (); diff --git a/nano/node/fwd.hpp b/nano/node/fwd.hpp index 4a0f903a8a..14897ecd25 100644 --- a/nano/node/fwd.hpp +++ b/nano/node/fwd.hpp @@ -8,6 +8,7 @@ namespace nano { class active_elections; class confirming_set; +class block_processor; class ledger; class local_block_broadcaster; class local_vote_history; @@ -28,5 +29,6 @@ class vote_processor; class vote_router; class wallets; +enum class block_source; enum class vote_code; } \ No newline at end of file