Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix & refactor bootstrap_server #3861

Merged
merged 12 commits into from
Aug 6, 2022
Merged

Fix & refactor bootstrap_server #3861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7b4ca50
Extract `message_deserializer` class
pwojcikdev Jul 4, 2022
c7835cd
Integrating `message_deserializer` into `bootstrap_server`
pwojcikdev Jul 4, 2022
9bf9dfa
Remove adding an empty message from bootstrap test
pwojcikdev Jul 11, 2022
ea0aa67
Stats & error messages
pwojcikdev Jul 5, 2022
a549747
Restore timeout value
pwojcikdev Jul 18, 2022
7da55ed
Function name typo
pwojcikdev Jul 19, 2022
5665066
Use fault error codes in early return conditions
pwojcikdev Jul 19, 2022
f44213d
Adjust max message size comparison
pwojcikdev Jul 19, 2022
f8d3be5
Compilation error & readability
pwojcikdev Jul 20, 2022
e9782df
Properly increase stats when handling early return conditions
pwojcikdev Jul 20, 2022
cc28e7f
Rename to `is_undefined_connection`
pwojcikdev Aug 5, 2022
7f16250
Comments for `message_deserializer`
pwojcikdev Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 0 additions & 29 deletions nano/core_test/bootstrap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ TEST (bulk_pull, no_address)
auto req = std::make_unique<nano::bulk_pull> (nano::dev::network_params.network);
req->start = 1;
req->end = 2;
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::bulk_pull_server> (connection, std::move (req)));
ASSERT_EQ (request->current, request->request->end);
ASSERT_TRUE (request->current.is_zero ());
Expand All @@ -29,7 +28,6 @@ TEST (bulk_pull, genesis_to_end)
auto req = std::make_unique<nano::bulk_pull> (nano::dev::network_params.network);
req->start = nano::dev::genesis_key.pub;
req->end.clear ();
connection->requests.push (nullptr);
auto request (std::make_shared<nano::bulk_pull_server> (connection, std::move (req)));
ASSERT_EQ (system.nodes[0]->latest (nano::dev::genesis_key.pub), request->current);
ASSERT_EQ (request->request->end, request->request->end);
Expand All @@ -43,7 +41,6 @@ TEST (bulk_pull, no_end)
auto req = std::make_unique<nano::bulk_pull> (nano::dev::network_params.network);
req->start = nano::dev::genesis_key.pub;
req->end = 1;
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::bulk_pull_server> (connection, std::move (req)));
ASSERT_EQ (system.nodes[0]->latest (nano::dev::genesis_key.pub), request->current);
ASSERT_TRUE (request->request->end.is_zero ());
Expand Down Expand Up @@ -76,7 +73,6 @@ TEST (bulk_pull, end_not_owned)
auto req = std::make_unique<nano::bulk_pull> (nano::dev::network_params.network);
req->start = key2.pub;
req->end = nano::dev::genesis->hash ();
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::bulk_pull_server> (connection, std::move (req)));
ASSERT_EQ (request->current, request->request->end);
}
Expand All @@ -88,7 +84,6 @@ TEST (bulk_pull, none)
auto req = std::make_unique<nano::bulk_pull> (nano::dev::network_params.network);
req->start = nano::dev::genesis_key.pub;
req->end = nano::dev::genesis->hash ();
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::bulk_pull_server> (connection, std::move (req)));
auto block (request->get_next ());
ASSERT_EQ (nullptr, block);
Expand All @@ -101,12 +96,10 @@ TEST (bulk_pull, get_next_on_open)
auto req = std::make_unique<nano::bulk_pull> (nano::dev::network_params.network);
req->start = nano::dev::genesis_key.pub;
req->end.clear ();
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::bulk_pull_server> (connection, std::move (req)));
auto block (request->get_next ());
ASSERT_NE (nullptr, block);
ASSERT_TRUE (block->previous ().is_zero ());
ASSERT_FALSE (connection->requests.empty ());
ASSERT_EQ (request->current, request->request->end);
}

