diff --git a/nano/core_test/active_transactions.cpp b/nano/core_test/active_transactions.cpp index 85b8eeb3..60dcdcb3 100644 --- a/nano/core_test/active_transactions.cpp +++ b/nano/core_test/active_transactions.cpp @@ -823,3 +823,31 @@ TEST (active_transactions, dropped_cleanup) ASSERT_FALSE (node.network.publish_filter.apply (block_bytes.data (), block_bytes.size ())); } } + +namespace nano +{ +// Blocks that won an election must always be seen as confirming or cemented +TEST (active_transactions, confirmation_consistency) +{ + nano::system system; + nano::node_config node_config (nano::get_available_port (), system.logging); + node_config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled; + auto & node = *system.add_node (node_config); + system.wallet (0)->insert_adhoc (nano::test_genesis_key.prv); + for (unsigned i = 0; i < 10; ++i) + { + auto block (system.wallet (0)->send_action (nano::test_genesis_key.pub, nano::public_key (), node.config.receive_minimum.number ())); + ASSERT_NE (nullptr, block); + system.deadline_set (5s); + while (!node.ledger.block_confirmed (node.store.tx_begin_read (), block->hash ())) + { + ASSERT_FALSE (node.active.insert (block).second); + ASSERT_NO_ERROR (system.poll (5ms)); + } + nano::lock_guard guard (node.active.mutex); + ASSERT_EQ (i + 1, node.active.recently_confirmed.size ()); + ASSERT_EQ (block->qualified_root (), node.active.recently_confirmed.back ()); + ASSERT_EQ (i + 1, node.active.recently_cemented.size ()); + } +} +} \ No newline at end of file diff --git a/nano/core_test/confirmation_height.cpp b/nano/core_test/confirmation_height.cpp index 13ba95d6..b9c309ec 100644 --- a/nano/core_test/confirmation_height.cpp +++ b/nano/core_test/confirmation_height.cpp @@ -1012,6 +1012,7 @@ TEST (confirmation_height, prioritize_frontiers) std::array frontiers{ send17.qualified_root (), send6.qualified_root (), send7.qualified_root (), open2.qualified_root (), send11.qualified_root () }; for (auto & frontier : frontiers) { + nano::lock_guard guard (node->active.mutex); ASSERT_NE (node->active.roots.find (frontier), node->active.roots.end ()); } }; @@ -1122,7 +1123,7 @@ TEST (confirmation_height, callback_confirmed_history) ASSERT_NO_ERROR (system.poll ()); } - ASSERT_EQ (0, node->active.list_confirmed ().size ()); + ASSERT_EQ (0, node->active.list_recently_cemented ().size ()); { nano::lock_guard guard (node->active.mutex); ASSERT_EQ (0, node->active.blocks.size ()); @@ -1162,7 +1163,7 @@ TEST (confirmation_height, callback_confirmed_history) ASSERT_NO_ERROR (system.poll ()); } - ASSERT_EQ (1, node->active.list_confirmed ().size ()); + ASSERT_EQ (1, node->active.list_recently_cemented ().size ()); ASSERT_EQ (0, node->active.blocks.size ()); // Confirm the callback is not called under this circumstance diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 222307b2..a4e34de2 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -2506,9 +2506,9 @@ TEST (node, block_confirm) ASSERT_EQ (nano::process_result::progress, node2.ledger.process (transaction, *send2).code); } node1.block_confirm (send2); - ASSERT_TRUE (node1.active.list_confirmed ().empty ()); + ASSERT_TRUE (node1.active.list_recently_cemented ().empty ()); system.deadline_set (10s); - while (node1.active.list_confirmed ().empty ()) + while (node1.active.list_recently_cemented ().empty ()) { ASSERT_NO_ERROR (system.poll ()); } diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index 3ef813ca..c20049f2 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -158,12 +158,10 @@ void nano::active_transactions::block_cemented_callback (std::shared_ptrsecond; election_winner_details.erase (hash); election_winners_lk.unlock (); - // Make sure mutex is held before election usage so we know that confirm_once has - // finished removing the root from active to avoid any data race. nano::unique_lock lk (mutex); if (election->confirmed () && election->status.winner->hash () == hash) { - add_confirmed (election->status, block_a->qualified_root ()); + add_recently_cemented (election->status); lk.unlock (); node.receive_confirmed (transaction, block_a, hash); nano::account account (0); @@ -171,9 +169,11 @@ void nano::active_transactions::block_cemented_callback (std::shared_ptrstatus.type = *election_status_type; election->status.confirmation_request_count = election->confirmation_request_count; node.observers.blocks.notify (election->status, account, amount, is_state_send); + lk.unlock (); if (amount > 0) { node.observers.account_balance.notify (account, false); @@ -488,7 +488,7 @@ std::pair, bool> nano::active_transactions::inse auto existing (roots.get ().find (root)); if (existing == roots.get ().end ()) { - if (confirmed_set.get ().find (root) == confirmed_set.get ().end ()) + if (recently_confirmed.get ().find (root) == recently_confirmed.get ().end ()) { result.second = true; auto hash (block_a->hash ()); @@ -777,23 +777,27 @@ std::deque> nano::active_transactions::list_blocks return result; } -std::deque nano::active_transactions::list_confirmed () +std::deque nano::active_transactions::list_recently_cemented () { nano::lock_guard lock (mutex); - return confirmed; + return recently_cemented; } -void nano::active_transactions::add_confirmed (nano::election_status const & status_a, nano::qualified_root const & root_a) +void nano::active_transactions::add_recently_cemented (nano::election_status const & status_a) { - confirmed.push_back (status_a); - auto inserted (confirmed_set.get ().push_back (root_a)); - if (confirmed.size () > node.config.confirmation_history_size) + recently_cemented.push_back (status_a); + if (recently_cemented.size () > node.config.confirmation_history_size) { - confirmed.pop_front (); - if (inserted.second) - { - confirmed_set.get ().pop_front (); - } + recently_cemented.pop_front (); + } +} + +void nano::active_transactions::add_recently_confirmed (nano::qualified_root const & root_a) +{ + recently_confirmed.get ().push_back (root_a); + if (recently_confirmed.size () > node.config.confirmation_history_size) + { + recently_confirmed.get ().pop_front (); } } @@ -1015,20 +1019,23 @@ std::unique_ptr nano::collect_container_info (ac { size_t roots_count; size_t blocks_count; - size_t confirmed_count; + size_t recently_confirmed_count; + size_t recently_cemented_count; { nano::lock_guard guard (active_transactions.mutex); roots_count = active_transactions.roots.size (); blocks_count = active_transactions.blocks.size (); - confirmed_count = active_transactions.confirmed.size (); + recently_confirmed_count = active_transactions.recently_confirmed.size (); + recently_cemented_count = active_transactions.recently_cemented.size (); } auto composite = std::make_unique (name); composite->add_component (std::make_unique (container_info{ "roots", roots_count, sizeof (decltype (active_transactions.roots)::value_type) })); composite->add_component (std::make_unique (container_info{ "blocks", blocks_count, sizeof (decltype (active_transactions.blocks)::value_type) })); composite->add_component (std::make_unique (container_info{ "election_winner_details", active_transactions.election_winner_details_size (), sizeof (decltype (active_transactions.election_winner_details)::value_type) })); - composite->add_component (std::make_unique (container_info{ "confirmed", confirmed_count, sizeof (decltype (active_transactions.confirmed)::value_type) })); + composite->add_component (std::make_unique (container_info{ "recently_confirmed", recently_confirmed_count, sizeof (decltype (active_transactions.recently_confirmed)::value_type) })); + composite->add_component (std::make_unique (container_info{ "recently_cemented", recently_cemented_count, sizeof (decltype (active_transactions.recently_cemented)::value_type) })); composite->add_component (std::make_unique (container_info{ "priority_wallet_cementable_frontiers_count", active_transactions.priority_wallet_cementable_frontiers_size (), sizeof (nano::cementable_account) })); composite->add_component (std::make_unique (container_info{ "priority_cementable_frontiers_count", active_transactions.priority_cementable_frontiers_size (), sizeof (nano::cementable_account) })); composite->add_component (std::make_unique (container_info{ "inactive_votes_cache_count", active_transactions.inactive_votes_cache_size (), sizeof (nano::gap_information) })); diff --git a/nano/node/active_transactions.hpp b/nano/node/active_transactions.hpp index e1decad2..59e7c930 100644 --- a/nano/node/active_transactions.hpp +++ b/nano/node/active_transactions.hpp @@ -120,9 +120,11 @@ public: roots; // clang-format on std::unordered_map> blocks; - std::deque list_confirmed (); - std::deque confirmed; - void add_confirmed (nano::election_status const &, nano::qualified_root const &); + std::deque list_recently_cemented (); + std::deque recently_cemented; + + void add_recently_cemented (nano::election_status const &); + void add_recently_confirmed (nano::qualified_root const &); void add_inactive_votes_cache (nano::block_hash const &, nano::account const &); nano::inactive_cache_information find_inactive_votes_cache (nano::block_hash const &); void erase_inactive_votes_cache (nano::block_hash const &); @@ -168,7 +170,7 @@ private: mi::sequenced>, mi::hashed_unique, mi::identity>>> - confirmed_set; + recently_confirmed; using prioritize_num_uncemented = boost::multi_index_container, @@ -202,6 +204,7 @@ private: friend class active_transactions_dropped_cleanup_Test; friend class confirmation_height_prioritize_frontiers_Test; friend class confirmation_height_prioritize_frontiers_overwrite_Test; + friend class active_transactions_confirmation_consistency_Test; friend std::unique_ptr collect_container_info (active_transactions &, const std::string &); }; diff --git a/nano/node/election.cpp b/nano/node/election.cpp index 828e176e..74ed1ed7 100644 --- a/nano/node/election.cpp +++ b/nano/node/election.cpp @@ -54,6 +54,7 @@ void nano::election::confirm_once (nano::election_status_type type_a) // election winner details can be cleared consistently sucessfully in active_transactions::block_cemented_callback node.active.add_election_winner_details (status_l.winner->hash (), this_l); } + node.active.add_recently_confirmed (status_l.winner->qualified_root ()); node.background ([node_l, status_l, confirmation_action_l, this_l]() { node_l->process_confirmed (status_l, this_l); confirmation_action_l (status_l.winner); diff --git a/nano/node/json_handler.cpp b/nano/node/json_handler.cpp index ea11063f..cbf45281 100644 --- a/nano/node/json_handler.cpp +++ b/nano/node/json_handler.cpp @@ -1022,8 +1022,11 @@ void nano::json_handler::block_confirm () { if (!node.ledger.block_confirmed (transaction, hash)) { - // Start new confirmation for unconfirmed block - node.block_confirm (std::move (block_l)); + // Start new confirmation for unconfirmed (or not being confirmed) block + if (!node.confirmation_height_processor.is_processing_block (hash)) + { + node.block_confirm (std::move (block_l)); + } } else { @@ -1031,11 +1034,7 @@ void nano::json_handler::block_confirm () nano::election_status status{ block_l, 0, std::chrono::duration_cast (std::chrono::system_clock::now ().time_since_epoch ()), std::chrono::duration_values::zero (), 0, 1, 0, nano::election_status_type::active_confirmation_height }; { nano::lock_guard lock (node.active.mutex); - node.active.confirmed.push_back (status); - if (node.active.confirmed.size () > node.config.confirmation_history_size) - { - node.active.confirmed.pop_front (); - } + node.active.add_recently_cemented (status); } // Trigger callback for confirmed block node.block_arrival.add (hash); @@ -1812,7 +1811,7 @@ void nano::json_handler::confirmation_history () } if (!ec) { - auto confirmed (node.active.list_confirmed ()); + auto confirmed (node.active.list_recently_cemented ()); for (auto i (confirmed.begin ()), n (confirmed.end ()); i != n; ++i) { if (hash.is_zero () || i->winner->hash () == hash) diff --git a/nano/node/wallet.cpp b/nano/node/wallet.cpp index 9a344ea3..07153afa 100644 --- a/nano/node/wallet.cpp +++ b/nano/node/wallet.cpp @@ -1275,8 +1275,11 @@ bool nano::wallet::search_pending () } else { - // Request confirmation for unconfirmed block - wallets.node.block_confirm (block); + if (!wallets.node.confirmation_height_processor.is_processing_block (hash)) + { + // Request confirmation for block which is not being processed yet + wallets.node.block_confirm (block); + } } } } diff --git a/nano/rpc_test/rpc.cpp b/nano/rpc_test/rpc.cpp index 5323779e..ff073715 100644 --- a/nano/rpc_test/rpc.cpp +++ b/nano/rpc_test/rpc.cpp @@ -6235,11 +6235,11 @@ TEST (rpc, confirmation_history) auto node = add_ipc_enabled_node (system); nano::keypair key; system.wallet (0)->insert_adhoc (nano::test_genesis_key.prv); - ASSERT_TRUE (node->active.list_confirmed ().empty ()); + ASSERT_TRUE (node->active.list_recently_cemented ().empty ()); auto block (system.wallet (0)->send_action (nano::test_genesis_key.pub, key.pub, nano::Gxrb_ratio)); scoped_io_thread_name_change scoped_thread_name_io; system.deadline_set (10s); - while (node->active.list_confirmed ().empty ()) + while (node->active.list_recently_cemented ().empty ()) { ASSERT_NO_ERROR (system.poll ()); } @@ -6282,13 +6282,13 @@ TEST (rpc, confirmation_history_hash) auto node = add_ipc_enabled_node (system); nano::keypair key; system.wallet (0)->insert_adhoc (nano::test_genesis_key.prv); - ASSERT_TRUE (node->active.list_confirmed ().empty ()); + ASSERT_TRUE (node->active.list_recently_cemented ().empty ()); auto send1 (system.wallet (0)->send_action (nano::test_genesis_key.pub, key.pub, nano::Gxrb_ratio)); auto send2 (system.wallet (0)->send_action (nano::test_genesis_key.pub, key.pub, nano::Gxrb_ratio)); auto send3 (system.wallet (0)->send_action (nano::test_genesis_key.pub, key.pub, nano::Gxrb_ratio)); scoped_io_thread_name_change scoped_thread_name_io; system.deadline_set (10s); - while (node->active.list_confirmed ().size () != 3) + while (node->active.list_recently_cemented ().size () != 3) { ASSERT_NO_ERROR (system.poll ()); } @@ -6419,7 +6419,7 @@ TEST (rpc, block_confirm_confirmed) ASSERT_EQ (200, response.status); ASSERT_EQ ("1", response.json.get ("started")); // Check confirmation history - auto confirmed (node->active.list_confirmed ()); + auto confirmed (node->active.list_recently_cemented ()); ASSERT_EQ (1, confirmed.size ()); ASSERT_EQ (nano::genesis_hash, confirmed.begin ()->winner->hash ()); // Check callback