Do not sort by hash in receivable rpc command handler

The fact that the results are sorted by hash is an implementation detail so we shouldn't test for it and shouldn't make API decisions that assume that the results will always be sorted by hash internally. Replace partial_sort with stable_sort so that the ordering of equal elements is not re-ordered and are left as the internal implementations laid them out. This is to ensure that when using count and offset to walk through the results, no result falls throw the gap due to ordering inconsistencies. Co-authored-by: Dimitrios Siganos <[email protected]>
nanocurrency · Mar 15, 2022 · 9a19ec6 · 9a19ec6
1 parent 4512cfd
commit 9a19ec6
Show file tree

Hide file tree

Showing 2 changed files with 153 additions and 9 deletions.
diff --git a/nano/node/json_handler.cpp b/nano/node/json_handler.cpp
@@ -3027,8 +3027,7 @@ void nano::json_handler::pending ()
 		{
 			if (source || min_version)
 			{
-				auto mid = hash_ptree_pairs.size () <= (offset + count) ? hash_ptree_pairs.end () : hash_ptree_pairs.begin () + offset + count;
-				std::partial_sort (hash_ptree_pairs.begin (), mid, hash_ptree_pairs.end (), [] (auto const & lhs, auto const & rhs) {
+				std::stable_sort (hash_ptree_pairs.begin (), hash_ptree_pairs.end (), [] (auto const & lhs, auto const & rhs) {
 					return lhs.second.template get<nano::uint128_t> ("amount") > rhs.second.template get<nano::uint128_t> ("amount");
 				});
 				for (auto i = offset, j = offset + count; i < hash_ptree_pairs.size () && i < j; ++i)
@@ -3038,8 +3037,7 @@ void nano::json_handler::pending ()
 			}
 			else
 			{
-				auto mid = hash_amount_pairs.size () <= (offset + count) ? hash_amount_pairs.end () : hash_amount_pairs.begin () + offset + count;
-				std::partial_sort (hash_amount_pairs.begin (), mid, hash_amount_pairs.end (), [] (auto const & lhs, auto const & rhs) {
+				std::stable_sort (hash_amount_pairs.begin (), hash_amount_pairs.end (), [] (auto const & lhs, auto const & rhs) {
 					return lhs.second > rhs.second;
 				});
 

diff --git a/nano/rpc_test/rpc.cpp b/nano/rpc_test/rpc.cpp
@@ -1858,16 +1858,162 @@ TEST (rpc, pending)
 		ASSERT_EQ (block4->hash (), hash);
 		ASSERT_EQ (block3->hash (), hash1);
 	}
+}
+
+/**
+ * This test case tests the receivable RPC command when used with offsets and sorting.
+ */
+TEST (rpc, receivable_offset_and_sorting)
+{
+	nano::system system;
+	auto node = add_ipc_enabled_node (system);
+	nano::keypair key1;
+	system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv);
+
+	auto block1 = system.wallet (0)->send_action (nano::dev::genesis_key.pub, key1.pub, 200);
+	auto block2 = system.wallet (0)->send_action (nano::dev::genesis_key.pub, key1.pub, 100);
+	auto block3 = system.wallet (0)->send_action (nano::dev::genesis_key.pub, key1.pub, 400);
+	auto block4 = system.wallet (0)->send_action (nano::dev::genesis_key.pub, key1.pub, 300);
+	auto block5 = system.wallet (0)->send_action (nano::dev::genesis_key.pub, key1.pub, 300);
+	auto block6 = system.wallet (0)->send_action (nano::dev::genesis_key.pub, key1.pub, 300);
+
+	// check that all blocks got confirmed
+	ASSERT_TIMELY (5s, node->ledger.account_pending (node->store.tx_begin_read (), key1.pub, true) == 1600);
+
+	// check confirmation height is as expected, there is no perfect clarity yet when confirmation height updates after a block get confirmed
+	nano::confirmation_height_info confirmation_height_info;
+	ASSERT_FALSE (node->store.confirmation_height.get (node->store.tx_begin_read (), nano::dev::genesis->account (), confirmation_height_info));
+	ASSERT_EQ (confirmation_height_info.height, 7);
+	ASSERT_EQ (confirmation_height_info.frontier, block6->hash ());
+
+	// returns true if hash is found in node
+	// if match_first is set then the function looks for key (first item)
+	// if match_first is not set then the function looks for value (second item)
+	auto hash_exists = [] (boost::property_tree::ptree & node, bool match_first, nano::block_hash hash) {
+		std::stringstream ss;
+		boost::property_tree::json_parser::write_json (ss, node);
+		for (auto itr = node.begin (); itr != node.end (); ++itr)
+		{
+			std::string possible_match = match_first ? itr->first : itr->second.get<std::string> ("");
+			if (possible_match == hash.to_string ())
+			{
+				return true;
+			}
+		}
+		return false;
+	};
 
-	request.put ("offset", "2"); // Offset test
+	auto const rpc_ctx = add_rpc (system, node);
+	boost::property_tree::ptree request;
+	request.put ("action", "receivable");
+	request.put ("account", key1.pub.to_account ());
+
+	request.put ("offset", "0");
+	request.put ("sorting", "false");
 	{
 		auto response (wait_response (system, rpc_ctx, request));
 		auto & blocks_node (response.get_child ("blocks"));
+		ASSERT_EQ (6, blocks_node.size ());
+
+		// check that all 6 blocks are listed, the order does not matter
+		ASSERT_TRUE (hash_exists (blocks_node, false, block1->hash ()));
+		ASSERT_TRUE (hash_exists (blocks_node, false, block2->hash ()));
+		ASSERT_TRUE (hash_exists (blocks_node, false, block3->hash ()));
+		ASSERT_TRUE (hash_exists (blocks_node, false, block4->hash ()));
+		ASSERT_TRUE (hash_exists (blocks_node, false, block5->hash ()));
+		ASSERT_TRUE (hash_exists (blocks_node, false, block6->hash ()));
+	}
+
+	request.put ("offset", "4");
+	{
+		auto response (wait_response (system, rpc_ctx, request));
+		auto & blocks_node (response.get_child ("blocks"));
+		// since we haven't asked for sorted, we can't be sure which 2 blocks will be returned
 		ASSERT_EQ (2, blocks_node.size ());
-		nano::block_hash hash (blocks_node.begin ()->first);
-		nano::block_hash hash1 ((++blocks_node.begin ())->first);
-		ASSERT_EQ (block2->hash (), hash);
-		ASSERT_EQ (block1->hash (), hash1);
+	}
+
+	request.put ("count", "2");
+	request.put ("offset", "2");
+	{
+		auto response (wait_response (system, rpc_ctx, request));
+		auto & blocks_node (response.get_child ("blocks"));
+		// since we haven't asked for sorted, we can't be sure which 2 blocks will be returned
+		ASSERT_EQ (2, blocks_node.size ());
+	}
+
+	// Sort by amount from here onwards, this is a sticky setting that applies for the rest of the test case
+	request.put ("sorting", "true");
+
+	request.put ("count", "5");
+	request.put ("offset", "0");
+	{
+		auto response (wait_response (system, rpc_ctx, request));
+		auto & blocks_node (response.get_child ("blocks"));
+		ASSERT_EQ (5, blocks_node.size ());
+
+		// the first block should be block3 with amount 400
+		auto itr = blocks_node.begin ();
+		ASSERT_EQ (block3->hash (), nano::block_hash{ itr->first });
+		ASSERT_EQ ("400", itr->second.get<std::string> (""));
+
+		// the next 3 block will be of amount 300 but in unspecified order
+		++itr;
+		ASSERT_EQ ("300", itr->second.get<std::string> (""));
+
+		++itr;
+		ASSERT_EQ ("300", itr->second.get<std::string> (""));
+
+		++itr;
+		ASSERT_EQ ("300", itr->second.get<std::string> (""));
+
+		// the last one will be block1 with amount 200
+		++itr;
+		ASSERT_EQ (block1->hash (), nano::block_hash{ itr->first });
+		ASSERT_EQ ("200", itr->second.get<std::string> (""));
+
+		// check that the blocks returned with 300 amounts have the right hashes
+		ASSERT_TRUE (hash_exists (blocks_node, true, block4->hash ()));
+		ASSERT_TRUE (hash_exists (blocks_node, true, block5->hash ()));
+		ASSERT_TRUE (hash_exists (blocks_node, true, block6->hash ()));
+	}
+
+	request.put ("count", "3");
+	request.put ("offset", "3");
+	{
+		auto response (wait_response (system, rpc_ctx, request));
+		auto & blocks_node (response.get_child ("blocks"));
+		ASSERT_EQ (3, blocks_node.size ());
+
+		auto itr = blocks_node.begin ();
+		ASSERT_EQ ("300", itr->second.get<std::string> (""));
+
+		++itr;
+		ASSERT_EQ (block1->hash (), nano::block_hash{ itr->first });
+		ASSERT_EQ ("200", itr->second.get<std::string> (""));
+
+		++itr;
+		ASSERT_EQ (block2->hash (), nano::block_hash{ itr->first });
+		ASSERT_EQ ("100", itr->second.get<std::string> (""));
+	}
+
+	request.put ("source", "true");
+	request.put ("min_version", "true");
+	request.put ("count", "3");
+	request.put ("offset", "2");
+	{
+		auto response (wait_response (system, rpc_ctx, request));
+		auto & blocks_node (response.get_child ("blocks"));
+		ASSERT_EQ (3, blocks_node.size ());
+
+		auto itr = blocks_node.begin ();
+		ASSERT_EQ ("300", itr->second.get<std::string> ("amount"));
+
+		++itr;
+		ASSERT_EQ ("300", itr->second.get<std::string> ("amount"));
+
+		++itr;
+		ASSERT_EQ (block1->hash (), nano::block_hash{ itr->first });
+		ASSERT_EQ ("200", itr->second.get<std::string> ("amount"));
 	}
 }