Squash merge PR nanocurrency#4663: Fix bootstrap <-> blockprocessor n…

…otifications

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);
+}

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);
 }

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

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,25 +26,30 @@ void nano::bootstrap_ascending::account_sets::priority_up (nano::account const &
 		auto iter = priorities.get<tag_account> ().find (account);
 		if (iter != priorities.get<tag_account> ().end ())
 		{
-			priorities.get<tag_account> ().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<tag_account> ().modify (iter, [priority_new] (auto & val) {
+				val.priority = priority_new;
 			});
+			return priority_new;
 		}
 		else
 		{
 			priorities.get<tag_account> ().insert ({ account, account_sets::priority_initial });
 			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<tag_account> ().find (account);
 	if (iter != priorities.get<tag_account> ().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

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<nano::block_hash> const & hash = std::nullopt);
 		void timestamp (nano::account const & account, bool reset = false);

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<nano::mutex> lock{ mutex };
+	return accounts.blocked (account);
+}
+
+float nano::bootstrap_ascending::service::priority (nano::account const & account) const
+{
+	nano::lock_guard<nano::mutex> 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<nano::mutex> 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; });
 	}
 }