Merge branch 'develop' into accounts_frontiers_per_item_response

CMakeLists.txt

@@ -615,13 +615,29 @@ if(NANO_TEST OR RAIBLOCKS_TEST)
     COMMAND echo "BATCH BUILDING TESTS"
     WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
     DEPENDS core_test load_test rpc_test nano_node nano_rpc)
-
   add_custom_target(
     run_tests
     COMMAND ${PROJECT_SOURCE_DIR}/ci/test.sh ${CMAKE_BINARY_DIR}
     WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
     DEPENDS build_tests)
 endif()
+
+if(NANO_TEST OR RAIBLOCKS_TEST)
+  if(NANO_GUI OR RAIBLOCKS_GUI)
+    add_custom_target(
+      quick_tests
+      COMMAND echo "BATCH QUICK TESTS"
+      WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
+      DEPENDS core_test rpc_test qt_test)
+  else()
+    add_custom_target(
+      quick_tests
+      COMMAND echo "BATCH QUICK TESTS -- qt_test is disabled"
+      WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
+      DEPENDS core_test rpc_test)
+  endif()
+endif()
+
 if(NANO_GUI OR RAIBLOCKS_GUI)
   install(FILES ${PROJECT_BINARY_DIR}/config-node.toml.sample DESTINATION .)
   install(FILES ${PROJECT_BINARY_DIR}/config-rpc.toml.sample DESTINATION .)

nano/core_test/node.cpp

@@ -1629,13 +1629,15 @@ TEST (node, bootstrap_fork_open)
 // Unconfirmed blocks from bootstrap should be confirmed
 TEST (node, bootstrap_confirm_frontiers)
 {
+	// create 2 separate systems, the 2 system do not interact with each other automatically
 	nano::system system0 (1);
 	nano::system system1 (1);
-	auto node0 (system0.nodes[0]);
-	auto node1 (system1.nodes[0]);
+	auto node0 = system0.nodes[0];
+	auto node1 = system1.nodes[0];
 	system0.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv);
 	nano::keypair key0;
-	// node0 knows about send0 but node1 doesn't.
+
+	// create block to send 500 raw from genesis to key0 and save into node0 ledger without immediately triggering an election
 	auto send0 = nano::send_block_builder ()
 				 .previous (nano::dev::genesis->hash ())
 				 .destination (key0.pub)
@@ -1645,39 +1647,23 @@ TEST (node, bootstrap_confirm_frontiers)
 				 .build_shared ();
 	ASSERT_EQ (nano::process_result::progress, node0->process (*send0).code);
 
+	// each system only has one node, so there should be no bootstrapping going on
 	ASSERT_FALSE (node0->bootstrap_initiator.in_progress ());
 	ASSERT_FALSE (node1->bootstrap_initiator.in_progress ());
 	ASSERT_TRUE (node1->active.empty ());
-	node1->bootstrap_initiator.bootstrap (node0->network.endpoint ()); // Additionally add new peer to confirm bootstrap frontier
-	system1.deadline_set (10s);
-	while (node1->block (send0->hash ()) == nullptr)
-	{
-		ASSERT_NO_ERROR (system0.poll ());
-		ASSERT_NO_ERROR (system1.poll ());
-	}
-	// Wait for election start
-	system1.deadline_set (10s);
-	while (node1->active.empty ())
-	{
-		ASSERT_NO_ERROR (system0.poll ());
-		ASSERT_NO_ERROR (system1.poll ());
-	}
-	{
-		nano::lock_guard<nano::mutex> guard (node1->active.mutex);
-		auto existing1 (node1->active.blocks.find (send0->hash ()));
-		ASSERT_NE (node1->active.blocks.end (), existing1);
-	}
-	// Wait for confirmation height update
+
+	// create a bootstrap connection from node1 to node0
+	// this also has the side effect of adding node0 to node1's list of peers, which will trigger realtime connections too
+	node1->bootstrap_initiator.bootstrap (node0->network.endpoint ());
+
+	// Wait until the block is confirmed on node1. Poll more than usual because we are polling
+	// on 2 different systems at once and in sequence and there might be strange timing effects.
+	system0.deadline_set (10s);
 	system1.deadline_set (10s);
