From f8706c9b918e6cb98497e1a8a161d4cd95608f24 Mon Sep 17 00:00:00 2001 From: Wesley Shillingford Date: Sat, 18 Jan 2020 22:19:30 +0000 Subject: [PATCH] Unchecked cache not handling duplicate entries or unchecked_clear (#2488) * Unchecked cache not handling duplicate entries or unchecked_clear * Add comment to make return type of unchecked_del more obvious --- nano/core_test/bootstrap.cpp | 3 ++ nano/core_test/node.cpp | 4 +++ nano/node/blockprocessor.cpp | 29 ++++++++++++++++---- nano/node/json_handler.cpp | 1 + nano/node/node.cpp | 8 ++++-- nano/rpc_test/rpc.cpp | 44 ++++++++++++++++++++++++++++++ nano/secure/blockstore.hpp | 4 ++- nano/secure/blockstore_partial.hpp | 11 +++++++- 8 files changed, 94 insertions(+), 10 deletions(-) diff --git a/nano/core_test/bootstrap.cpp b/nano/core_test/bootstrap.cpp index 95d85993..a0307fa5 100644 --- a/nano/core_test/bootstrap.cpp +++ b/nano/core_test/bootstrap.cpp @@ -605,6 +605,9 @@ TEST (bootstrap_processor, lazy_max_pull_count) { ASSERT_NO_ERROR (system.poll ()); } + + auto transaction = node1->store.tx_begin_read (); + ASSERT_EQ (node1->ledger.cache.unchecked_count, node1->store.unchecked_count (transaction)); node1->stop (); } diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 5fd63493..b9c4a846 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -276,6 +276,9 @@ TEST (node, auto_bootstrap) ASSERT_NO_ERROR (system.poll ()); } + auto transaction = node1->store.tx_begin_read (); + ASSERT_EQ (node1->ledger.cache.unchecked_count, node1->store.unchecked_count (transaction)); + node1->stop (); } @@ -2867,6 +2870,7 @@ TEST (node, block_processor_signatures) { auto transaction (node1.store.tx_begin_write ()); node1.store.unchecked_put (transaction, send5->previous (), send5); + ++node1.ledger.cache.unchecked_count; } auto receive1 (std::make_shared (key1.pub, 0, nano::test_genesis_key.pub, nano::Gxrb_ratio, send1->hash (), key1.prv, key1.pub, 0)); node1.work_generate_blocking (*receive1); diff --git a/nano/node/blockprocessor.cpp b/nano/node/blockprocessor.cpp index bbb6b57c..2b9db6ea 100644 --- a/nano/node/blockprocessor.cpp +++ b/nano/node/blockprocessor.cpp @@ -410,8 +410,15 @@ nano::process_return nano::block_processor::process_one (nano::write_transaction { info_a.modified = nano::seconds_since_epoch (); } - node.store.unchecked_put (transaction_a, nano::unchecked_key (info_a.block->previous (), hash), info_a); - ++node.ledger.cache.unchecked_count; + + nano::unchecked_key unchecked_key (info_a.block->previous (), hash); + auto exists = node.store.unchecked_exists (transaction_a, unchecked_key); + node.store.unchecked_put (transaction_a, unchecked_key, info_a); + if (!exists) + { + ++node.ledger.cache.unchecked_count; + } + node.gap_cache.add (hash); break; } @@ -426,8 +433,15 @@ nano::process_return nano::block_processor::process_one (nano::write_transaction { info_a.modified = nano::seconds_since_epoch (); } - node.store.unchecked_put (transaction_a, nano::unchecked_key (node.ledger.block_source (transaction_a, *(info_a.block)), hash), info_a); - ++node.ledger.cache.unchecked_count; + + nano::unchecked_key unchecked_key (node.ledger.block_source (transaction_a, *(info_a.block)), hash); + auto exists = node.store.unchecked_exists (transaction_a, unchecked_key); + node.store.unchecked_put (transaction_a, unchecked_key, info_a); + if (!exists) + { + ++node.ledger.cache.unchecked_count; + } + node.gap_cache.add (hash); break; } @@ -526,8 +540,11 @@ void nano::block_processor::queue_unchecked (nano::write_transaction const & tra { if (!node.flags.fast_bootstrap) { - node.store.unchecked_del (transaction_a, nano::unchecked_key (hash_a, info.block->hash ())); - --node.ledger.cache.unchecked_count; + if (!node.store.unchecked_del (transaction_a, nano::unchecked_key (hash_a, info.block->hash ()))) + { + assert (node.ledger.cache.unchecked_count > 0); + --node.ledger.cache.unchecked_count; + } } add (info); } diff --git a/nano/node/json_handler.cpp b/nano/node/json_handler.cpp index cf90f730..2a99ef89 100644 --- a/nano/node/json_handler.cpp +++ b/nano/node/json_handler.cpp @@ -3934,6 +3934,7 @@ void nano::json_handler::unchecked_clear () node.worker.push_task ([rpc_l]() { auto transaction (rpc_l->node.store.tx_begin_write ()); rpc_l->node.store.unchecked_clear (transaction); + rpc_l->node.ledger.cache.unchecked_count = 0; rpc_l->response_l.put ("success", ""); rpc_l->response_errors (); }); diff --git a/nano/node/node.cpp b/nano/node/node.cpp index cc4ea7e9..f7e2a101 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -457,6 +457,7 @@ startup_time (std::chrono::steady_clock::now ()) { auto transaction (store.tx_begin_write ()); store.unchecked_clear (transaction); + ledger.cache.unchecked_count = 0; logger.always_log ("Dropping unchecked blocks"); } } @@ -936,8 +937,11 @@ void nano::node::unchecked_cleanup () { auto key (cleaning_list.front ()); cleaning_list.pop_front (); - store.unchecked_del (transaction, key); - --ledger.cache.unchecked_count; + if (!store.unchecked_del (transaction, key)) + { + assert (ledger.cache.unchecked_count > 0); + --ledger.cache.unchecked_count; + } } } } diff --git a/nano/rpc_test/rpc.cpp b/nano/rpc_test/rpc.cpp index 08e27cf5..72b53379 100644 --- a/nano/rpc_test/rpc.cpp +++ b/nano/rpc_test/rpc.cpp @@ -6514,6 +6514,50 @@ TEST (rpc, unchecked_get) } } +TEST (rpc, unchecked_clear) +{ + nano::system system; + auto & node = *add_ipc_enabled_node (system); + nano::keypair key; + nano::node_rpc_config node_rpc_config; + nano::ipc::ipc_server ipc_server (node, node_rpc_config); + nano::rpc_config rpc_config (nano::get_available_port (), true); + rpc_config.rpc_process.ipc_port = node.config.ipc_config.transport_tcp.port; + nano::ipc_rpc_processor ipc_rpc_processor (system.io_ctx, rpc_config); + nano::rpc rpc (system.io_ctx, rpc_config, ipc_rpc_processor); + rpc.start (); + auto open (std::make_shared (key.pub, 0, key.pub, 1, key.pub, key.prv, key.pub, *system.work.generate (key.pub))); + node.process_active (open); + node.block_processor.flush (); + boost::property_tree::ptree request; + ASSERT_EQ (node.ledger.cache.unchecked_count, 1); + { + auto transaction = node.store.tx_begin_read (); + ASSERT_EQ (node.store.unchecked_count (transaction), 1); + } + request.put ("action", "unchecked_clear"); + test_response response (request, rpc.config.port, system.io_ctx); + system.deadline_set (5s); + while (response.status == 0) + { + ASSERT_NO_ERROR (system.poll ()); + } + ASSERT_EQ (200, response.status); + + system.deadline_set (5s); + while (true) + { + auto transaction = node.store.tx_begin_read (); + if (node.store.unchecked_count (transaction) == 0) + { + break; + } + ASSERT_NO_ERROR (system.poll ()); + } + + ASSERT_EQ (node.ledger.cache.unchecked_count, 0); +} + TEST (rpc, unopened) { nano::system system; diff --git a/nano/secure/blockstore.hpp b/nano/secure/blockstore.hpp index 6fc066d6..0fcf2c8f 100644 --- a/nano/secure/blockstore.hpp +++ b/nano/secure/blockstore.hpp @@ -702,7 +702,9 @@ public: virtual void unchecked_put (nano::write_transaction const &, nano::unchecked_key const &, nano::unchecked_info const &) = 0; virtual void unchecked_put (nano::write_transaction const &, nano::block_hash const &, std::shared_ptr const &) = 0; virtual std::vector unchecked_get (nano::transaction const &, nano::block_hash const &) = 0; - virtual void unchecked_del (nano::write_transaction const &, nano::unchecked_key const &) = 0; + virtual bool unchecked_exists (nano::transaction const & transaction_a, nano::unchecked_key const & unchecked_key_a) = 0; + /* Returns true if nothing was deleted because it was not found, false otherwise */ + virtual bool unchecked_del (nano::write_transaction const &, nano::unchecked_key const &) = 0; virtual nano::store_iterator unchecked_begin (nano::transaction const &) const = 0; virtual nano::store_iterator unchecked_begin (nano::transaction const &, nano::unchecked_key const &) const = 0; virtual nano::store_iterator unchecked_end () const = 0; diff --git a/nano/secure/blockstore_partial.hpp b/nano/secure/blockstore_partial.hpp index e4d716ba..ab723a3c 100644 --- a/nano/secure/blockstore_partial.hpp +++ b/nano/secure/blockstore_partial.hpp @@ -77,6 +77,14 @@ public: return iterator != pending_end () && nano::pending_key (iterator->first) == key_a; } + bool unchecked_exists (nano::transaction const & transaction_a, nano::unchecked_key const & unchecked_key_a) override + { + nano::db_val value; + auto status (get (transaction_a, tables::unchecked, nano::db_val (unchecked_key_a), value)); + release_assert (success (status) || not_found (status)); + return (success (status)); + } + std::vector unchecked_get (nano::transaction const & transaction_a, nano::block_hash const & hash_a) override { std::vector result; @@ -477,10 +485,11 @@ public: release_assert (success (status)); } - void unchecked_del (nano::write_transaction const & transaction_a, nano::unchecked_key const & key_a) override + bool unchecked_del (nano::write_transaction const & transaction_a, nano::unchecked_key const & key_a) override { auto status (del (transaction_a, tables::unchecked, key_a)); release_assert (success (status) || not_found (status)); + return not_found (status); } std::shared_ptr vote_get (nano::transaction const & transaction_a, nano::account const & account_a) override