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

@@ -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 ();
 }
 

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<nano::state_block> (key1.pub, 0, nano::test_genesis_key.pub, nano::Gxrb_ratio, send1->hash (), key1.prv, key1.pub, 0));
 	node1.work_generate_blocking (*receive1);

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 ()));
+			if (!node.store.unchecked_del (transaction_a, nano::unchecked_key (hash_a, info.block->hash ())))
-			--node.ledger.cache.unchecked_count;
+			{
+				assert (node.ledger.cache.unchecked_count > 0);
+				--node.ledger.cache.unchecked_count;
+			}
 		}
 		add (info);
 	}

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 ();
 	});

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);
+			if (!store.unchecked_del (transaction, key))
-			--ledger.cache.unchecked_count;
+			{
+				assert (ledger.cache.unchecked_count > 0);
+				--ledger.cache.unchecked_count;
+			}
 		}
 	}
 }

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<nano::state_block> (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;

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<nano::block> const &) = 0;
 	virtual std::vector<nano::unchecked_info> 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<nano::unchecked_key, nano::unchecked_info> unchecked_begin (nano::transaction const &) const = 0;
 	virtual nano::store_iterator<nano::unchecked_key, nano::unchecked_info> unchecked_begin (nano::transaction const &, nano::unchecked_key const &) const = 0;
 	virtual nano::store_iterator<nano::unchecked_key, nano::unchecked_info> unchecked_end () const = 0;

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<Val> value;
+		auto status (get (transaction_a, tables::unchecked, nano::db_val<Val> (unchecked_key_a), value));
+		release_assert (success (status) || not_found (status));
+		return (success (status));
+	}
+
 	std::vector<nano::unchecked_info> unchecked_get (nano::transaction const & transaction_a, nano::block_hash const & hash_a) override
 	{
 		std::vector<nano::unchecked_info> 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<nano::vote> vote_get (nano::transaction const & transaction_a, nano::account const & account_a) override