Expand Down Expand Up @@ -135,7 +128,6 @@ TEST (bulk_pull, ascending_one_hash)
req->start = nano::dev::genesis->hash ();
req->end = nano::dev::genesis->hash ();
req->header.flag_set (nano::message_header::bulk_pull_ascending_flag);
connection->requests.push (std::unique_ptr<nano::message>{});
auto request = std::make_shared<nano::bulk_pull_server> (connection, std::move (req));
auto block_out1 = request->get_next ();
ASSERT_NE (nullptr, block_out1);
Expand Down Expand Up @@ -168,7 +160,6 @@ TEST (bulk_pull, ascending_two_account)
req->start = nano::dev::genesis->hash ();
req->end.clear ();
req->header.flag_set (nano::message_header::bulk_pull_ascending_flag);
connection->requests.push (std::unique_ptr<nano::message>{});
auto request = std::make_shared<nano::bulk_pull_server> (connection, std::move (req));
auto block_out1 = request->get_next ();
ASSERT_NE (nullptr, block_out1);
Expand Down Expand Up @@ -204,7 +195,6 @@ TEST (bulk_pull, ascending_end)
req->start = nano::dev::genesis_key.pub;
req->end = block1->hash ();
req->header.flag_set (nano::message_header::bulk_pull_ascending_flag);
connection->requests.push (std::unique_ptr<nano::message>{});
auto request = std::make_shared<nano::bulk_pull_server> (connection, std::move (req));
auto block_out1 = request->get_next ();
ASSERT_NE (nullptr, block_out1);
Expand All @@ -219,7 +209,6 @@ TEST (bulk_pull, by_block)
auto req = std::make_unique<nano::bulk_pull> (nano::dev::network_params.network);
req->start = nano::dev::genesis->hash ();
req->end.clear ();
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::bulk_pull_server> (connection, std::move (req)));
auto block (request->get_next ());
ASSERT_NE (nullptr, block);
Expand All @@ -236,7 +225,6 @@ TEST (bulk_pull, by_block_single)
auto req = std::make_unique<nano::bulk_pull> (nano::dev::network_params.network);
req->start = nano::dev::genesis->hash ();
req->end = nano::dev::genesis->hash ();
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::bulk_pull_server> (connection, std::move (req)));
auto block (request->get_next ());
ASSERT_NE (nullptr, block);
Expand Down Expand Up @@ -275,7 +263,6 @@ TEST (bulk_pull, count_limit)
req->start = receive1->hash ();
req->set_count_present (true);
req->count = 2;
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::bulk_pull_server> (connection, std::move (req)));

