From 5186a225060c6d6a59bdb12e20b61dd77f71ae8c Mon Sep 17 00:00:00 2001 From: clemahieu Date: Tue, 22 Feb 2022 15:52:47 +0000 Subject: [PATCH] Remove coupling of block processor and unchecked_map on unchecked_info::modified. (#3728) * Currently the modified timestamp is updated by the block processor which is a violation of separations of concerns with the unchecked_map. This moves responsibility of setting modification timestamp in to unchecked_map. * The block_arrival container tracks blocks that are from realtime traffic instead of bootstrap traffic. This is sufficient to trigger process_live on its own. Checking the modification timestamp from unchecked_map is unrelated. * Encapsulating nano::unchecked_info::modified since it's only set by the unchecked_map class itself when writing. * Remove nano::unchecked_info friend class in favor of setting nano::unchecked_info::modified in the constructor. * Removes the unnecessary origination field from block_processor::add Co-authored-by: Thiago Silva --- nano/core_test/gap_cache.cpp | 6 +++--- nano/core_test/node.cpp | 4 ++-- nano/core_test/unchecked_map.cpp | 2 +- nano/nano_node/entry.cpp | 2 +- nano/node/blockprocessor.cpp | 22 +++++----------------- nano/node/blockprocessor.hpp | 2 +- nano/node/bootstrap/bootstrap_attempt.cpp | 2 +- nano/node/bootstrap/bootstrap_lazy.cpp | 2 +- nano/node/json_handler.cpp | 4 ++-- nano/node/node.cpp | 8 ++++---- nano/node/unchecked_map.cpp | 3 ++- nano/secure/common.cpp | 15 ++++++++++----- nano/secure/common.hpp | 9 +++++++-- nano/slow_test/node.cpp | 2 +- 14 files changed, 41 insertions(+), 42 deletions(-) diff --git a/nano/core_test/gap_cache.cpp b/nano/core_test/gap_cache.cpp index bbbd1cee..8c1d7a6b 100644 --- a/nano/core_test/gap_cache.cpp +++ b/nano/core_test/gap_cache.cpp @@ -95,13 +95,13 @@ TEST (gap_cache, two_dependencies) auto send2 (std::make_shared (send1->hash (), key.pub, 0, nano::dev::genesis_key.prv, nano::dev::genesis_key.pub, *system.work.generate (send1->hash ()))); auto open (std::make_shared (send1->hash (), key.pub, key.pub, key.prv, key.pub, *system.work.generate (key.pub))); ASSERT_EQ (0, node1.gap_cache.size ()); - node1.block_processor.add (send2, nano::seconds_since_epoch ()); + node1.block_processor.add (send2); node1.block_processor.flush (); ASSERT_EQ (1, node1.gap_cache.size ()); - node1.block_processor.add (open, nano::seconds_since_epoch ()); + node1.block_processor.add (open); node1.block_processor.flush (); ASSERT_EQ (2, node1.gap_cache.size ()); - node1.block_processor.add (send1, nano::seconds_since_epoch ()); + node1.block_processor.add (send1); node1.block_processor.flush (); ASSERT_TIMELY (5s, node1.gap_cache.size () == 0); auto transaction (node1.store.tx_begin_read ()); diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index f89daf48..5172eee2 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -2205,8 +2205,8 @@ TEST (node, block_confirm) auto send1_copy = builder.make_block () .from (*send1) .build_shared (); - node1.block_processor.add (send1, nano::seconds_since_epoch ()); - node2.block_processor.add (send1_copy, nano::seconds_since_epoch ()); + node1.block_processor.add (send1); + node2.block_processor.add (send1_copy); ASSERT_TIMELY (5s, node1.ledger.block_or_pruned_exists (send1->hash ()) && node2.ledger.block_or_pruned_exists (send1_copy->hash ())); ASSERT_TRUE (node1.ledger.block_or_pruned_exists (send1->hash ())); ASSERT_TRUE (node2.ledger.block_or_pruned_exists (send1_copy->hash ())); diff --git a/nano/core_test/unchecked_map.cpp b/nano/core_test/unchecked_map.cpp index c92d779d..742cdc6e 100644 --- a/nano/core_test/unchecked_map.cpp +++ b/nano/core_test/unchecked_map.cpp @@ -49,7 +49,7 @@ TEST (unchecked_map, construction) TEST (unchecked_map, put_one) { context context; - nano::unchecked_info info{ block (), nano::dev::genesis_key.pub, nano::seconds_since_epoch () }; + nano::unchecked_info info{ block (), nano::dev::genesis_key.pub }; context.unchecked.put (info.block->previous (), info); } diff --git a/nano/nano_node/entry.cpp b/nano/nano_node/entry.cpp index 522c87f5..399cd5a8 100644 --- a/nano/nano_node/entry.cpp +++ b/nano/nano_node/entry.cpp @@ -1824,7 +1824,7 @@ int main (int argc, char * const * argv) { std::cout << boost::str (boost::format ("%1% blocks retrieved") % count) << std::endl; } - nano::unchecked_info unchecked_info (block, account, 0, nano::signature_verification::unknown); + nano::unchecked_info unchecked_info (block, account, nano::signature_verification::unknown); node.node->block_processor.add (unchecked_info); if (block->type () == nano::block_type::state && block->previous ().is_zero () && source_node->ledger.is_epoch_link (block->link ())) { diff --git a/nano/node/blockprocessor.cpp b/nano/node/blockprocessor.cpp index 08fb7b40..9de8ece9 100644 --- a/nano/node/blockprocessor.cpp +++ b/nano/node/blockprocessor.cpp @@ -97,9 +97,9 @@ bool nano::block_processor::half_full () return size () >= node.flags.block_processor_full_size / 2; } -void nano::block_processor::add (std::shared_ptr const & block_a, uint64_t origination) +void nano::block_processor::add (std::shared_ptr const & block_a) { - nano::unchecked_info info (block_a, 0, origination, nano::signature_verification::unknown); + nano::unchecked_info info (block_a, 0, nano::signature_verification::unknown); add (info); } @@ -256,7 +256,7 @@ void nano::block_processor::process_batch (nano::unique_lock & lock } else { - info = nano::unchecked_info (forced.front (), 0, nano::seconds_since_epoch (), nano::signature_verification::unknown); + info = nano::unchecked_info (forced.front (), 0, nano::signature_verification::unknown); forced.pop_front (); hash = info.block->hash (); force = true; @@ -353,7 +353,7 @@ nano::process_return nano::block_processor::process_one (nano::write_transaction block->serialize_json (block_string, node.config.logging.single_line_record ()); node.logger.try_log (boost::str (boost::format ("Processing block %1%: %2%") % hash.to_string () % block_string)); } - if ((info_a.modified > nano::seconds_since_epoch () - 300 && node.block_arrival.recent (hash)) || forced_a) + if (node.block_arrival.recent (hash) || forced_a) { events_a.events.emplace_back ([this, hash, block = info_a.block, result, origin_a] (nano::transaction const & post_event_transaction_a) { process_live (post_event_transaction_a, hash, block, result, origin_a); }); } @@ -376,10 +376,6 @@ nano::process_return nano::block_processor::process_one (nano::write_transaction node.logger.try_log (boost::str (boost::format ("Gap previous for: %1%") % hash.to_string ())); } info_a.verified = result.verified; - if (info_a.modified == 0) - { - info_a.modified = nano::seconds_since_epoch (); - } node.unchecked.put (block->previous (), info_a); events_a.events.emplace_back ([this, hash] (nano::transaction const & /* unused */) { this->node.gap_cache.add (hash); }); node.stats.inc (nano::stat::type::ledger, nano::stat::detail::gap_previous); @@ -392,10 +388,6 @@ nano::process_return nano::block_processor::process_one (nano::write_transaction node.logger.try_log (boost::str (boost::format ("Gap source for: %1%") % hash.to_string ())); } info_a.verified = result.verified; - if (info_a.modified == 0) - { - info_a.modified = nano::seconds_since_epoch (); - } node.unchecked.put (node.ledger.block_source (transaction_a, *(block)), info_a); events_a.events.emplace_back ([this, hash] (nano::transaction const & /* unused */) { this->node.gap_cache.add (hash); }); node.stats.inc (nano::stat::type::ledger, nano::stat::detail::gap_source); @@ -408,10 +400,6 @@ nano::process_return nano::block_processor::process_one (nano::write_transaction node.logger.try_log (boost::str (boost::format ("Gap pending entries for epoch open: %1%") % hash.to_string ())); } info_a.verified = result.verified; - if (info_a.modified == 0) - { - info_a.modified = nano::seconds_since_epoch (); - } node.unchecked.put (block->account (), info_a); // Specific unchecked key starting with epoch open block account public key node.stats.inc (nano::stat::type::ledger, nano::stat::detail::gap_source); break; @@ -503,7 +491,7 @@ nano::process_return nano::block_processor::process_one (nano::write_transaction nano::process_return nano::block_processor::process_one (nano::write_transaction const & transaction_a, block_post_events & events_a, std::shared_ptr const & block_a) { - nano::unchecked_info info (block_a, block_a->account (), 0, nano::signature_verification::unknown); + nano::unchecked_info info (block_a, block_a->account (), nano::signature_verification::unknown); auto result (process_one (transaction_a, events_a, info)); return result; } diff --git a/nano/node/blockprocessor.hpp b/nano/node/blockprocessor.hpp index b3f2ace5..27043dfb 100644 --- a/nano/node/blockprocessor.hpp +++ b/nano/node/blockprocessor.hpp @@ -56,7 +56,7 @@ public: bool half_full (); void add_local (nano::unchecked_info const & info_a); void add (nano::unchecked_info const &); - void add (std::shared_ptr const &, uint64_t = 0); + void add (std::shared_ptr const &); void force (std::shared_ptr const &); void wait_write (); bool should_log (); diff --git a/nano/node/bootstrap/bootstrap_attempt.cpp b/nano/node/bootstrap/bootstrap_attempt.cpp index bc070d01..ed192a4b 100644 --- a/nano/node/bootstrap/bootstrap_attempt.cpp +++ b/nano/node/bootstrap/bootstrap_attempt.cpp @@ -144,7 +144,7 @@ bool nano::bootstrap_attempt::process_block (std::shared_ptr const } else { - nano::unchecked_info info (block_a, known_account_a, 0, nano::signature_verification::unknown); + nano::unchecked_info info (block_a, known_account_a, nano::signature_verification::unknown); node->block_processor.add (info); } return stop_pull; diff --git a/nano/node/bootstrap/bootstrap_lazy.cpp b/nano/node/bootstrap/bootstrap_lazy.cpp index cb9b3701..dd880884 100644 --- a/nano/node/bootstrap/bootstrap_lazy.cpp +++ b/nano/node/bootstrap/bootstrap_lazy.cpp @@ -268,7 +268,7 @@ bool nano::bootstrap_attempt_lazy::process_block_lazy (std::shared_ptrblock_processor.add (info); } // Force drop lazy bootstrap connection for long bulk_pull diff --git a/nano/node/json_handler.cpp b/nano/node/json_handler.cpp index a89119ec..db696a40 100644 --- a/nano/node/json_handler.cpp +++ b/nano/node/json_handler.cpp @@ -4078,7 +4078,7 @@ void nano::json_handler::unchecked_get () if (key.hash == hash) { nano::unchecked_info const & info (i->second); - response_l.put ("modified_timestamp", std::to_string (info.modified)); + response_l.put ("modified_timestamp", std::to_string (info.modified ())); if (json_block_l) { @@ -4126,7 +4126,7 @@ void nano::json_handler::unchecked_keys () nano::unchecked_info const & info (i->second); entry.put ("key", i->first.key ().to_string ()); entry.put ("hash", info.block->hash ().to_string ()); - entry.put ("modified_timestamp", std::to_string (info.modified)); + entry.put ("modified_timestamp", std::to_string (info.modified ())); if (json_block_l) { boost::property_tree::ptree block_node_l; diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 5cc5b31b..62dc5e8e 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -566,7 +566,7 @@ std::unique_ptr nano::collect_container_info (no void nano::node::process_active (std::shared_ptr const & incoming) { block_arrival.add (incoming->hash ()); - block_processor.add (incoming, nano::seconds_since_epoch ()); + block_processor.add (incoming); } nano::process_return nano::node::process (nano::block & block_a) @@ -581,7 +581,7 @@ nano::process_return nano::node::process_local (std::shared_ptr con // Add block hash as recently arrived to trigger automatic rebroadcast and election block_arrival.add (block_a->hash ()); // Set current time to trigger automatic rebroadcast and election - nano::unchecked_info info (block_a, block_a->account (), nano::seconds_since_epoch (), nano::signature_verification::unknown); + nano::unchecked_info info (block_a, block_a->account (), nano::signature_verification::unknown); // Notify block processor to release write lock block_processor.wait_write (); // Process block @@ -595,7 +595,7 @@ void nano::node::process_local_async (std::shared_ptr const & block // Add block hash as recently arrived to trigger automatic rebroadcast and election block_arrival.add (block_a->hash ()); // Set current time to trigger automatic rebroadcast and election - nano::unchecked_info info (block_a, block_a->account (), nano::seconds_since_epoch (), nano::signature_verification::unknown); + nano::unchecked_info info (block_a, block_a->account (), nano::signature_verification::unknown); block_processor.add_local (info); } @@ -950,7 +950,7 @@ void nano::node::unchecked_cleanup () { nano::unchecked_key const & key (i->first); nano::unchecked_info const & info (i->second); - if ((now - info.modified) > static_cast (config.unchecked_cutoff_time.count ())) + if ((now - info.modified ()) > static_cast (config.unchecked_cutoff_time.count ())) { digests.push_back (network.publish_filter.hash (info.block)); cleaning_list.push_back (key); diff --git a/nano/node/unchecked_map.cpp b/nano/node/unchecked_map.cpp index f36bcbbf..1347bec4 100644 --- a/nano/node/unchecked_map.cpp +++ b/nano/node/unchecked_map.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include @@ -96,7 +97,7 @@ nano::unchecked_map::item_visitor::item_visitor (unchecked_map & unchecked, nano void nano::unchecked_map::item_visitor::operator() (insert const & item) { auto const & [dependency, info] = item; - unchecked.store.unchecked.put (transaction, dependency, info); + unchecked.store.unchecked.put (transaction, dependency, { info.block, info.account, info.verified }); } void nano::unchecked_map::item_visitor::operator() (query const & item) diff --git a/nano/secure/common.cpp b/nano/secure/common.cpp index 21aa4a3f..387b76b3 100644 --- a/nano/secure/common.cpp +++ b/nano/secure/common.cpp @@ -354,16 +354,16 @@ nano::account const & nano::pending_key::key () const return account; } -nano::unchecked_info::unchecked_info (std::shared_ptr const & block_a, nano::account const & account_a, uint64_t modified_a, nano::signature_verification verified_a) : +nano::unchecked_info::unchecked_info (std::shared_ptr const & block_a, nano::account const & account_a, nano::signature_verification verified_a) : block (block_a), account (account_a), - modified (modified_a), + modified_m (nano::seconds_since_epoch ()), verified (verified_a) { } nano::unchecked_info::unchecked_info (std::shared_ptr const & block) : - unchecked_info{ block, block->account (), nano::seconds_since_epoch (), nano::signature_verification::unknown } + unchecked_info{ block, block->account (), nano::signature_verification::unknown } { } @@ -372,7 +372,7 @@ void nano::unchecked_info::serialize (nano::stream & stream_a) const debug_assert (block != nullptr); nano::serialize_block (stream_a, *block); nano::write (stream_a, account.bytes); - nano::write (stream_a, modified); + nano::write (stream_a, modified_m); nano::write (stream_a, verified); } @@ -385,7 +385,7 @@ bool nano::unchecked_info::deserialize (nano::stream & stream_a) try { nano::read (stream_a, account.bytes); - nano::read (stream_a, modified); + nano::read (stream_a, modified_m); nano::read (stream_a, verified); } catch (std::runtime_error const &) @@ -396,6 +396,11 @@ bool nano::unchecked_info::deserialize (nano::stream & stream_a) return error; } +uint64_t nano::unchecked_info::modified () const +{ + return modified_m; +} + nano::endpoint_key::endpoint_key (std::array const & address_a, uint16_t port_a) : address (address_a), network_port (boost::endian::native_to_big (port_a)) { diff --git a/nano/secure/common.hpp b/nano/secure/common.hpp index 4d3a32ae..c97fabf3 100644 --- a/nano/secure/common.hpp +++ b/nano/secure/common.hpp @@ -199,14 +199,19 @@ class unchecked_info final { public: unchecked_info () = default; - unchecked_info (std::shared_ptr const &, nano::account const &, uint64_t, nano::signature_verification = nano::signature_verification::unknown); + unchecked_info (std::shared_ptr const &, nano::account const &, nano::signature_verification = nano::signature_verification::unknown); unchecked_info (std::shared_ptr const &); void serialize (nano::stream &) const; bool deserialize (nano::stream &); + uint64_t modified () const; std::shared_ptr block; nano::account account{}; + +private: /** Seconds since posix epoch */ - uint64_t modified{ 0 }; + uint64_t modified_m{ 0 }; + +public: nano::signature_verification verified{ nano::signature_verification::unknown }; }; diff --git a/nano/slow_test/node.cpp b/nano/slow_test/node.cpp index 7fd6f1ac..df80d5df 100644 --- a/nano/slow_test/node.cpp +++ b/nano/slow_test/node.cpp @@ -531,7 +531,7 @@ TEST (node, mass_vote_by_hash) } for (auto i (blocks.begin ()), n (blocks.end ()); i != n; ++i) { - system.nodes[0]->block_processor.add (*i, nano::seconds_since_epoch ()); + system.nodes[0]->block_processor.add (*i); } }