From 076df48948f4099f271c02f14763f426f40e7e1c Mon Sep 17 00:00:00 2001 From: Colin LeMahieu Date: Fri, 27 Sep 2024 18:25:11 +0100 Subject: [PATCH 1/4] Removing use_noops from write_queue. --- nano/core_test/confirming_set.cpp | 2 -- nano/core_test/ledger_confirm.cpp | 4 ++-- nano/node/make_store.cpp | 4 ++-- nano/node/make_store.hpp | 2 +- nano/node/node.cpp | 2 +- nano/node/nodeconfig.hpp | 1 - nano/slow_test/node.cpp | 4 ++-- nano/store/component.cpp | 3 +-- nano/store/component.hpp | 3 +-- nano/store/lmdb/lmdb.cpp | 3 +-- nano/store/rocksdb/rocksdb.cpp | 5 ++--- nano/store/rocksdb/rocksdb.hpp | 2 +- nano/store/write_queue.cpp | 15 ++------------- nano/store/write_queue.hpp | 4 +--- 14 files changed, 17 insertions(+), 37 deletions(-) diff --git a/nano/core_test/confirming_set.cpp b/nano/core_test/confirming_set.cpp index 47b8eaf7ee..905bf523a3 100644 --- a/nano/core_test/confirming_set.cpp +++ b/nano/core_test/confirming_set.cpp @@ -119,7 +119,6 @@ TEST (confirmation_callback, confirmed_history) { nano::test::system system; nano::node_flags node_flags; - node_flags.force_use_write_queue = true; node_flags.disable_ascending_bootstrap = true; nano::node_config node_config = system.default_config (); node_config.backlog_population.enable = false; @@ -194,7 +193,6 @@ TEST (confirmation_callback, dependent_election) { nano::test::system system; nano::node_flags node_flags; - node_flags.force_use_write_queue = true; nano::node_config node_config = system.default_config (); node_config.backlog_population.enable = false; auto node = system.add_node (node_config, node_flags); diff --git a/nano/core_test/ledger_confirm.cpp b/nano/core_test/ledger_confirm.cpp index 65f351bacc..360a3512bb 100644 --- a/nano/core_test/ledger_confirm.cpp +++ b/nano/core_test/ledger_confirm.cpp @@ -784,7 +784,7 @@ TEST (ledger_confirm, pruned_source) ASSERT_TRUE (!store->init_error ()); nano::ledger ledger (*store, system.stats, nano::dev::constants); ledger.pruning = true; - nano::store::write_queue write_queue (false); + nano::store::write_queue write_queue; nano::work_pool pool{ nano::dev::network_params.network, std::numeric_limits::max () }; nano::keypair key1, key2; nano::block_builder builder; @@ -868,7 +868,7 @@ TEST (ledger_confirmDeathTest, rollback_added_block) auto store = nano::make_store (system.logger, path, nano::dev::constants); ASSERT_TRUE (!store->init_error ()); nano::ledger ledger (*store, system.stats, nano::dev::constants); - nano::store::write_queue write_queue (false); + nano::store::write_queue write_queue; nano::work_pool pool{ nano::dev::network_params.network, std::numeric_limits::max () }; nano::keypair key1; nano::block_builder builder; diff --git a/nano/node/make_store.cpp b/nano/node/make_store.cpp index 9122a23d07..87c15700fe 100644 --- a/nano/node/make_store.cpp +++ b/nano/node/make_store.cpp @@ -3,11 +3,11 @@ #include #include -std::unique_ptr nano::make_store (nano::logger & logger, std::filesystem::path const & path, nano::ledger_constants & constants, bool read_only, bool add_db_postfix, nano::rocksdb_config const & rocksdb_config, nano::txn_tracking_config const & txn_tracking_config_a, std::chrono::milliseconds block_processor_batch_max_time_a, nano::lmdb_config const & lmdb_config_a, bool backup_before_upgrade, bool force_use_write_queue) +std::unique_ptr nano::make_store (nano::logger & logger, std::filesystem::path const & path, nano::ledger_constants & constants, bool read_only, bool add_db_postfix, nano::rocksdb_config const & rocksdb_config, nano::txn_tracking_config const & txn_tracking_config_a, std::chrono::milliseconds block_processor_batch_max_time_a, nano::lmdb_config const & lmdb_config_a, bool backup_before_upgrade) { if (rocksdb_config.enable) { - return std::make_unique (logger, add_db_postfix ? path / "rocksdb" : path, constants, rocksdb_config, read_only, force_use_write_queue); + return std::make_unique (logger, add_db_postfix ? path / "rocksdb" : path, constants, rocksdb_config, read_only); } return std::make_unique (logger, add_db_postfix ? path / "data.ldb" : path, constants, txn_tracking_config_a, block_processor_batch_max_time_a, lmdb_config_a, backup_before_upgrade); diff --git a/nano/node/make_store.hpp b/nano/node/make_store.hpp index d66db5cbfb..5f5b5f9372 100644 --- a/nano/node/make_store.hpp +++ b/nano/node/make_store.hpp @@ -22,5 +22,5 @@ class component; namespace nano { -std::unique_ptr make_store (nano::logger &, std::filesystem::path const & path, nano::ledger_constants & constants, bool open_read_only = false, bool add_db_postfix = true, nano::rocksdb_config const & rocksdb_config = nano::rocksdb_config{}, nano::txn_tracking_config const & txn_tracking_config_a = nano::txn_tracking_config{}, std::chrono::milliseconds block_processor_batch_max_time_a = std::chrono::milliseconds (5000), nano::lmdb_config const & lmdb_config_a = nano::lmdb_config{}, bool backup_before_upgrade = false, bool force_use_write_queue = false); +std::unique_ptr make_store (nano::logger &, std::filesystem::path const & path, nano::ledger_constants & constants, bool open_read_only = false, bool add_db_postfix = true, nano::rocksdb_config const & rocksdb_config = nano::rocksdb_config{}, nano::txn_tracking_config const & txn_tracking_config_a = nano::txn_tracking_config{}, std::chrono::milliseconds block_processor_batch_max_time_a = std::chrono::milliseconds (5000), nano::lmdb_config const & lmdb_config_a = nano::lmdb_config{}, bool backup_before_upgrade = false); } diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 46519b0801..9a3b35e2cd 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -83,7 +83,7 @@ nano::node::node (std::shared_ptr io_ctx_a, std::filesy flags (flags_a), work (work_a), distributed_work (*this), - store_impl (nano::make_store (logger, application_path_a, network_params.ledger, flags.read_only, true, config_a.rocksdb_config, config_a.diagnostics_config.txn_tracking, config_a.block_processor_batch_max_time, config_a.lmdb_config, config_a.backup_before_upgrade, flags.force_use_write_queue)), + store_impl (nano::make_store (logger, application_path_a, network_params.ledger, flags.read_only, true, config_a.rocksdb_config, config_a.diagnostics_config.txn_tracking, config_a.block_processor_batch_max_time, config_a.lmdb_config, config_a.backup_before_upgrade)), store (*store_impl), unchecked{ config.max_unchecked_blocks, stats, flags.disable_block_processor_unchecked_deletion }, wallets_store_impl (std::make_unique (application_path_a / "wallets.ldb", config_a.lmdb_config)), diff --git a/nano/node/nodeconfig.hpp b/nano/node/nodeconfig.hpp index 6f9ec2d802..8981ccf87f 100644 --- a/nano/node/nodeconfig.hpp +++ b/nano/node/nodeconfig.hpp @@ -182,7 +182,6 @@ class node_flags final bool allow_bootstrap_peers_duplicates{ false }; bool disable_max_peers_per_ip{ false }; // For testing only bool disable_max_peers_per_subnetwork{ false }; // For testing only - bool force_use_write_queue{ false }; // For testing only. RocksDB does not use the database queue, but some tests rely on it being used. bool disable_search_pending{ false }; // For testing only bool enable_pruning{ false }; bool fast_bootstrap{ false }; diff --git a/nano/slow_test/node.cpp b/nano/slow_test/node.cpp index 09c2f22fbb..d5e168c883 100644 --- a/nano/slow_test/node.cpp +++ b/nano/slow_test/node.cpp @@ -1139,13 +1139,13 @@ TEST (confirmation_height, many_accounts_send_receive_self_no_elections) ASSERT_TRUE (!store->init_error ()); nano::stats stats{ logger }; nano::ledger ledger (*store, stats, nano::dev::constants); - nano::store::write_queue write_database_queue (false); + nano::store::write_queue write_database_queue; nano::work_pool pool{ nano::dev::network_params.network, std::numeric_limits::max () }; std::atomic stopped{ false }; boost::latch initialized_latch{ 0 }; nano::block_hash block_hash_being_processed{ 0 }; - nano::store::write_queue write_queue{ false }; + nano::store::write_queue write_queue; nano::confirming_set_config confirming_set_config{}; nano::confirming_set confirming_set{ confirming_set_config, ledger, stats }; diff --git a/nano/store/component.cpp b/nano/store/component.cpp index 815f3e5950..5d8c8bec20 100644 --- a/nano/store/component.cpp +++ b/nano/store/component.cpp @@ -7,7 +7,7 @@ #include #include -nano::store::component::component (nano::store::block & block_store_a, nano::store::account & account_store_a, nano::store::pending & pending_store_a, nano::store::online_weight & online_weight_store_a, nano::store::pruned & pruned_store_a, nano::store::peer & peer_store_a, nano::store::confirmation_height & confirmation_height_store_a, nano::store::final_vote & final_vote_store_a, nano::store::version & version_store_a, nano::store::rep_weight & rep_weight_a, bool use_noops_a) : +nano::store::component::component (nano::store::block & block_store_a, nano::store::account & account_store_a, nano::store::pending & pending_store_a, nano::store::online_weight & online_weight_store_a, nano::store::pruned & pruned_store_a, nano::store::peer & peer_store_a, nano::store::confirmation_height & confirmation_height_store_a, nano::store::final_vote & final_vote_store_a, nano::store::version & version_store_a, nano::store::rep_weight & rep_weight_a) : block (block_store_a), account (account_store_a), pending (pending_store_a), @@ -17,7 +17,6 @@ nano::store::component::component (nano::store::block & block_store_a, nano::sto confirmation_height (confirmation_height_store_a), final_vote (final_vote_store_a), version (version_store_a), - write_queue (use_noops_a), rep_weight (rep_weight_a) { } diff --git a/nano/store/component.hpp b/nano/store/component.hpp index a64ae1a60f..e85de20aa4 100644 --- a/nano/store/component.hpp +++ b/nano/store/component.hpp @@ -53,8 +53,7 @@ namespace store nano::store::confirmation_height &, nano::store::final_vote &, nano::store::version &, - nano::store::rep_weight &, - bool use_noops_a + nano::store::rep_weight & ); // clang-format on virtual ~component () = default; diff --git a/nano/store/lmdb/lmdb.cpp b/nano/store/lmdb/lmdb.cpp index a2fbb618ed..b9bc04721a 100644 --- a/nano/store/lmdb/lmdb.cpp +++ b/nano/store/lmdb/lmdb.cpp @@ -26,8 +26,7 @@ nano::store::lmdb::component::component (nano::logger & logger_a, std::filesyste confirmation_height_store, final_vote_store, version_store, - rep_weight_store, - false // write_queue use_noops + rep_weight_store }, // clang-format on block_store{ *this }, diff --git a/nano/store/rocksdb/rocksdb.cpp b/nano/store/rocksdb/rocksdb.cpp index 439cb92db7..c16c6f57dd 100644 --- a/nano/store/rocksdb/rocksdb.cpp +++ b/nano/store/rocksdb/rocksdb.cpp @@ -35,7 +35,7 @@ class event_listener : public rocksdb::EventListener }; } -nano::store::rocksdb::component::component (nano::logger & logger_a, std::filesystem::path const & path_a, nano::ledger_constants & constants, nano::rocksdb_config const & rocksdb_config_a, bool open_read_only_a, bool force_use_write_queue) : +nano::store::rocksdb::component::component (nano::logger & logger_a, std::filesystem::path const & path_a, nano::ledger_constants & constants, nano::rocksdb_config const & rocksdb_config_a, bool open_read_only_a) : // clang-format off nano::store::component{ block_store, @@ -47,8 +47,7 @@ nano::store::rocksdb::component::component (nano::logger & logger_a, std::filesy confirmation_height_store, final_vote_store, version_store, - rep_weight_store, - !force_use_write_queue // write_queue use_noops + rep_weight_store }, // clang-format on block_store{ *this }, diff --git a/nano/store/rocksdb/rocksdb.hpp b/nano/store/rocksdb/rocksdb.hpp index e106931c3d..fe197536e3 100644 --- a/nano/store/rocksdb/rocksdb.hpp +++ b/nano/store/rocksdb/rocksdb.hpp @@ -64,7 +64,7 @@ class component : public nano::store::component friend class nano::store::rocksdb::version; friend class nano::store::rocksdb::rep_weight; - explicit component (nano::logger &, std::filesystem::path const &, nano::ledger_constants & constants, nano::rocksdb_config const & = nano::rocksdb_config{}, bool open_read_only = false, bool force_use_write_queue = false); + explicit component (nano::logger &, std::filesystem::path const &, nano::ledger_constants & constants, nano::rocksdb_config const & = nano::rocksdb_config{}, bool open_read_only = false); store::write_transaction tx_begin_write (std::vector const & tables_requiring_lock = {}, std::vector const & tables_no_lock = {}) override; store::read_transaction tx_begin_read () const override; diff --git a/nano/store/write_queue.cpp b/nano/store/write_queue.cpp index 154a62cee6..41b687ce76 100644 --- a/nano/store/write_queue.cpp +++ b/nano/store/write_queue.cpp @@ -54,8 +54,7 @@ void nano::store::write_guard::renew () * write_queue */ -nano::store::write_queue::write_queue (bool use_noops_a) : - use_noops{ use_noops_a } +nano::store::write_queue::write_queue () { } @@ -66,7 +65,6 @@ nano::store::write_guard nano::store::write_queue::wait (writer writer) bool nano::store::write_queue::contains (writer writer) const { - debug_assert (!use_noops); nano::lock_guard guard{ mutex }; return std::find (queue.cbegin (), queue.cend (), writer) != queue.cend (); } @@ -83,11 +81,6 @@ void nano::store::write_queue::pop () void nano::store::write_queue::acquire (writer writer) { - if (use_noops) - { - return; // Pass immediately - } - nano::unique_lock lock{ mutex }; // There should be no duplicates in the queue @@ -105,10 +98,6 @@ void nano::store::write_queue::acquire (writer writer) void nano::store::write_queue::release (writer writer) { - if (use_noops) - { - return; // Pass immediately - } { nano::lock_guard guard{ mutex }; release_assert (!queue.empty ()); @@ -116,4 +105,4 @@ void nano::store::write_queue::release (writer writer) queue.pop_front (); } condition.notify_all (); -} \ No newline at end of file +} diff --git a/nano/store/write_queue.hpp b/nano/store/write_queue.hpp index e4f1fdc69a..6b4688618a 100644 --- a/nano/store/write_queue.hpp +++ b/nano/store/write_queue.hpp @@ -54,7 +54,7 @@ class write_queue final friend class write_guard; public: - explicit write_queue (bool use_noops); + explicit write_queue (); /** Blocks until we are at the head of the queue and blocks other waiters until write_guard goes out of scope */ [[nodiscard ("write_guard blocks other waiters")]] write_guard wait (writer writer); @@ -70,8 +70,6 @@ class write_queue final void release (writer writer); private: - bool const use_noops; - std::deque queue; mutable nano::mutex mutex; nano::condition_variable condition; From 68d47e308e7173c035c1d47155e04b433dec6045 Mon Sep 17 00:00:00 2001 From: Colin LeMahieu Date: Fri, 27 Sep 2024 11:57:13 +0100 Subject: [PATCH 2/4] Removing rocksdb write lock mutexes as store::write_queue serializes write transactions. --- nano/store/rocksdb/rocksdb.cpp | 31 ++----------------------- nano/store/rocksdb/rocksdb.hpp | 2 -- nano/store/rocksdb/transaction.cpp | 27 +++------------------ nano/store/rocksdb/transaction_impl.hpp | 8 +------ 4 files changed, 6 insertions(+), 62 deletions(-) diff --git a/nano/store/rocksdb/rocksdb.cpp b/nano/store/rocksdb/rocksdb.cpp index c16c6f57dd..d17cbe28e9 100644 --- a/nano/store/rocksdb/rocksdb.cpp +++ b/nano/store/rocksdb/rocksdb.cpp @@ -117,11 +117,6 @@ nano::store::rocksdb::component::component (nano::logger & logger_a, std::filesy db.reset (nullptr); } - if (!open_read_only_a) - { - construct_column_family_mutexes (); - } - if (is_fully_upgraded) { open (error, path_a, open_read_only_a, options, create_column_families ()); @@ -422,24 +417,10 @@ std::vector nano::store::rocksdb::component::cr return column_families; } -nano::store::write_transaction nano::store::rocksdb::component::tx_begin_write (std::vector const & tables_requiring_locks_a, std::vector const & tables_no_locks_a) +nano::store::write_transaction nano::store::rocksdb::component::tx_begin_write (std::vector const &, std::vector const &) { - std::unique_ptr txn; release_assert (db != nullptr); - if (tables_requiring_locks_a.empty () && tables_no_locks_a.empty ()) - { - // Use all tables if none are specified - txn = std::make_unique (transaction_db, all_tables (), tables_no_locks_a, write_lock_mutexes); - } - else - { - txn = std::make_unique (transaction_db, tables_requiring_locks_a, tables_no_locks_a, write_lock_mutexes); - } - - // Tables must be kept in alphabetical order. These can be used for mutex locking, so order is important to prevent deadlocking - debug_assert (std::is_sorted (tables_requiring_locks_a.begin (), tables_requiring_locks_a.end ())); - - return store::write_transaction{ std::move (txn) }; + return store::write_transaction{ std::make_unique (transaction_db) }; } nano::store::read_transaction nano::store::rocksdb::component::tx_begin_read () const @@ -743,14 +724,6 @@ int nano::store::rocksdb::component::clear (::rocksdb::ColumnFamilyHandle * colu return status.code (); } -void nano::store::rocksdb::component::construct_column_family_mutexes () -{ - for (auto table : all_tables ()) - { - write_lock_mutexes.emplace (std::piecewise_construct, std::forward_as_tuple (table), std::forward_as_tuple ()); - } -} - rocksdb::Options nano::store::rocksdb::component::get_db_options () { ::rocksdb::Options db_options; diff --git a/nano/store/rocksdb/rocksdb.hpp b/nano/store/rocksdb/rocksdb.hpp index fe197536e3..643a67a672 100644 --- a/nano/store/rocksdb/rocksdb.hpp +++ b/nano/store/rocksdb/rocksdb.hpp @@ -108,7 +108,6 @@ class component : public nano::store::component ::rocksdb::TransactionDB * transaction_db = nullptr; std::unique_ptr<::rocksdb::DB> db; std::vector> handles; - std::unordered_map write_lock_mutexes; nano::rocksdb_config rocksdb_config; unsigned const max_block_write_batch_num_m; @@ -152,7 +151,6 @@ class component : public nano::store::component void upgrade_v22_to_v23 (store::write_transaction &); void upgrade_v23_to_v24 (store::write_transaction &); - void construct_column_family_mutexes (); ::rocksdb::Options get_db_options (); ::rocksdb::BlockBasedTableOptions get_table_options () const; ::rocksdb::ColumnFamilyOptions get_cf_options (std::string const & cf_name_a) const; diff --git a/nano/store/rocksdb/transaction.cpp b/nano/store/rocksdb/transaction.cpp index abccc8bf6f..5642029cc4 100644 --- a/nano/store/rocksdb/transaction.cpp +++ b/nano/store/rocksdb/transaction.cpp @@ -32,13 +32,9 @@ void * nano::store::rocksdb::read_transaction_impl::get_handle () const return (void *)&options; } -nano::store::rocksdb::write_transaction_impl::write_transaction_impl (::rocksdb::TransactionDB * db_a, std::vector const & tables_requiring_locks_a, std::vector const & tables_no_locks_a, std::unordered_map & mutexes_a) : - db (db_a), - tables_requiring_locks (tables_requiring_locks_a), - tables_no_locks (tables_no_locks_a), - mutexes (mutexes_a) +nano::store::rocksdb::write_transaction_impl::write_transaction_impl (::rocksdb::TransactionDB * db_a) : + db (db_a) { - lock (); ::rocksdb::TransactionOptions txn_options; txn_options.set_snapshot = true; txn = db->BeginTransaction (::rocksdb::WriteOptions (), txn_options); @@ -48,7 +44,6 @@ nano::store::rocksdb::write_transaction_impl::~write_transaction_impl () { commit (); delete txn; - unlock (); } void nano::store::rocksdb::write_transaction_impl::commit () @@ -74,23 +69,7 @@ void * nano::store::rocksdb::write_transaction_impl::get_handle () const return txn; } -void nano::store::rocksdb::write_transaction_impl::lock () -{ - for (auto table : tables_requiring_locks) - { - mutexes.at (table).lock (); - } -} - -void nano::store::rocksdb::write_transaction_impl::unlock () -{ - for (auto table : tables_requiring_locks) - { - mutexes.at (table).unlock (); - } -} - bool nano::store::rocksdb::write_transaction_impl::contains (nano::tables table_a) const { - return (std::find (tables_requiring_locks.begin (), tables_requiring_locks.end (), table_a) != tables_requiring_locks.end ()) || (std::find (tables_no_locks.begin (), tables_no_locks.end (), table_a) != tables_no_locks.end ()); + return true; } diff --git a/nano/store/rocksdb/transaction_impl.hpp b/nano/store/rocksdb/transaction_impl.hpp index ab9429fcd8..9ccbedc184 100644 --- a/nano/store/rocksdb/transaction_impl.hpp +++ b/nano/store/rocksdb/transaction_impl.hpp @@ -26,7 +26,7 @@ class read_transaction_impl final : public store::read_transaction_impl class write_transaction_impl final : public store::write_transaction_impl { public: - write_transaction_impl (::rocksdb::TransactionDB * db_a, std::vector const & tables_requiring_locks_a, std::vector const & tables_no_locks_a, std::unordered_map & mutexes_a); + write_transaction_impl (::rocksdb::TransactionDB * db_a); ~write_transaction_impl (); void commit () override; void renew () override; @@ -36,12 +36,6 @@ class write_transaction_impl final : public store::write_transaction_impl private: ::rocksdb::Transaction * txn; ::rocksdb::TransactionDB * db; - std::vector tables_requiring_locks; - std::vector tables_no_locks; - std::unordered_map & mutexes; bool active{ true }; - - void lock (); - void unlock (); }; } From dbdf1b1e03a8a0400e8b12b08578a573d028c012 Mon Sep 17 00:00:00 2001 From: Colin LeMahieu Date: Fri, 27 Sep 2024 13:02:49 +0100 Subject: [PATCH 3/4] Remove table locking argument as write_guard provides exclusive write access. --- nano/core_test/active_elections.cpp | 2 +- nano/core_test/ledger.cpp | 8 ++++---- nano/node/blockprocessor.cpp | 2 +- nano/node/confirming_set.cpp | 2 +- nano/node/node.cpp | 10 +++++----- nano/node/online_reps.cpp | 2 +- nano/node/peer_history.cpp | 2 +- nano/node/vote_generator.cpp | 2 +- nano/secure/ledger.cpp | 18 +++++++++--------- nano/secure/ledger.hpp | 2 +- nano/store/component.hpp | 2 +- nano/store/lmdb/lmdb.cpp | 2 +- nano/store/lmdb/lmdb.hpp | 2 +- nano/store/rocksdb/rocksdb.cpp | 4 ++-- nano/store/rocksdb/rocksdb.hpp | 2 +- nano/test_common/testutil.cpp | 2 +- 16 files changed, 32 insertions(+), 32 deletions(-) diff --git a/nano/core_test/active_elections.cpp b/nano/core_test/active_elections.cpp index 38c599df17..3b30dcf409 100644 --- a/nano/core_test/active_elections.cpp +++ b/nano/core_test/active_elections.cpp @@ -1408,7 +1408,7 @@ TEST (active_elections, bound_election_winners) { // Prevent cementing of confirmed blocks - auto guard = node.ledger.tx_begin_write ({}, nano::store::writer::testing); + auto guard = node.ledger.tx_begin_write (nano::store::writer::testing); // Ensure that when the number of election winners reaches the limit, AEC vacancy reflects that ASSERT_TRUE (node.active.vacancy (nano::election_behavior::priority) > 0); diff --git a/nano/core_test/ledger.cpp b/nano/core_test/ledger.cpp index 809ac7b6fc..83d87fe1d8 100644 --- a/nano/core_test/ledger.cpp +++ b/nano/core_test/ledger.cpp @@ -5819,21 +5819,21 @@ TEST (ledger_transaction, write_wait_order) std::latch latch3{ 1 }; auto fut1 = std::async (std::launch::async, [&] { - auto tx = ctx.ledger ().tx_begin_write ({}, nano::store::writer::generic); + auto tx = ctx.ledger ().tx_begin_write (nano::store::writer::generic); acquired1 = true; latch1.wait (); // Wait for the signal to drop tx }); WAIT (250ms); // Allow thread to start auto fut2 = std::async (std::launch::async, [&ctx, &acquired2, &latch2] { - auto tx = ctx.ledger ().tx_begin_write ({}, nano::store::writer::blockprocessor); + auto tx = ctx.ledger ().tx_begin_write (nano::store::writer::blockprocessor); acquired2 = true; latch2.wait (); // Wait for the signal to drop tx }); WAIT (250ms); // Allow thread to start auto fut3 = std::async (std::launch::async, [&ctx, &acquired3, &latch3] { - auto tx = ctx.ledger ().tx_begin_write ({}, nano::store::writer::confirmation_height); + auto tx = ctx.ledger ().tx_begin_write (nano::store::writer::confirmation_height); acquired3 = true; latch3.wait (); // Wait for the signal to drop tx }); @@ -5855,4 +5855,4 @@ TEST (ledger_transaction, write_wait_order) // Signal to continue and drop the third transaction latch3.count_down (); -} \ No newline at end of file +} diff --git a/nano/node/blockprocessor.cpp b/nano/node/blockprocessor.cpp index ba4c8a84fc..9bf2348b59 100644 --- a/nano/node/blockprocessor.cpp +++ b/nano/node/blockprocessor.cpp @@ -319,7 +319,7 @@ auto nano::block_processor::process_batch (nano::unique_lock & lock lock.unlock (); - auto transaction = node.ledger.tx_begin_write ({ tables::accounts, tables::blocks, tables::pending, tables::rep_weights }, nano::store::writer::blockprocessor); + auto transaction = node.ledger.tx_begin_write (nano::store::writer::blockprocessor); nano::timer timer; timer.start (); diff --git a/nano/node/confirming_set.cpp b/nano/node/confirming_set.cpp index 867aa80e1c..5459d0f4f7 100644 --- a/nano/node/confirming_set.cpp +++ b/nano/node/confirming_set.cpp @@ -162,7 +162,7 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) }; { - auto transaction = ledger.tx_begin_write ({ nano::tables::confirmation_height }, nano::store::writer::confirmation_height); + auto transaction = ledger.tx_begin_write (nano::store::writer::confirmation_height); for (auto const & hash : batch) { do diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 9a3b35e2cd..e60e21832a 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -323,7 +323,7 @@ nano::node::node (std::shared_ptr io_ctx_a, std::filesy if (!is_initialized && !flags.read_only) { - auto const transaction (store.tx_begin_write ({ tables::accounts, tables::blocks, tables::confirmation_height, tables::rep_weights })); + auto const transaction = store.tx_begin_write (); // Store was empty meaning we just created it, add the genesis block store.initialize (transaction, ledger.cache, ledger.constants); } @@ -572,7 +572,7 @@ void nano::node::process_active (std::shared_ptr const & incoming) nano::block_status nano::node::process (std::shared_ptr block) { - auto const transaction = ledger.tx_begin_write ({ tables::accounts, tables::blocks, tables::pending, tables::rep_weights }, nano::store::writer::node); + auto const transaction = ledger.tx_begin_write (nano::store::writer::node); return process (transaction, block); } @@ -787,7 +787,7 @@ nano::uint128_t nano::node::minimum_principal_weight () void nano::node::long_inactivity_cleanup () { bool perform_cleanup = false; - auto const transaction (store.tx_begin_write ({ tables::online_weight, tables::peers })); + auto const transaction = store.tx_begin_write (); if (store.online_weight.count (transaction) > 0) { auto sample (store.online_weight.rbegin (transaction)); @@ -994,7 +994,7 @@ void nano::node::ledger_pruning (uint64_t const batch_size_a, bool bootstrap_wei transaction_write_count = 0; if (!pruning_targets.empty () && !stopped) { - auto write_transaction = ledger.tx_begin_write ({ tables::blocks, tables::pruned }, nano::store::writer::pruning); + auto write_transaction = ledger.tx_begin_write (nano::store::writer::pruning); while (!pruning_targets.empty () && transaction_write_count < batch_size_a && !stopped) { auto const & pruning_hash (pruning_targets.front ()); @@ -1348,4 +1348,4 @@ nano::keypair nano::load_or_create_node_id (std::filesystem::path const & applic release_assert (!ofs.fail ()); return kp; } -} \ No newline at end of file +} diff --git a/nano/node/online_reps.cpp b/nano/node/online_reps.cpp index 3fdd2442dd..3b35b3c5c3 100644 --- a/nano/node/online_reps.cpp +++ b/nano/node/online_reps.cpp @@ -40,7 +40,7 @@ void nano::online_reps::sample () lock.unlock (); nano::uint128_t trend_l; { - auto transaction (ledger.store.tx_begin_write ({ tables::online_weight })); + auto transaction = ledger.store.tx_begin_write (); // Discard oldest entries while (ledger.store.online_weight.count (transaction) >= config.network_params.node.max_weight_samples) { diff --git a/nano/node/peer_history.cpp b/nano/node/peer_history.cpp index bf1fa099bb..1c605d9015 100644 --- a/nano/node/peer_history.cpp +++ b/nano/node/peer_history.cpp @@ -81,7 +81,7 @@ void nano::peer_history::run () void nano::peer_history::run_one () { auto live_peers = network.list (); - auto transaction = store.tx_begin_write ({ tables::peers }); + auto transaction = store.tx_begin_write (); // Add or update live peers for (auto const & peer : live_peers) diff --git a/nano/node/vote_generator.cpp b/nano/node/vote_generator.cpp index 4809e873ee..3bdbedee31 100644 --- a/nano/node/vote_generator.cpp +++ b/nano/node/vote_generator.cpp @@ -123,7 +123,7 @@ void nano::vote_generator::process_batch (std::deque & batch) if (is_final) { - transaction_variant_t transaction_variant{ ledger.tx_begin_write ({ tables::final_votes }, nano::store::writer::voting_final) }; + transaction_variant_t transaction_variant{ ledger.tx_begin_write (nano::store::writer::voting_final) }; verify_batch (transaction_variant, batch); diff --git a/nano/secure/ledger.cpp b/nano/secure/ledger.cpp index daf73eacb1..4396c798ce 100644 --- a/nano/secure/ledger.cpp +++ b/nano/secure/ledger.cpp @@ -732,10 +732,10 @@ nano::ledger::~ledger () { } -auto nano::ledger::tx_begin_write (std::vector const & tables_to_lock, nano::store::writer guard_type) const -> secure::write_transaction +auto nano::ledger::tx_begin_write (nano::store::writer guard_type) const -> secure::write_transaction { auto guard = store.write_queue.wait (guard_type); - auto txn = store.tx_begin_write (tables_to_lock); + auto txn = store.tx_begin_write (); return secure::write_transaction{ std::move (txn), std::move (guard) }; } @@ -1292,7 +1292,7 @@ bool nano::ledger::migrate_lmdb_to_rocksdb (std::filesystem::path const & data_p std::atomic count = 0; store.block.for_each_par ( [&] (store::read_transaction const & /*unused*/, auto i, auto n) { - auto rocksdb_transaction (rocksdb_store->tx_begin_write ({}, { nano::tables::blocks })); + auto rocksdb_transaction = rocksdb_store->tx_begin_write (); for (; i != n; ++i) { rocksdb_transaction.refresh_if_needed (); @@ -1317,7 +1317,7 @@ bool nano::ledger::migrate_lmdb_to_rocksdb (std::filesystem::path const & data_p count = 0; store.pending.for_each_par ( [&] (store::read_transaction const & /*unused*/, auto i, auto n) { - auto rocksdb_transaction (rocksdb_store->tx_begin_write ({}, { nano::tables::pending })); + auto rocksdb_transaction = rocksdb_store->tx_begin_write (); for (; i != n; ++i) { rocksdb_transaction.refresh_if_needed (); @@ -1335,7 +1335,7 @@ bool nano::ledger::migrate_lmdb_to_rocksdb (std::filesystem::path const & data_p count = 0; store.confirmation_height.for_each_par ( [&] (store::read_transaction const & /*unused*/, auto i, auto n) { - auto rocksdb_transaction (rocksdb_store->tx_begin_write ({}, { nano::tables::confirmation_height })); + auto rocksdb_transaction = rocksdb_store->tx_begin_write (); for (; i != n; ++i) { rocksdb_transaction.refresh_if_needed (); @@ -1353,7 +1353,7 @@ bool nano::ledger::migrate_lmdb_to_rocksdb (std::filesystem::path const & data_p count = 0; store.account.for_each_par ( [&] (store::read_transaction const & /*unused*/, auto i, auto n) { - auto rocksdb_transaction (rocksdb_store->tx_begin_write ({}, { nano::tables::accounts })); + auto rocksdb_transaction = rocksdb_store->tx_begin_write (); for (; i != n; ++i) { rocksdb_transaction.refresh_if_needed (); @@ -1371,7 +1371,7 @@ bool nano::ledger::migrate_lmdb_to_rocksdb (std::filesystem::path const & data_p count = 0; store.rep_weight.for_each_par ( [&] (store::read_transaction const & /*unused*/, auto i, auto n) { - auto rocksdb_transaction (rocksdb_store->tx_begin_write ({}, { nano::tables::rep_weights })); + auto rocksdb_transaction = rocksdb_store->tx_begin_write (); for (; i != n; ++i) { rocksdb_transaction.refresh_if_needed (); @@ -1389,7 +1389,7 @@ bool nano::ledger::migrate_lmdb_to_rocksdb (std::filesystem::path const & data_p count = 0; store.pruned.for_each_par ( [&] (store::read_transaction const & /*unused*/, auto i, auto n) { - auto rocksdb_transaction (rocksdb_store->tx_begin_write ({}, { nano::tables::pruned })); + auto rocksdb_transaction = rocksdb_store->tx_begin_write (); for (; i != n; ++i) { rocksdb_transaction.refresh_if_needed (); @@ -1407,7 +1407,7 @@ bool nano::ledger::migrate_lmdb_to_rocksdb (std::filesystem::path const & data_p count = 0; store.final_vote.for_each_par ( [&] (store::read_transaction const & /*unused*/, auto i, auto n) { - auto rocksdb_transaction (rocksdb_store->tx_begin_write ({}, { nano::tables::final_votes })); + auto rocksdb_transaction = rocksdb_store->tx_begin_write (); for (; i != n; ++i) { rocksdb_transaction.refresh_if_needed (); diff --git a/nano/secure/ledger.hpp b/nano/secure/ledger.hpp index 3419731e11..4e3f45d5fd 100644 --- a/nano/secure/ledger.hpp +++ b/nano/secure/ledger.hpp @@ -39,7 +39,7 @@ class ledger final ~ledger (); /** Start read-write transaction */ - secure::write_transaction tx_begin_write (std::vector const & tables_to_lock = {}, nano::store::writer guard_type = nano::store::writer::generic) const; + secure::write_transaction tx_begin_write (nano::store::writer guard_type = nano::store::writer::generic) const; /** Start read-only transaction */ secure::read_transaction tx_begin_read () const; diff --git a/nano/store/component.hpp b/nano/store/component.hpp index e85de20aa4..f36f837efe 100644 --- a/nano/store/component.hpp +++ b/nano/store/component.hpp @@ -96,7 +96,7 @@ namespace store virtual bool init_error () const = 0; /** Start read-write transaction */ - virtual write_transaction tx_begin_write (std::vector const & tables_to_lock = {}, std::vector const & tables_no_lock = {}) = 0; + virtual write_transaction tx_begin_write () = 0; /** Start read-only transaction */ virtual read_transaction tx_begin_read () const = 0; diff --git a/nano/store/lmdb/lmdb.cpp b/nano/store/lmdb/lmdb.cpp index b9bc04721a..a9d83eb483 100644 --- a/nano/store/lmdb/lmdb.cpp +++ b/nano/store/lmdb/lmdb.cpp @@ -162,7 +162,7 @@ void nano::store::lmdb::component::serialize_memory_stats (boost::property_tree: json.put ("page_size", stats.ms_psize); } -nano::store::write_transaction nano::store::lmdb::component::tx_begin_write (std::vector const &, std::vector const &) +nano::store::write_transaction nano::store::lmdb::component::tx_begin_write () { return env.tx_begin_write (create_txn_callbacks ()); } diff --git a/nano/store/lmdb/lmdb.hpp b/nano/store/lmdb/lmdb.hpp index 290b19f813..5920caa4b1 100644 --- a/nano/store/lmdb/lmdb.hpp +++ b/nano/store/lmdb/lmdb.hpp @@ -64,7 +64,7 @@ class component : public nano::store::component public: component (nano::logger &, std::filesystem::path const &, nano::ledger_constants & constants, nano::txn_tracking_config const & txn_tracking_config_a = nano::txn_tracking_config{}, std::chrono::milliseconds block_processor_batch_max_time_a = std::chrono::milliseconds (5000), nano::lmdb_config const & lmdb_config_a = nano::lmdb_config{}, bool backup_before_upgrade = false); - store::write_transaction tx_begin_write (std::vector const & tables_requiring_lock = {}, std::vector const & tables_no_lock = {}) override; + store::write_transaction tx_begin_write () override; store::read_transaction tx_begin_read () const override; std::string vendor_get () const override; diff --git a/nano/store/rocksdb/rocksdb.cpp b/nano/store/rocksdb/rocksdb.cpp index d17cbe28e9..391364fa8a 100644 --- a/nano/store/rocksdb/rocksdb.cpp +++ b/nano/store/rocksdb/rocksdb.cpp @@ -417,9 +417,9 @@ std::vector nano::store::rocksdb::component::cr return column_families; } -nano::store::write_transaction nano::store::rocksdb::component::tx_begin_write (std::vector const &, std::vector const &) +nano::store::write_transaction nano::store::rocksdb::component::tx_begin_write () { - release_assert (db != nullptr); + release_assert (transaction_db != nullptr); return store::write_transaction{ std::make_unique (transaction_db) }; } diff --git a/nano/store/rocksdb/rocksdb.hpp b/nano/store/rocksdb/rocksdb.hpp index 643a67a672..1d964b8ee1 100644 --- a/nano/store/rocksdb/rocksdb.hpp +++ b/nano/store/rocksdb/rocksdb.hpp @@ -66,7 +66,7 @@ class component : public nano::store::component explicit component (nano::logger &, std::filesystem::path const &, nano::ledger_constants & constants, nano::rocksdb_config const & = nano::rocksdb_config{}, bool open_read_only = false); - store::write_transaction tx_begin_write (std::vector const & tables_requiring_lock = {}, std::vector const & tables_no_lock = {}) override; + store::write_transaction tx_begin_write () override; store::read_transaction tx_begin_read () const override; std::string vendor_get () const override; diff --git a/nano/test_common/testutil.cpp b/nano/test_common/testutil.cpp index 59554846fa..06e5ed505d 100644 --- a/nano/test_common/testutil.cpp +++ b/nano/test_common/testutil.cpp @@ -68,7 +68,7 @@ nano::account nano::test::random_account () bool nano::test::process (nano::node & node, std::vector> blocks) { - auto const transaction = node.ledger.tx_begin_write ({ tables::accounts, tables::blocks, tables::pending, tables::rep_weights }); + auto const transaction = node.ledger.tx_begin_write (); for (auto & block : blocks) { auto result = node.process (transaction, block); From 9320e81fb569d74fc36d91548f6747ddc0f89c6f Mon Sep 17 00:00:00 2001 From: Colin LeMahieu Date: Sat, 28 Sep 2024 13:51:57 +0100 Subject: [PATCH 4/4] Add single transaction sanity check. --- nano/store/rocksdb/transaction.cpp | 8 ++++++++ nano/store/rocksdb/transaction_impl.hpp | 2 ++ 2 files changed, 10 insertions(+) diff --git a/nano/store/rocksdb/transaction.cpp b/nano/store/rocksdb/transaction.cpp index 5642029cc4..c812eaa71e 100644 --- a/nano/store/rocksdb/transaction.cpp +++ b/nano/store/rocksdb/transaction.cpp @@ -35,6 +35,7 @@ void * nano::store::rocksdb::read_transaction_impl::get_handle () const nano::store::rocksdb::write_transaction_impl::write_transaction_impl (::rocksdb::TransactionDB * db_a) : db (db_a) { + debug_assert (check_no_write_tx ()); ::rocksdb::TransactionOptions txn_options; txn_options.set_snapshot = true; txn = db->BeginTransaction (::rocksdb::WriteOptions (), txn_options); @@ -73,3 +74,10 @@ bool nano::store::rocksdb::write_transaction_impl::contains (nano::tables table_ { return true; } + +bool nano::store::rocksdb::write_transaction_impl::check_no_write_tx () const +{ + std::vector<::rocksdb::Transaction *> transactions; + db->GetAllPreparedTransactions (&transactions); + return transactions.empty (); +} diff --git a/nano/store/rocksdb/transaction_impl.hpp b/nano/store/rocksdb/transaction_impl.hpp index 9ccbedc184..8d795322a6 100644 --- a/nano/store/rocksdb/transaction_impl.hpp +++ b/nano/store/rocksdb/transaction_impl.hpp @@ -34,6 +34,8 @@ class write_transaction_impl final : public store::write_transaction_impl bool contains (nano::tables table_a) const override; private: + bool check_no_write_tx () const; + ::rocksdb::Transaction * txn; ::rocksdb::TransactionDB * db; bool active{ true };