-	bool done (false);
-	while (!done)
+	while (!node1->ledger.block_confirmed (node1->store.tx_begin_read (), send0->hash ()))
 	{
-		{
-			auto transaction (node1->store.tx_begin_read ());
-			done = node1->ledger.block_confirmed (transaction, send0->hash ());
-		}
-		ASSERT_NO_ERROR (system0.poll ());
-		ASSERT_NO_ERROR (system1.poll ());
+		ASSERT_NO_ERROR (system0.poll (std::chrono::milliseconds (1)));
+		ASSERT_NO_ERROR (system1.poll (std::chrono::milliseconds (1)));
 	}
 }
 

nano/core_test/uint256_union.cpp

@@ -375,6 +375,20 @@ TEST (uint256_union, decode_nano_variant)
 	ASSERT_FALSE (key.decode_account ("nano_1111111111111111111111111111111111111111111111111111hifc8npp"));
 }
 
+/**
+ * It used to be the case that when the address was wrong only in the checksum part
+ * then the decode_account would return error and it would also write the address with
+ * fixed checksum into 'key', which is not desirable.
+ */
+TEST (uint256_union, key_is_not_updated_on_checksum_error)
+{
+	nano::account key;
+	ASSERT_EQ (key, 0);
+	bool result = key.decode_account ("nano_3e3j5tkog48pnny9dmfzj1r16pg8t1e76dz5tmac6iq689wyjfpiij4txtd1");
+	ASSERT_EQ (key, 0);
+	ASSERT_TRUE (result);
+}
+
 TEST (uint256_union, account_transcode)
 {
 	nano::account value;

nano/lib/numbers.cpp

@@ -116,14 +116,18 @@ bool nano::public_key::decode_account (std::string const & source_a)
 					}
 					if (!error)
 					{
-						*this = (number_l >> 40).convert_to<nano::uint256_t> ();
+						nano::public_key temp = (number_l >> 40).convert_to<nano::uint256_t> ();
 						uint64_t check (number_l & static_cast<uint64_t> (0xffffffffff));
 						uint64_t validation (0);
 						blake2b_state hash;
 						blake2b_init (&hash, 5);
-						blake2b_update (&hash, bytes.data (), bytes.size ());
+						blake2b_update (&hash, temp.bytes.data (), temp.bytes.size ());
 						blake2b_final (&hash, reinterpret_cast<uint8_t *> (&validation), 5);
 						error = check != validation;
+						if (!error)
+						{
+							*this = temp;
+						}
 					}
 				}
 				else

nano/node/json_handler.cpp

@@ -14,6 +14,7 @@
 
 #include <algorithm>
 #include <chrono>
+#include <vector>
 
 namespace
 {
@@ -911,18 +912,21 @@ void nano::json_handler::accounts_balances ()
 void nano::json_handler::accounts_representatives ()
 {
 	boost::property_tree::ptree representatives;
-	for (auto & accounts : request.get_child ("accounts"))
+	auto transaction = node.store.tx_begin_read ();
+	for (auto & account_from_request : request.get_child ("accounts"))
 	{
-		auto account (account_impl (accounts.second.data ()));
-		auto transaction (node.store.tx_begin_read ());
-		auto info (account_info_impl (transaction, account));
-
+		auto account = account_impl (account_from_request.second.data ());
 		if (!ec)
 		{
-			boost::property_tree::ptree entry;
-			entry.put ("", info.representative.to_account ());
-			representatives.push_back (std::make_pair (accounts.second.data (), entry));
+			auto info = account_info_impl (transaction, account);
+			if (!ec)
+			{
+				representatives.put (account_from_request.second.data (), info.representative.to_account ());
+				continue;
+			}
 		}
+		representatives.put (account_from_request.second.data (), boost::format ("error: %1%") % ec.message ());
+		ec = {};
 	}
 	response_l.add_child ("representatives", representatives);
 	response_errors ();

nano/rpc_test/rpc.cpp

@@ -3157,6 +3157,7 @@ TEST (rpc, accounts_balances)
 	}
 }
 
