Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix vote_spacing logic #4840

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Fix vote_spacing logic #4840

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d2811db
fix(vote_generator): Properly spaces votes for same block while allow…
Feb 5, 2025
d7db86d
Add comment why vote spacing is skipped for replies
Feb 5, 2025
eedb696
Cleanup
pwojcikdev Feb 16, 2025
7278daa
Organize tests
pwojcikdev Feb 16, 2025
b6548d7
Allow overriding current time
pwojcikdev Feb 16, 2025
0bc0a2c
Use qualified root for vote tracking
pwojcikdev Feb 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 20 additions & 11 deletions nano/core_test/voting.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ TEST (vote_spacing, basic)
ASSERT_TRUE (spacing.votable (root1, hash3));
spacing.flag (root1, hash3);
ASSERT_EQ (1, spacing.size ());
ASSERT_TRUE (spacing.votable (root1, hash3));
ASSERT_FALSE (spacing.votable (root1, hash4));
ASSERT_FALSE (spacing.votable (root1, hash3));
ASSERT_TRUE (spacing.votable (root1, hash4));
spacing.flag (root2, hash5);
ASSERT_EQ (2, spacing.size ());
}
Expand Down Expand Up @@ -176,15 +176,16 @@ TEST (vote_spacing, vote_generator)
ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send1));
ASSERT_EQ (0, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
node.generator.add (nano::dev::genesis->hash (), send1->hash ());
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts), 1);
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));

ASSERT_FALSE (node.ledger.rollback (node.ledger.tx_begin_write (), send1->hash ()));
ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send2));
node.generator.add (nano::dev::genesis->hash (), send2->hash ());
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing), 1);
ASSERT_EQ (1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
std::this_thread::sleep_for (config.network_params.voting.delay);
ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
// vote on send2 second time (rapid succession) - expect spacing
node.generator.add (nano::dev::genesis->hash (), send2->hash ());
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts), 2);
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing));
ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
}

TEST (vote_spacing, rapid)
Expand Down Expand Up @@ -219,13 +220,21 @@ TEST (vote_spacing, rapid)
.build ();
ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send1));
node.generator.add (nano::dev::genesis->hash (), send1->hash ());
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts), 1);
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
ASSERT_FALSE (node.ledger.rollback (node.ledger.tx_begin_write (), send1->hash ()));
ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send2));
// vote on send2 first time - expect : new broadcast & no spacing
node.generator.add (nano::dev::genesis->hash (), send2->hash ());
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing), 1);
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
ASSERT_TIMELY_EQ (3s, 0, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing));

// vote on send2 again rapidly - expect : no broadcast & new spacing
node.generator.add (nano::dev::genesis->hash (), send2->hash ());
ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing));
std::this_thread::sleep_for (config.network_params.voting.delay);
// vote on send2 again after the spacing delay - expect : new broadcast & no spacing
node.generator.add (nano::dev::genesis->hash (), send2->hash ());
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts), 2);
ASSERT_TIMELY_EQ (3s, 3, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing));
}
11 changes: 2 additions & 9 deletions nano/node/vote_generator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,15 +230,8 @@ void nano::vote_generator::reply (nano::unique_lock<nano::mutex> & lock_a, reque
auto const & [root, hash] = *i;
if (std::find (roots.begin (), roots.end (), root) == roots.end ())
{
if (spacing.votable (root, hash))
{
roots.push_back (root);
hashes.push_back (hash);
}
else
{
stats.inc (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing);
}
roots.push_back (root);
hashes.push_back (hash);
}
}
if (!hashes.empty ())
Expand Down
17 changes: 12 additions & 5 deletions nano/node/vote_spacing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,16 @@ void nano::vote_spacing::trim ()

bool nano::vote_spacing::votable (nano::root const & root_a, nano::block_hash const & hash_a) const
{
bool result = true;
for (auto range = recent.get<tag_root> ().equal_range (root_a); result && range.first != range.second; ++range.first)
auto now = std::chrono::steady_clock::now ();
for (auto range = recent.get<tag_root> ().equal_range (root_a); range.first != range.second; ++range.first)
{
auto & item = *range.first;
result = hash_a == item.hash || item.time < std::chrono::steady_clock::now () - delay;
if (hash_a == item.hash && item.time >= now - delay)
{
return false; // Same hash and not expired -> not votable
}
}
return result;
return true; // Either different hash or expired -> votable
}

void nano::vote_spacing::flag (nano::root const & root_a, nano::block_hash const & hash_a)
Expand All @@ -23,8 +26,12 @@ void nano::vote_spacing::flag (nano::root const & root_a, nano::block_hash const
auto existing = recent.get<tag_root> ().find (root_a);
if (existing != recent.end ())
{
recent.get<tag_root> ().modify (existing, [now] (entry & entry) {
// We update both timestamp and hash because we want to track which fork
// we most recently voted for. This ensures proper spacing between votes
// for the same block while allowing immediate votes for competing forks.
recent.get<tag_root> ().modify (existing, [now, hash_a] (entry & entry) {
entry.time = now;
entry.hash = hash_a;
});
}
else
Expand Down
Loading