ASSERT_EQ (request->max_count, 2);
Expand Down Expand Up @@ -1645,7 +1632,6 @@ TEST (frontier_req_response, DISABLED_destruction)
req->start.clear ();
req->age = std::numeric_limits<decltype (req->age)>::max ();
req->count = std::numeric_limits<decltype (req->count)>::max ();
connection->requests.push (std::unique_ptr<nano::message>{});
hold = std::make_shared<nano::frontier_req_server> (connection, std::move (req));
}
}
Expand All @@ -1660,7 +1646,6 @@ TEST (frontier_req, begin)
req->start.clear ();
req->age = std::numeric_limits<decltype (req->age)>::max ();
req->count = std::numeric_limits<decltype (req->count)>::max ();
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::frontier_req_server> (connection, std::move (req)));
ASSERT_EQ (nano::dev::genesis_key.pub, request->current);
ASSERT_EQ (nano::dev::genesis->hash (), request->frontier);
Expand All @@ -1674,7 +1659,6 @@ TEST (frontier_req, end)
req->start = nano::dev::genesis_key.pub.number () + 1;
req->age = std::numeric_limits<decltype (req->age)>::max ();
req->count = std::numeric_limits<decltype (req->count)>::max ();
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::frontier_req_server> (connection, std::move (req)));
ASSERT_TRUE (request->current.is_zero ());
}
Expand Down Expand Up @@ -1716,7 +1700,6 @@ TEST (frontier_req, count)
req->start.clear ();
req->age = std::numeric_limits<decltype (req->age)>::max ();
req->count = 1;
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::frontier_req_server> (connection, std::move (req)));
ASSERT_EQ (nano::dev::genesis_key.pub, request->current);
ASSERT_EQ (send1->hash (), request->frontier);
Expand All @@ -1730,7 +1713,6 @@ TEST (frontier_req, time_bound)
req->start.clear ();
req->age = 1;
req->count = std::numeric_limits<decltype (req->count)>::max ();
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::frontier_req_server> (connection, std::move (req)));
ASSERT_EQ (nano::dev::genesis_key.pub, request->current);
// Wait 2 seconds until age of account will be > 1 seconds
Expand All @@ -1740,7 +1722,6 @@ TEST (frontier_req, time_bound)
req2->age = 1;
req2->count = std::numeric_limits<decltype (req2->count)>::max ();
auto connection2 (std::make_shared<nano::bootstrap_server> (std::make_shared<nano::socket> (*system.nodes[0], nano::socket::endpoint_type_t::server), system.nodes[0]));
connection2->requests.push (std::unique_ptr<nano::message>{});
auto request2 (std::make_shared<nano::frontier_req_server> (connection, std::move (req2)));
ASSERT_TRUE (request2->current.is_zero ());
}
Expand All @@ -1753,7 +1734,6 @@ TEST (frontier_req, time_cutoff)
req->start.clear ();
req->age = 3;
req->count = std::numeric_limits<decltype (req->count)>::max ();
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::frontier_req_server> (connection, std::move (req)));
ASSERT_EQ (nano::dev::genesis_key.pub, request->current);
ASSERT_EQ (nano::dev::genesis->hash (), request->frontier);
Expand All @@ -1764,7 +1744,6 @@ TEST (frontier_req, time_cutoff)
req2->age = 3;
req2->count = std::numeric_limits<decltype (req2->count)>::max ();
auto connection2 (std::make_shared<nano::bootstrap_server> (std::make_shared<nano::socket> (*system.nodes[0], nano::socket::endpoint_type_t::server), system.nodes[0]));
connection2->requests.push (std::unique_ptr<nano::message>{});
auto request2 (std::make_shared<nano::frontier_req_server> (connection, std::move (req2)));
ASSERT_TRUE (request2->frontier.is_zero ());
}
Expand Down Expand Up @@ -1844,7 +1823,6 @@ TEST (frontier_req, confirmed_frontier)
ASSERT_FALSE (req->header.frontier_req_is_only_confirmed_present ());
req->header.flag_set (nano::message_header::frontier_req_only_confirmed);
ASSERT_TRUE (req->header.frontier_req_is_only_confirmed_present ());
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::frontier_req_server> (connection, std::move (req)));
ASSERT_EQ (nano::dev::genesis_key.pub, request->current);
ASSERT_EQ (nano::dev::genesis->hash (), request->frontier);
Expand All @@ -1858,7 +1836,6 @@ TEST (frontier_req, confirmed_frontier)
ASSERT_FALSE (req2->header.frontier_req_is_only_confirmed_present ());
req2->header.flag_set (nano::message_header::frontier_req_only_confirmed);
ASSERT_TRUE (req2->header.frontier_req_is_only_confirmed_present ());
connection2->requests.push (std::unique_ptr<nano::message>{});
auto request2 (std::make_shared<nano::frontier_req_server> (connection2, std::move (req2)));
ASSERT_EQ (nano::dev::genesis_key.pub, request2->current);
ASSERT_EQ (nano::dev::genesis->hash (), request2->frontier);
Expand All @@ -1872,7 +1849,6 @@ TEST (frontier_req, confirmed_frontier)
ASSERT_FALSE (req3->header.frontier_req_is_only_confirmed_present ());
req3->header.flag_set (nano::message_header::frontier_req_only_confirmed);
ASSERT_TRUE (req3->header.frontier_req_is_only_confirmed_present ());
connection3->requests.push (std::unique_ptr<nano::message>{});
auto request3 (std::make_shared<nano::frontier_req_server> (connection3, std::move (req3)));
ASSERT_TRUE (request3->current.is_zero ());
ASSERT_TRUE (request3->frontier.is_zero ());
Expand All @@ -1884,7 +1860,6 @@ TEST (frontier_req, confirmed_frontier)
req4->age = std::numeric_limits<decltype (req4->age)>::max ();
req4->count = std::numeric_limits<decltype (req4->count)>::max ();
ASSERT_FALSE (req4->header.frontier_req_is_only_confirmed_present ());
connection4->requests.push (std::unique_ptr<nano::message>{});
auto request4 (std::make_shared<nano::frontier_req_server> (connection4, std::move (req4)));
ASSERT_EQ (key_before_genesis.pub, request4->current);
ASSERT_EQ (receive1->hash (), request4->frontier);
Expand All @@ -1896,7 +1871,6 @@ TEST (frontier_req, confirmed_frontier)
req5->age = std::numeric_limits<decltype (req5->age)>::max ();
req5->count = std::numeric_limits<decltype (req5->count)>::max ();
ASSERT_FALSE (req5->header.frontier_req_is_only_confirmed_present ());
connection5->requests.push (std::unique_ptr<nano::message>{});
auto request5 (std::make_shared<nano::frontier_req_server> (connection5, std::move (req5)));
ASSERT_EQ (key_after_genesis.pub, request5->current);
ASSERT_EQ (receive2->hash (), request5->frontier);
Expand All @@ -1912,7 +1886,6 @@ TEST (frontier_req, confirmed_frontier)
ASSERT_FALSE (req6->header.frontier_req_is_only_confirmed_present ());
req6->header.flag_set (nano::message_header::frontier_req_only_confirmed);
ASSERT_TRUE (req6->header.frontier_req_is_only_confirmed_present ());
connection6->requests.push (std::unique_ptr<nano::message>{});
auto request6 (std::make_shared<nano::frontier_req_server> (connection6, std::move (req6)));
ASSERT_EQ (key_before_genesis.pub, request6->current);
ASSERT_EQ (receive1->hash (), request6->frontier);
Expand All @@ -1928,7 +1901,6 @@ TEST (frontier_req, confirmed_frontier)
ASSERT_FALSE (req7->header.frontier_req_is_only_confirmed_present ());
req7->header.flag_set (nano::message_header::frontier_req_only_confirmed);
ASSERT_TRUE (req7->header.frontier_req_is_only_confirmed_present ());
connection7->requests.push (std::unique_ptr<nano::message>{});
auto request7 (std::make_shared<nano::frontier_req_server> (connection7, std::move (req7)));
ASSERT_EQ (key_after_genesis.pub, request7->current);
ASSERT_EQ (receive2->hash (), request7->frontier);
Expand Down Expand Up @@ -2105,7 +2077,6 @@ TEST (bulk_pull_account, basics)
req->account = key1.pub;
req->minimum_amount = 5;
req->flags = nano::bulk_pull_account_flags ();
connection->requests.push (std::unique_ptr<nano::message>{});
auto request (std::make_shared<nano::bulk_pull_account_server> (connection, std::move (req)));
ASSERT_FALSE (request->invalid_request);
ASSERT_FALSE (request->pending_include_address);
Expand Down
6 changes: 3 additions & 3 deletions nano/core_test/network.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,7 @@ TEST (node, port_mapping)
node0->port_mapping.start ();
auto end (std::chrono::steady_clock::now () + std::chrono::seconds (500));
(void)end;
//while (std::chrono::steady_clock::now () < end)
// while (std::chrono::steady_clock::now () < end)
{
ASSERT_NO_ERROR (system.poll ());
}
Expand Down Expand Up @@ -1382,7 +1382,7 @@ TEST (network, filter_invalid_network_bytes)
const_cast<nano::networks &> (keepalive.header.network) = nano::networks::invalid;
channel->send (keepalive);

