Pending RPC should sort by absolute amounts when returning a subset (#…

…3060)

nano/node/json_handler.cpp

@@ -2859,11 +2859,15 @@ void nano::json_handler::pending ()
 	const bool include_only_confirmed = request.get<bool> ("include_only_confirmed", false);
 	const bool sorting = request.get<bool> ("sorting", false);
 	auto simple (threshold.is_zero () && !source && !min_version && !sorting); // if simple, response is a list of hashes
+	const bool should_sort = sorting && !simple;
 	if (!ec)
 	{
 		boost::property_tree::ptree peers_l;
 		auto transaction (node.store.tx_begin_read ());
-		for (auto i (node.store.pending_begin (transaction, nano::pending_key (account, 0))), n (node.store.pending_end ()); i != n && nano::pending_key (i->first).account == account && peers_l.size () < count; ++i)
+		// The ptree container is used if there are any children nodes (e.g source/min_version) otherwise the amount container is used.
+		std::vector<std::pair<std::string, boost::property_tree::ptree>> hash_ptree_pairs;
+		std::vector<std::pair<std::string, nano::uint128_t>> hash_amount_pairs;
+		for (auto i (node.store.pending_begin (transaction, nano::pending_key (account, 0))), n (node.store.pending_end ()); i != n && nano::pending_key (i->first).account == account && (should_sort || peers_l.size () < count); ++i)
 		{
 			nano::pending_key const & key (i->first);
 			if (block_confirmed (node, transaction, key.hash, include_active, include_only_confirmed))
@@ -2891,29 +2895,55 @@ void nano::json_handler::pending ()
 							{
 								pending_tree.put ("min_version", epoch_as_string (info.epoch));
 							}
-							peers_l.add_child (key.hash.to_string (), pending_tree);
+
+							if (should_sort)
+							{
+								hash_ptree_pairs.emplace_back (key.hash.to_string (), pending_tree);
+							}
+							else
+							{
+								peers_l.add_child (key.hash.to_string (), pending_tree);
+							}
 						}
 						else
 						{
-							peers_l.put (key.hash.to_string (), info.amount.number ().convert_to<std::string> ());
+							if (should_sort)
+							{
+								hash_amount_pairs.emplace_back (key.hash.to_string (), info.amount.number ());
+							}
+							else
+							{
+								peers_l.put (key.hash.to_string (), info.amount.number ().convert_to<std::string> ());
+							}
 						}
 					}
 				}
 			}
 		}
-		if (sorting && !simple)
+		if (should_sort)
 		{
 			if (source || min_version)
 			{
-				peers_l.sort ([](const auto & child1, const auto & child2) -> bool {
-					return child1.second.template get<nano::uint128_t> ("amount") > child2.second.template get<nano::uint128_t> ("amount");
+				auto mid = hash_ptree_pairs.size () <= count ? hash_ptree_pairs.end () : hash_ptree_pairs.begin () + count;
+				std::partial_sort (hash_ptree_pairs.begin (), mid, hash_ptree_pairs.end (), [](const auto & lhs, const auto & rhs) {
+					return lhs.second.template get<nano::uint128_t> ("amount") > rhs.second.template get<nano::uint128_t> ("amount");
 				});
+				for (auto i = 0; i < hash_ptree_pairs.size () && i < count; ++i)
+				{
+					peers_l.add_child (hash_ptree_pairs[i].first, hash_ptree_pairs[i].second);
+				}
 			}
 			else
 			{
-				peers_l.sort ([](const auto & child1, const auto & child2) -> bool {
-					return child1.second.template get<nano::uint128_t> ("") > child2.second.template get<nano::uint128_t> ("");
+				auto mid = hash_amount_pairs.size () <= count ? hash_amount_pairs.end () : hash_amount_pairs.begin () + count;
+				std::partial_sort (hash_amount_pairs.begin (), mid, hash_amount_pairs.end (), [](const auto & lhs, const auto & rhs) {
+					return lhs.second > rhs.second;
 				});
+
+				for (auto i = 0; i < hash_amount_pairs.size () && i < count; ++i)
+				{
+					peers_l.put (hash_amount_pairs[i].first, hash_amount_pairs[i].second.convert_to<std::string> ());
+				}
 			}
 		}
 		response_l.add_child ("blocks", peers_l);

nano/rpc_test/rpc.cpp

@@ -2398,6 +2398,32 @@ TEST (rpc, pending)
 	reset_confirmation_height (system.nodes.front ()->store, block1->account ());
 	scoped_thread_name_io.renew ();
 	check_block_response_count (0);
+
+	// Sorting with a smaller count than total should give absolute sorted amounts
+	scoped_thread_name_io.reset ();
+	node->store.confirmation_height_put (node->store.tx_begin_write (), nano::dev_genesis_key.pub, { 2, block1->hash () });
+	auto block2 (system.wallet (0)->send_action (nano::dev_genesis_key.pub, key1.pub, 200));
+	auto block3 (system.wallet (0)->send_action (nano::dev_genesis_key.pub, key1.pub, 300));
+	auto block4 (system.wallet (0)->send_action (nano::dev_genesis_key.pub, key1.pub, 400));
+	scoped_thread_name_io.renew ();
+
+	ASSERT_TIMELY (10s, node->ledger.account_pending (node->store.tx_begin_read (), key1.pub) == 1000);
+	ASSERT_TIMELY (5s, !node->active.active (*block4));
+	ASSERT_TIMELY (5s, node->block_confirmed (block4->hash ()));
+
+	request.put ("count", "2");
+
+	{
+		test_response response (request, rpc.config.port, system.io_ctx);
+		ASSERT_TIMELY (5s, response.status != 0);
+		ASSERT_EQ (200, response.status);
+		auto & blocks_node (response.json.get_child ("blocks"));
+		ASSERT_EQ (2, blocks_node.size ());
+		nano::block_hash hash (blocks_node.begin ()->first);
+		nano::block_hash hash1 ((++blocks_node.begin ())->first);
+		ASSERT_EQ (block4->hash (), hash);
+		ASSERT_EQ (block3->hash (), hash1);
+	}
 }
 
 TEST (rpc, pending_burn)