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.

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

Rep crawler overhaul #4449

Merged
merged 28 commits into from
Mar 8, 2024
Merged

Conversation

pwojcikdev
Copy link
Contributor

@pwojcikdev pwojcikdev commented Feb 25, 2024

Initial motivation for this PR was providing better control over rep crawler component lifetime and running its internal logic on a separate dedicated thread. In the process of working on it I made many smaller improvements that should make the logic clearer and less error prone.

Testing results provided by @gr0vity-dev show that performance is on par with current develop.
image

@pwojcikdev
Copy link
Contributor Author

I made a few adjustments based on recent network stress test. It should make repcrawler a bit better at finding representatives when vote requests are unreliable.

develop
Screenshot 2024-02-28 at 13 48 37
rep-crawler-overhaul
Screenshot 2024-02-28 at 13 48 30

@pwojcikdev
Copy link
Contributor Author

With the latest commit running on live network, rep crawler averages about ~6 requests per second with response rate of about 25%. It is able to keep a stable peered stake.
Screenshot 2024-03-05 at 14 23 09

nano/lib/config.hpp Show resolved Hide resolved
nano/lib/config.hpp Show resolved Hide resolved
nano/lib/config.hpp Show resolved Hide resolved
nano/node/repcrawler.hpp Show resolved Hide resolved
nano/node/repcrawler.cpp Show resolved Hide resolved
nano/node/repcrawler.cpp Show resolved Hide resolved
nano/node/repcrawler.cpp Show resolved Hide resolved
@dsiganos
Copy link
Contributor

dsiganos commented Mar 6, 2024

I am trying to understand how multiple reps inside one node would be dealt.
So I created this test case and it fails:

// Test that nodes can track PRs when multiple PRs are inside one node
TEST (rep_crawler, two_reps_one_node)
{
	nano::test::system system;
	auto & node1 = *system.add_node ();
	auto & node2 = *system.add_node ();

	// create a second PR account
	nano::keypair pr1 = nano::test::setup_rep (system, node1, node1.balance (nano::dev::genesis_key.pub) / 10);

	ASSERT_EQ (0, node2.rep_crawler.representative_count ());

	// enable the two PRs in node1
	system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv);
	system.wallet (0)->insert_adhoc (pr1.prv);

	ASSERT_TIMELY_EQ (5s, node2.rep_crawler.representative_count (), 2);
	auto reps = node2.rep_crawler.representatives ();
	ASSERT_EQ (2, reps.size ());
}

@pwojcikdev
Copy link
Contributor Author

@dsiganos Good catch, feel free to commit this testcase. I'll look into solving it.

@qwahzi qwahzi added this to the V27 milestone Mar 6, 2024
nano/node/repcrawler.cpp Outdated Show resolved Hide resolved
Do not delete the query when a confirm_ack is received with the right vote.
We might get more replies and we have no way of knowing how many will come.
Just let the query timeout.
# Conflicts:
#	nano/lib/stats_enums.hpp
#	nano/lib/thread_roles.cpp
#	nano/lib/thread_roles.hpp
@dsiganos dsiganos merged commit d5bf9a2 into nanocurrency:develop Mar 8, 2024
23 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged / V27.0
Development

Successfully merging this pull request may close these issues.

3 participants