Skip to content
New issue

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

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

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Status: Merged / V27.0
Development

Successfully merging this pull request may close these issues.

3 participants