+// Tests the  happy path of retrieving an account's representative
 TEST (rpc, accounts_representatives)
 {
 	nano::system system;
@@ -3166,17 +3167,69 @@ TEST (rpc, accounts_representatives)
 	request.put ("action", "accounts_representatives");
 	boost::property_tree::ptree entry;
 	boost::property_tree::ptree accounts;
+	// Adds a valid account present in the ledger.
 	entry.put ("", nano::dev::genesis_key.pub.to_account ());
 	accounts.push_back (std::make_pair ("", entry));
 	request.add_child ("accounts", accounts);
 	auto response (wait_response (system, rpc_ctx, request));
+	// Ensures the response is correct.
 	auto response_representative (response.get_child ("representatives").get<std::string> (nano::dev::genesis->account ().to_account ()));
 	ASSERT_EQ (response_representative, nano::dev::genesis->account ().to_account ());
 }
 
 /**
  * Test the RPC accounts_frontiers with 3 accounts, one good one, one with an invalid account ID and one with an account that does not exist.
  */
+TEST (rpc, accounts_representatives_per_account_result_with_errors)
+{
+	nano::system system;
+	auto node = add_ipc_enabled_node (system);
+	auto const rpc_ctx = add_rpc (system, node);
+	boost::property_tree::ptree request;
+	request.put ("action", "accounts_representatives");
+	boost::property_tree::ptree entry1, entry2, entry3;
+	boost::property_tree::ptree accounts_l;
+
+	// Adds a valid account present in the ledger.
+	entry1.put ("", nano::dev::genesis_key.pub.to_account ());
+	accounts_l.push_back (std::make_pair ("", entry1));
+
+	// Adds an invalid account, malformed number with a wrong checksum.
+	// Got with this formula: key1.substr(0, 40) + key2.substr(40, key2.size()).
+	auto const invalid_key = "nano_36uccgpjzhjsdbj44wm1y5hyz8gefx3wjpp1jircxt84nopxkxti5bzq1rnz";
+	entry2.put ("", invalid_key);
+	accounts_l.push_back (std::make_pair ("", entry2));
+
+	// Adds a valid key but that isn't on the ledger. It wont'be found.
+	auto const valid_key = "nano_1hrts7hcoozxccnffoq9hqhngnn9jz783usapejm57ejtqcyz9dpso1bibuy";
+	entry3.put ("", valid_key);
+	accounts_l.push_back (std::make_pair ("", entry3));
+
+	// Packs all the account entries.
+	request.add_child ("accounts", accounts_l);
+	auto response (wait_response (system, rpc_ctx, request));
+
+	auto get_error_message = [] (nano::error_common error_common) -> std::string {
+		std::error_code ec = error_common;
+		return boost::str (boost::format ("error: %1%") % ec.message ());
+	};
+
+	std::map<std::string, std::string> reply_map{
+		{ nano::dev::genesis_key.pub.to_account (), nano::dev::genesis->account ().to_account () },
+		{ invalid_key, get_error_message (nano::error_common::bad_account_number) },
+		{ valid_key, get_error_message (nano::error_common::account_not_found) }
+	};
+
+	for (auto & representative : response.get_child ("representatives"))
+	{
+		std::string account_text = representative.first;
+		std::string frontier_text = representative.second.get<std::string> ("");
+		ASSERT_EQ (frontier_text, reply_map[account_text]);
+		reply_map.erase (account_text);
+	}
+	ASSERT_EQ (reply_map.size (), 0);
+}
+
 TEST (rpc, accounts_frontiers)
 {
 	nano::system system;