Fix online reps weight sampling (#4927)

nano/core_test/node.cpp

@@ -201,17 +201,23 @@ TEST (node, quick_confirm)
 	auto genesis_start_balance (node1.balance (nano::dev::genesis_key.pub));
 	system.wallet (0)->insert_adhoc (key.prv);
 	system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv);
+
+	// Wait for online weight to stabilize after wallet insertion
+	ASSERT_TIMELY (5s, node1.online_reps.online () >= genesis_start_balance);
+	auto delta = node1.online_reps.delta ();
+
 	auto send = nano::send_block_builder ()
 				.previous (previous)
 				.destination (key.pub)
-				.balance (node1.online_reps.delta () + 1)
+				.balance (delta + 1)
 				.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
 				.work (*system.work.generate (previous))
 				.build ();
 	node1.process_active (send);
 	ASSERT_TIMELY (10s, !node1.balance (key.pub).is_zero ());
-	ASSERT_EQ (node1.balance (nano::dev::genesis_key.pub), node1.online_reps.delta () + 1);
+
-	ASSERT_EQ (node1.balance (key.pub), genesis_start_balance - (node1.online_reps.delta () + 1));
+	ASSERT_EQ (node1.balance (nano::dev::genesis_key.pub), delta + 1);
+	ASSERT_EQ (node1.balance (key.pub), genesis_start_balance - (delta + 1));
 }
 
 TEST (node, node_receive_quorum)

nano/core_test/online_reps.cpp

@@ -238,7 +238,7 @@ TEST (online_reps, observe_slow)
 	ASSERT_TRUE (nano::test::process (node, { send1, send2, open1, open2 }));
 	nano::test::confirm (node, { send1, send2, open1, open2 });
 
-	ASSERT_EQ (node.active.size (), 0);
+	ASSERT_TIMELY_EQ (5s, node.active.size (), 0);
 
 	// Add a block that we can vote on
 	auto send_dummy = builder.state ()
@@ -271,4 +271,71 @@ TEST (online_reps, observe_slow)
 
 	// The slow rep weight should still be counted as online, even though it arrived slightly after the election already reached quorum
 	ASSERT_TIMELY_EQ (5s, node.online_reps.online (), weight * 2);
+}
+
+// Test that online weight recalculates when existing representative weights change
+TEST (online_reps, weight_change_recalculation)
+{
+	nano::test::system system;
+	auto & node = *system.add_node ();
+	ASSERT_EQ (0, node.online_reps.online ());
+
+	nano::keypair key1, key2;
+	auto const initial_weight = nano::nano_ratio * 1000;
+	auto const additional_weight = nano::nano_ratio * 2000;
+
+	// Create initial distribution
+	nano::block_builder builder;
+	auto send1 = builder.state ()
+				 .account (nano::dev::genesis_key.pub)
+				 .previous (nano::dev::genesis->hash ())
+				 .representative (nano::dev::genesis_key.pub)
+				 .balance (nano::dev::constants.genesis_amount - initial_weight)
+				 .link (key1.pub)
+				 .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
+				 .work (*system.work.generate (nano::dev::genesis->hash ()))
+				 .build ();
+	auto open1 = builder.state ()
+				 .account (key1.pub)
+				 .previous (0)
+				 .representative (key1.pub)
+				 .balance (initial_weight)
+				 .link (send1->hash ())
+				 .sign (key1.prv, key1.pub)
+				 .work (*system.work.generate (key1.pub))
+				 .build ();
+	ASSERT_TRUE (nano::test::process (node, { send1, open1 }));
+
+	// Observe the representative - should register initial weight
+	node.online_reps.observe (key1.pub);
+	ASSERT_EQ (initial_weight, node.online_reps.online ());
+
+	// Create additional weight delegation to the same representative
+	auto send2 = builder.state ()
+				 .account (nano::dev::genesis_key.pub)
+				 .previous (send1->hash ())
+				 .representative (nano::dev::genesis_key.pub)
+				 .balance (nano::dev::constants.genesis_amount - initial_weight - additional_weight)
+				 .link (key2.pub)
+				 .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
+				 .work (*system.work.generate (send1->hash ()))
+				 .build ();
+	auto open2 = builder.state ()
+				 .account (key2.pub)
+				 .previous (0)
+				 .representative (key1.pub) // Delegate to key1
+				 .balance (additional_weight)
+				 .link (send2->hash ())
+				 .sign (key2.prv, key2.pub)
+				 .work (*system.work.generate (key2.pub))
+				 .build ();
+	ASSERT_TRUE (nano::test::process (node, { send2, open2 }));
+
+	// Observe the same representative again (simulating a vote from existing rep)
+	// This should trigger recalculation and pick up the new weight
+	node.online_reps.observe (key1.pub);
+
+	// The bug was that online weight would not recalculate for existing representatives
+	// With the fix, it should now reflect the updated weight
+	ASSERT_TIMELY_EQ (5s, node.online_reps.online (), initial_weight + additional_weight);
 }

