From 7a69c29288ef7d36b278dafe87e57574e85fdb5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20W=C3=B3jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 12 Jul 2025 11:22:33 +0200 Subject: [PATCH] Fix online reps weight sampling (#4927) --- nano/core_test/node.cpp | 12 ++++-- nano/core_test/online_reps.cpp | 69 +++++++++++++++++++++++++++++++++- nano/node/online_reps.cpp | 34 ++++++++++++----- nano/node/online_reps.hpp | 3 ++ 4 files changed, 104 insertions(+), 14 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 0e45225c..3f0c4933 100644 --- a/nano/core_test/node.cpp +++ b/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) diff --git a/nano/core_test/online_reps.cpp b/nano/core_test/online_reps.cpp index 8ef4a62b..cf9ab599 100644 --- a/nano/core_test/online_reps.cpp +++ b/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); } \ No newline at end of file diff --git a/nano/node/online_reps.cpp b/nano/node/online_reps.cpp index a7be75d2..59ad87e0 100644 --- a/nano/node/online_reps.cpp +++ b/nano/node/online_reps.cpp @@ -64,18 +64,15 @@ void nano::online_reps::observe (nano::account const & rep) nano::lock_guard lock{ mutex }; auto now = std::chrono::steady_clock::now (); - auto new_insert = reps.get ().erase (rep) == 0; + bool new_insert = reps.get ().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 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; } } } diff --git a/nano/node/online_reps.hpp b/nano/node/online_reps.hpp index 61c0f5e5..f27e634b 100644 --- a/nano/node/online_reps.hpp +++ b/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;