From 3a27dd95652014ae68c2b58139e4fbc30ffa13cf Mon Sep 17 00:00:00 2001 From: gr0vity-dev Date: Fri, 29 Sep 2023 23:02:43 +0200 Subject: [PATCH 1/4] Refactor active_transactions vote processing logic - Decomposed the `vote` method into smaller methods. - Separated hash categorisation, inactive vote handling, vote processing, and republishing logic for clarity and maintainability. - Enhanced readability by reducing the complexity of the primary vote processing function. --- nano/node/active_transactions.cpp | 103 +++++++++++++++++------------- nano/node/active_transactions.hpp | 5 ++ 2 files changed, 64 insertions(+), 44 deletions(-) diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index 05972cdc66..ce72b67650 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -448,66 +448,81 @@ nano::election_insertion_result nano::active_transactions::insert_impl (nano::un // Validate a vote and apply it to the current election if one exists nano::vote_code nano::active_transactions::vote (std::shared_ptr const & vote_a) { - nano::vote_code result{ nano::vote_code::indeterminate }; - // If all hashes were recently confirmed then it is a replay - unsigned recently_confirmed_counter (0); - std::vector, nano::block_hash>> process; - std::vector inactive; // Hashes that should be added to inactive vote cache + std::vector inactive; + unsigned recently_confirmed_counter = categorize_hashes (vote_a, process, inactive); + handle_inactive_votes (inactive, vote_a); + auto result = process_votes (process, vote_a); + if (result == nano::vote_code::indeterminate && is_replay (vote_a, recently_confirmed_counter)) { - nano::unique_lock lock{ mutex }; - for (auto const & hash : vote_a->hashes) + result = nano::vote_code::replay; + } + return result; +} + +unsigned nano::active_transactions::categorize_hashes (std::shared_ptr const & vote_a, std::vector, nano::block_hash>> & process, std::vector & inactive) +{ + unsigned recently_confirmed_counter = 0; + nano::unique_lock lock{ mutex }; + for (auto const & hash : vote_a->hashes) + { + auto existing = blocks.find (hash); + if (existing != blocks.end ()) { - auto existing (blocks.find (hash)); - if (existing != blocks.end ()) - { - process.emplace_back (existing->second, hash); - } - else if (!recently_confirmed.exists (hash)) - { - inactive.emplace_back (hash); - } - else - { - ++recently_confirmed_counter; - } + process.emplace_back (existing->second, hash); + } + else if (!recently_confirmed.exists (hash)) + { + inactive.emplace_back (hash); + } + else + { + ++recently_confirmed_counter; } } + return recently_confirmed_counter; +} - // Process inactive votes outside of the critical section +void nano::active_transactions::handle_inactive_votes (std::vector & inactive, std::shared_ptr const & vote_a) +{ for (auto & hash : inactive) { add_inactive_vote_cache (hash, vote_a); } +} - if (!process.empty ()) - { - bool replay (false); - bool processed (false); - for (auto const & [election, block_hash] : process) - { - auto const result_l = election->vote (vote_a->account, vote_a->timestamp (), block_hash); - processed = processed || result_l.processed; - replay = replay || result_l.replay; - } +nano::vote_code nano::active_transactions::process_votes (std::vector, nano::block_hash>> & process, std::shared_ptr const & vote_a) +{ + if (process.empty ()) + return nano::vote_code::indeterminate; - // Republish vote if it is new and the node does not host a principal representative (or close to) - if (processed) - { - auto const reps (node.wallets.reps ()); - if (!reps.have_half_rep () && !reps.exists (vote_a->account)) - { - node.network.flood_vote (vote_a, 0.5f); - } - } - result = replay ? nano::vote_code::replay : nano::vote_code::vote; + bool replay = false; + bool processed = false; + for (auto const & [election, block_hash] : process) + { + auto const result = election->vote (vote_a->account, vote_a->timestamp (), block_hash); + processed = processed || result.processed; + replay = replay || result.replay; } - else if (recently_confirmed_counter == vote_a->hashes.size ()) + + if (processed) + republish_vote_if_needed (vote_a); + return replay ? nano::vote_code::replay : nano::vote_code::vote; +} + +void nano::active_transactions::republish_vote_if_needed (std::shared_ptr const & vote_a) +{ + auto const reps = node.wallets.reps (); + if (!reps.have_half_rep () && !reps.exists (vote_a->account)) { - result = nano::vote_code::replay; + node.network.flood_vote (vote_a, 0.5f); } - return result; +} + +bool nano::active_transactions::is_replay (std::shared_ptr const & vote_a, unsigned recently_confirmed_counter) +{ + return recently_confirmed_counter == vote_a->hashes.size (); } bool nano::active_transactions::active (nano::qualified_root const & root_a) const diff --git a/nano/node/active_transactions.hpp b/nano/node/active_transactions.hpp index bf54078db6..25c5b3cd44 100644 --- a/nano/node/active_transactions.hpp +++ b/nano/node/active_transactions.hpp @@ -197,6 +197,11 @@ class active_transactions final * TODO: Should be moved to `vote_cache` class */ void add_inactive_vote_cache (nano::block_hash const & hash, std::shared_ptr vote); + unsigned categorize_hashes (std::shared_ptr const &, std::vector, nano::block_hash>> &, std::vector &); + void handle_inactive_votes (std::vector &, std::shared_ptr const &); + nano::vote_code process_votes (std::vector, nano::block_hash>> &, std::shared_ptr const &); + void republish_vote_if_needed (std::shared_ptr const &); + bool is_replay (std::shared_ptr const &, unsigned recently_confirmed_counter); private: // Dependencies nano::confirmation_height_processor & confirmation_height_processor; From 0a4194bdba91778eeabd8eea8611e1ee0857bacd Mon Sep 17 00:00:00 2001 From: gr0vity-dev Date: Mon, 2 Oct 2023 10:17:10 +0200 Subject: [PATCH 2/4] Refactor vote processing in active_transactions - Rename variables for clarity: `process` to `active_elections_hashes`, and `inactive` to `inactive_hashes`. - Simplify hash categorization in `categorize_hashes` by removing the recently_confirmed_counter logic. - Introduce `no_new_votes_present` method to check for replay votes based on recent confirmations. --- nano/node/active_transactions.cpp | 31 ++++++++++++++++--------------- nano/node/active_transactions.hpp | 5 +++-- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index ce72b67650..f736ed9df0 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -448,22 +448,21 @@ nano::election_insertion_result nano::active_transactions::insert_impl (nano::un // Validate a vote and apply it to the current election if one exists nano::vote_code nano::active_transactions::vote (std::shared_ptr const & vote_a) { - std::vector, nano::block_hash>> process; - std::vector inactive; + std::vector, nano::block_hash>> active_elections_hashes; + std::vector inactive_hashes; - unsigned recently_confirmed_counter = categorize_hashes (vote_a, process, inactive); - handle_inactive_votes (inactive, vote_a); - auto result = process_votes (process, vote_a); - if (result == nano::vote_code::indeterminate && is_replay (vote_a, recently_confirmed_counter)) + categorize_hashes (vote_a, active_elections_hashes, inactive_hashes); + handle_inactive_votes (inactive_hashes, vote_a); + auto result = process_votes (active_elections_hashes, vote_a); + if (result == nano::vote_code::indeterminate && no_new_votes_present (vote_a)) { result = nano::vote_code::replay; } return result; } -unsigned nano::active_transactions::categorize_hashes (std::shared_ptr const & vote_a, std::vector, nano::block_hash>> & process, std::vector & inactive) +void nano::active_transactions::categorize_hashes (std::shared_ptr const & vote_a, std::vector, nano::block_hash>> & process, std::vector & inactive) { - unsigned recently_confirmed_counter = 0; nano::unique_lock lock{ mutex }; for (auto const & hash : vote_a->hashes) { @@ -476,12 +475,7 @@ unsigned nano::active_transactions::categorize_hashes (std::shared_ptr & inactive, std::shared_ptr const & vote_a) @@ -520,9 +514,16 @@ void nano::active_transactions::republish_vote_if_needed (std::shared_ptr const & vote_a, unsigned recently_confirmed_counter) +bool nano::active_transactions::no_new_votes_present (std::shared_ptr const & vote_a) { - return recently_confirmed_counter == vote_a->hashes.size (); + for (const auto & hash : vote_a->hashes) + { + if (!recently_confirmed.exists(hash)) + { + return false; // At least one hash is not recently confirmed, so it's not a replay. + } + } + return true; // All hashes have been recently confirmed, it's a replay. } bool nano::active_transactions::active (nano::qualified_root const & root_a) const diff --git a/nano/node/active_transactions.hpp b/nano/node/active_transactions.hpp index 25c5b3cd44..6103a34fb7 100644 --- a/nano/node/active_transactions.hpp +++ b/nano/node/active_transactions.hpp @@ -197,11 +197,12 @@ class active_transactions final * TODO: Should be moved to `vote_cache` class */ void add_inactive_vote_cache (nano::block_hash const & hash, std::shared_ptr vote); - unsigned categorize_hashes (std::shared_ptr const &, std::vector, nano::block_hash>> &, std::vector &); + void categorize_hashes (std::shared_ptr const &, std::vector, nano::block_hash>> &, std::vector &); void handle_inactive_votes (std::vector &, std::shared_ptr const &); nano::vote_code process_votes (std::vector, nano::block_hash>> &, std::shared_ptr const &); void republish_vote_if_needed (std::shared_ptr const &); - bool is_replay (std::shared_ptr const &, unsigned recently_confirmed_counter); + bool no_new_votes_present (std::shared_ptr const &); + private: // Dependencies nano::confirmation_height_processor & confirmation_height_processor; From e0635899c5497923c37b752d10c84bfde6b5d1a9 Mon Sep 17 00:00:00 2001 From: gr0vity-dev Date: Mon, 2 Oct 2023 10:51:42 +0200 Subject: [PATCH 3/4] Refactor vote processing logic for better readability and maintainability - Renamed `process_votes` method to `handle_active_votes` to better convey its responsibility. - Integrated the handling of `nano::vote_code::indeterminate` and subsequent replay check directly within the `handle_active_votes` method. --- nano/node/active_transactions.cpp | 40 ++++++++++++++++++------------- nano/node/active_transactions.hpp | 2 +- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index f736ed9df0..411d59feee 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -453,11 +453,8 @@ nano::vote_code nano::active_transactions::vote (std::shared_ptr con categorize_hashes (vote_a, active_elections_hashes, inactive_hashes); handle_inactive_votes (inactive_hashes, vote_a); - auto result = process_votes (active_elections_hashes, vote_a); - if (result == nano::vote_code::indeterminate && no_new_votes_present (vote_a)) - { - result = nano::vote_code::replay; - } + auto result = handle_active_votes(active_elections_hashes, vote_a); + return result; } @@ -486,23 +483,32 @@ void nano::active_transactions::handle_inactive_votes (std::vector, nano::block_hash>> & process, std::shared_ptr const & vote_a) +nano::vote_code nano::active_transactions::handle_active_votes (std::vector, nano::block_hash>> & process, std::shared_ptr const & vote_a) { - if (process.empty ()) - return nano::vote_code::indeterminate; + auto result = nano::vote_code::indeterminate; - bool replay = false; - bool processed = false; - for (auto const & [election, block_hash] : process) + if (!process.empty ()) { - auto const result = election->vote (vote_a->account, vote_a->timestamp (), block_hash); - processed = processed || result.processed; - replay = replay || result.replay; + bool replay = false; + bool processed = false; + for (auto const & [election, block_hash] : process) + { + auto const result = election->vote (vote_a->account, vote_a->timestamp (), block_hash); + processed = processed || result.processed; + replay = replay || result.replay; + } + + if (processed) + republish_vote_if_needed (vote_a); + + result = replay ? nano::vote_code::replay : nano::vote_code::vote; } - if (processed) - republish_vote_if_needed (vote_a); - return replay ? nano::vote_code::replay : nano::vote_code::vote; + if (result == nano::vote_code::indeterminate && no_new_votes_present (vote_a)) + { + result = nano::vote_code::replay; + } + return result ; } void nano::active_transactions::republish_vote_if_needed (std::shared_ptr const & vote_a) diff --git a/nano/node/active_transactions.hpp b/nano/node/active_transactions.hpp index 6103a34fb7..90f6a6a757 100644 --- a/nano/node/active_transactions.hpp +++ b/nano/node/active_transactions.hpp @@ -199,9 +199,9 @@ class active_transactions final void add_inactive_vote_cache (nano::block_hash const & hash, std::shared_ptr vote); void categorize_hashes (std::shared_ptr const &, std::vector, nano::block_hash>> &, std::vector &); void handle_inactive_votes (std::vector &, std::shared_ptr const &); - nano::vote_code process_votes (std::vector, nano::block_hash>> &, std::shared_ptr const &); void republish_vote_if_needed (std::shared_ptr const &); bool no_new_votes_present (std::shared_ptr const &); + nano::vote_code handle_active_votes(std::vector, nano::block_hash>> & process, std::shared_ptr const & vote_a); private: // Dependencies From ad309d6f2330cdfe1ab6ffc19086ece3b1554868 Mon Sep 17 00:00:00 2001 From: gr0vity-dev Date: Mon, 2 Oct 2023 10:56:53 +0200 Subject: [PATCH 4/4] formatting - rename `process` into `active_elections_hashes` - rename `inactive`into `inactive_hashes` --- nano/node/active_transactions.cpp | 34 +++++++++++++++---------------- nano/node/active_transactions.hpp | 3 +-- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index 411d59feee..8db8b56560 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -453,12 +453,12 @@ nano::vote_code nano::active_transactions::vote (std::shared_ptr con categorize_hashes (vote_a, active_elections_hashes, inactive_hashes); handle_inactive_votes (inactive_hashes, vote_a); - auto result = handle_active_votes(active_elections_hashes, vote_a); + auto result = handle_active_votes (active_elections_hashes, vote_a); return result; } -void nano::active_transactions::categorize_hashes (std::shared_ptr const & vote_a, std::vector, nano::block_hash>> & process, std::vector & inactive) +void nano::active_transactions::categorize_hashes (std::shared_ptr const & vote_a, std::vector, nano::block_hash>> & active_elections_hashes, std::vector & inactive_hashes) { nano::unique_lock lock{ mutex }; for (auto const & hash : vote_a->hashes) @@ -466,32 +466,32 @@ void nano::active_transactions::categorize_hashes (std::shared_ptr c auto existing = blocks.find (hash); if (existing != blocks.end ()) { - process.emplace_back (existing->second, hash); + active_elections_hashes.emplace_back (existing->second, hash); } else if (!recently_confirmed.exists (hash)) { - inactive.emplace_back (hash); + inactive_hashes.emplace_back (hash); } } } -void nano::active_transactions::handle_inactive_votes (std::vector & inactive, std::shared_ptr const & vote_a) +void nano::active_transactions::handle_inactive_votes (std::vector & inactive_hashes, std::shared_ptr const & vote_a) { - for (auto & hash : inactive) + for (auto & hash : inactive_hashes) { add_inactive_vote_cache (hash, vote_a); } } -nano::vote_code nano::active_transactions::handle_active_votes (std::vector, nano::block_hash>> & process, std::shared_ptr const & vote_a) +nano::vote_code nano::active_transactions::handle_active_votes (std::vector, nano::block_hash>> & active_elections_hashes, std::shared_ptr const & vote_a) { auto result = nano::vote_code::indeterminate; - if (!process.empty ()) + if (!active_elections_hashes.empty ()) { bool replay = false; bool processed = false; - for (auto const & [election, block_hash] : process) + for (auto const & [election, block_hash] : active_elections_hashes) { auto const result = election->vote (vote_a->account, vote_a->timestamp (), block_hash); processed = processed || result.processed; @@ -508,7 +508,7 @@ nano::vote_code nano::active_transactions::handle_active_votes (std::vector const & vote_a) @@ -523,13 +523,13 @@ void nano::active_transactions::republish_vote_if_needed (std::shared_ptr const & vote_a) { for (const auto & hash : vote_a->hashes) - { - if (!recently_confirmed.exists(hash)) - { - return false; // At least one hash is not recently confirmed, so it's not a replay. - } - } - return true; // All hashes have been recently confirmed, it's a replay. + { + if (!recently_confirmed.exists (hash)) + { + return false; // At least one hash is not recently confirmed, so it's not a replay. + } + } + return true; // All hashes have been recently confirmed, it's a replay. } bool nano::active_transactions::active (nano::qualified_root const & root_a) const diff --git a/nano/node/active_transactions.hpp b/nano/node/active_transactions.hpp index 90f6a6a757..522df4b1c6 100644 --- a/nano/node/active_transactions.hpp +++ b/nano/node/active_transactions.hpp @@ -199,10 +199,9 @@ class active_transactions final void add_inactive_vote_cache (nano::block_hash const & hash, std::shared_ptr vote); void categorize_hashes (std::shared_ptr const &, std::vector, nano::block_hash>> &, std::vector &); void handle_inactive_votes (std::vector &, std::shared_ptr const &); + nano::vote_code handle_active_votes (std::vector, nano::block_hash>> & process, std::shared_ptr const & vote_a); void republish_vote_if_needed (std::shared_ptr const &); bool no_new_votes_present (std::shared_ptr const &); - nano::vote_code handle_active_votes(std::vector, nano::block_hash>> & process, std::shared_ptr const & vote_a); - private: // Dependencies nano::confirmation_height_processor & confirmation_height_processor;