ASSERT_TIMELY (5s, 1 == node1.stats.count (nano::stat::type::message, nano::stat::detail::invalid_network));
ASSERT_TIMELY (5s, 1 == node1.stats.count (nano::stat::type::error, nano::stat::detail::invalid_network));
}

// Ensure the network filters messages with the incorrect minimum version
Expand All @@ -1401,7 +1401,7 @@ TEST (network, filter_invalid_version_using)
const_cast<uint8_t &> (keepalive.header.version_using) = nano::dev::network_params.network.protocol_version_min - 1;
channel->send (keepalive);

ASSERT_TIMELY (5s, 1 == node1.stats.count (nano::stat::type::message, nano::stat::detail::outdated_version));
ASSERT_TIMELY (5s, 1 == node1.stats.count (nano::stat::type::error, nano::stat::detail::outdated_version));
}

TEST (network, fill_keepalive_self)
Expand Down
21 changes: 21 additions & 0 deletions nano/lib/stats.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,9 @@ std::string nano::stat::type_to_string (uint32_t key)
case nano::stat::type::bootstrap:
res = "bootstrap";
break;
case nano::stat::type::bootstrap_server:
res = "bootstrap_server";
break;
case nano::stat::type::error:
res = "error";
break;
Expand Down Expand Up @@ -645,12 +648,18 @@ std::string nano::stat::detail_to_string (stat::detail detail)
case nano::stat::detail::insufficient_work:
res = "insufficient_work";
break;
case nano::stat::detail::invalid:
res = "invalid";
break;
case nano::stat::detail::invocations:
res = "invocations";
break;
case nano::stat::detail::keepalive:
res = "keepalive";
break;
case nano::stat::detail::not_a_type:
res = "not_a_type";
break;
case nano::stat::detail::open:
res = "open";
break;
Expand Down Expand Up @@ -818,6 +827,18 @@ std::string nano::stat::detail_to_string (stat::detail detail)
case nano::stat::detail::invalid_telemetry_ack_message:
res = "invalid_telemetry_ack_message";
break;
case nano::stat::detail::invalid_bulk_pull_message:
res = "invalid_bulk_pull_message";
break;
case nano::stat::detail::invalid_bulk_pull_account_message:
res = "invalid_bulk_pull_account_message";
break;
case nano::stat::detail::invalid_frontier_req_message:
res = "invalid_frontier_req_message";
break;
case nano::stat::detail::message_too_big:
res = "message_too_big";
break;
case nano::stat::detail::outdated_version:
res = "outdated_version";
break;
Expand Down
7 changes: 7 additions & 0 deletions nano/lib/stats.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ class stat final
ledger,
rollback,
bootstrap,
bootstrap_server,
vote,
election,
http_callback,
Expand Down Expand Up @@ -275,6 +276,8 @@ class stat final
rollback_failed,

