Fix online reps weight sampling (#4927)
This commit is contained in:
		
					parent
					
						
							
								aa281cfaf0
							
						
					
				
			
			
				commit
				
					
						7a69c29288
					
				
			
		
					 4 changed files with 104 additions and 14 deletions
				
			
		|  | @ -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) | ||||
|  |  | |||
|  | @ -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 () | ||||
|  | @ -272,3 +272,70 @@ 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); | ||||
| } | ||||
|  | @ -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 ()); | ||||
| 
 | ||||
| 			cached_online = calculate_online (); | ||||
| 			update_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
 | ||||
| 		auto next = std::chrono::steady_clock::now () + config.network_params.node.weight_interval; | ||||
| 		condition.wait_until (lock, next, [this, next] { | ||||
| 			return stopped || std::chrono::steady_clock::now () >= next; | ||||
| 		condition.wait_for (lock, nano::is_dev_run () ? 100ms : 1s, [this] () { | ||||
| 			return stopped; | ||||
| 		}); | ||||
| 		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; | ||||
| 		} | ||||
| 	} | ||||
| } | ||||
|  |  | |||
|  | @ -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; | ||||
|  |  | |||
		Loading…
	
	Add table
		Add a link
		
	
		Reference in a new issue
	
	 Piotr Wójcik
				Piotr Wójcik