Minecon724/dncurrency
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages 1 Wiki Activity Actions 1

Do not sort by hash in receivable rpc command handler

Browse source 
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 <dimitris@siganos.org>
This commit is contained in:
Andreas Titan 2022-03-15 13:26:58 +01:00 • committed by GitHub
commit 9a19ec66a3
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 153 additions and 9 deletions
Download patch file Download diff file Expand all files Collapse all files

6
nano/node/json_handler.cpp
View file

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

156
nano/rpc_test/rpc.cpp
View file

@ -1858,16 +1858,162 @@ TEST (rpc, pending)
ASSERT_EQ (block4->hash (), hash);
ASSERT_EQ (block3->hash (), hash1);
}
}
request.put ("offset", "2"); // Offset test
/**
* 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;
};
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"));
}
}