nano/node/online_reps.cpp

@@ -64,18 +64,15 @@ void nano::online_reps::observe (nano::account const & rep)
 		nano::lock_guard<nano::mutex> lock{ mutex };
 
 		auto now = std::chrono::steady_clock::now ();
-		auto new_insert = reps.get<tag_account> ().erase (rep) == 0;
+		bool new_insert = reps.get<tag_account> ().erase (rep) == 0;
 		reps.insert ({ now, rep });
 
 		stats.inc (nano::stat::type::online_reps, new_insert ? nano::stat::detail::rep_new : nano::stat::detail::rep_update);
 
-		// Update current online weight if anything changed
 		if (new_insert)
 		{
-			stats.inc (nano::stat::type::online_reps, nano::stat::detail::update_online);
 			logger.debug (nano::log::type::online_reps, "Observed new representative: {}", rep.to_account ());
-
+			update_online ();
-			cached_online = calculate_online ();
 		}
 	}
 }
@@ -106,22 +103,39 @@ void nano::online_reps::trim ()
 	}
 }
 
+void nano::online_reps::update_online ()
+{
+	stats.inc (nano::stat::type::online_reps, nano::stat::detail::update_online);
+	cached_online = calculate_online ();
+}
+
 void nano::online_reps::run ()
 {
 	nano::unique_lock<nano::mutex> lock{ mutex };
+	last_sample = std::chrono::steady_clock::now ();
 	while (!stopped)
 	{
-		// Set next time point explicitly to ensure that we don't sample too early
+		condition.wait_for (lock, nano::is_dev_run () ? 100ms : 1s, [this] () {
-		auto next = std::chrono::steady_clock::now () + config.network_params.node.weight_interval;
+			return stopped;
-		condition.wait_until (lock, next, [this, next] {
-			return stopped || std::chrono::steady_clock::now () >= next;
 		});
-		if (!stopped)
+		if (stopped)
+		{
+			return;
+		}
+
+		// Always recalculate online weight
+		update_online ();
+
+		// Sample trended weight if the next sample time has been reached
+		auto const now = std::chrono::steady_clock::now ();
+		auto const next_sample = last_sample + config.network_params.node.weight_interval;
+		if (now >= next_sample)
 		{
 			trim ();
 			lock.unlock ();
 			sample ();
 			lock.lock ();
+			last_sample = now;
 		}
 	}
 }

nano/node/online_reps.hpp

@@ -64,6 +64,7 @@ private:
 	void trim_trended (nano::store::write_transaction const &);
 	/** Iterate over all database samples and remove invalid records. This is meant to clean potential leftovers from previous versions. */
 	void sanitize_trended (nano::store::write_transaction const &);
+	void update_online ();
 
 	struct trended_result
 	{
@@ -99,6 +100,8 @@ private:
 	nano::uint128_t cached_trended{0};
 	nano::uint128_t cached_online{0};
 
+	std::chrono::steady_clock::time_point last_sample;
+
 	bool stopped{ false };
 	nano::condition_variable condition;
 	mutable nano::mutex mutex;