Fix bootstrap <-> blockprocessor notifications (#4663)

* Fix bootstrap notifications

* Move toml test

* Tests

* Condition notify

* Test return values

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)
@@ -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 };
-	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);
+	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);
 }
 
-// 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,14 @@ 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));
+	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));
 }
 
 // Ensure priority value is bounded
@@ -152,7 +158,8 @@ TEST (account_sets, saturate_priority)
 	{
 		sets.priority_up (account);
 	}
-	ASSERT_EQ (sets.priority (account), nano::bootstrap_ascending::account_sets::priority_max);
+	ASSERT_EQ (sets.priority_up (account), sets.priority_max);
+	ASSERT_EQ (sets.priority (account), sets.priority_max);
 }
 
 /**
@@ -258,31 +265,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

@@ -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

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