From 9a19ec66a3c0e18d03190546299222b7400023c8 Mon Sep 17 00:00:00 2001 From: Andreas Titan Date: Tue, 15 Mar 2022 13:26:58 +0100 Subject: [PATCH] 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 --- nano/node/json_handler.cpp | 6 +- nano/rpc_test/rpc.cpp | 156 +++++++++++++++++++++++++++++++++++-- 2 files changed, 153 insertions(+), 9 deletions(-) diff --git a/nano/node/json_handler.cpp b/nano/node/json_handler.cpp index db696a40fa..d98092fc86 100644 --- 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 ("amount") > rhs.second.template get ("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 index 75b3ad82cf..6f7b27f5a0 100644 --- 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 (""); + 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 ("")); + + // the next 3 block will be of amount 300 but in unspecified order + ++itr; + ASSERT_EQ ("300", itr->second.get ("")); + + ++itr; + ASSERT_EQ ("300", itr->second.get ("")); + + ++itr; + ASSERT_EQ ("300", itr->second.get ("")); + + // 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 ("")); + + // 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 ("")); + + ++itr; + ASSERT_EQ (block1->hash (), nano::block_hash{ itr->first }); + ASSERT_EQ ("200", itr->second.get ("")); + + ++itr; + ASSERT_EQ (block2->hash (), nano::block_hash{ itr->first }); + ASSERT_EQ ("100", itr->second.get ("")); + } + + 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 ("amount")); + + ++itr; + ASSERT_EQ ("300", itr->second.get ("amount")); + + ++itr; + ASSERT_EQ (block1->hash (), nano::block_hash{ itr->first }); + ASSERT_EQ ("200", itr->second.get ("amount")); } }