// message specific
not_a_type,
invalid,
keepalive,
publish,
republish_vote,
Expand Down Expand Up @@ -344,6 +347,10 @@ class stat final
invalid_node_id_handshake_message,
invalid_telemetry_req_message,
invalid_telemetry_ack_message,
invalid_bulk_pull_message,
invalid_bulk_pull_account_message,
invalid_frontier_req_message,
message_too_big,
outdated_version,
udp_max_per_ip,
udp_max_per_subnetwork,
Expand Down
2 changes: 2 additions & 0 deletions nano/node/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ add_library(
bootstrap/bootstrap_server.cpp
bootstrap/bootstrap.hpp
bootstrap/bootstrap.cpp
bootstrap/message_deserializer.hpp
bootstrap/message_deserializer.cpp
cli.hpp
cli.cpp
common.hpp
Expand Down
4 changes: 2 additions & 2 deletions nano/node/bootstrap/bootstrap_bulk_pull.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ void nano::bulk_pull_server::no_block_sent (boost::system::error_code const & ec
if (!ec)
{
debug_assert (size_a == 1);
connection->finish_request ();
connection->start ();
}
else
{
Expand Down Expand Up @@ -849,7 +849,7 @@ void nano::bulk_pull_account_server::complete (boost::system::error_code const &
}
}

connection->finish_request ();
connection->start ();
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion nano/node/bootstrap/bootstrap_bulk_push.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ void nano::bulk_push_server::received_type ()
}
case nano::block_type::not_a_block:
{
connection->finish_request ();
connection->start ();
break;
}
default:
Expand Down
2 changes: 1 addition & 1 deletion nano/node/bootstrap/bootstrap_frontier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ void nano::frontier_req_server::no_block_sent (boost::system::error_code const &
{
if (!ec)
{
connection->finish_request ();
connection->start ();
}
else
{
Expand Down
Loading