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

Make election behavior mutable to support dynamic behaviour transition #4799

Merged
merged 6 commits into from
Dec 4, 2024

Conversation

gr0vity-dev
Copy link
Contributor

Problem:

  • Elections are currently immutable and can't be transitioned to priority status
  • Causing delays for optimistic elections rescheduled by priority scheduler as the next Vote broadcast is fixed to 15-second intervals. (by network.vote_broadcast_interval)

Changes:

  • Modifies election behavior to be mutable, allowing elections (optimistic/hinted/...) to be transitioned to priority status
  • Added ability to trigger immediate vote broadcasts on transition without waiting for the next 15-second round

Benefits:

  • It would allow for more aggressive use of optimistic elections (to narrow the gap between checked and cemented during bootstrapping)
    • currently configured to accounts with gap_threshold of 32 could be reduced to 4
  • Maintain quick response times for live traffic by allowing election behavior changes.

@gr0vity-dev-bot
Copy link

gr0vity-dev-bot commented Nov 30, 2024

Test Results for Commit bcf8ac0

Pull Request 4799: Results
Overall Status:

Test Case Results

  • 5n4pr_conf_10k_bintree: PASS (Duration: 118s)
  • 5n4pr_conf_10k_change: PASS (Duration: 157s)
  • 5n4pr_conf_change_dependant: PASS (Duration: 136s)
  • 5n4pr_conf_change_independant: PASS (Duration: 140s)
  • 5n4pr_conf_send_dependant: PASS (Duration: 139s)
  • 5n4pr_conf_send_independant: PASS (Duration: 136s)
  • 5n4pr_rocks_10k_bintree: PASS (Duration: 122s)
  • 5n4pr_rocks_10k_change: PASS (Duration: 185s)

Last updated: 2024-12-03 16:41:13 UTC

@pwojcikdev
Copy link
Contributor

Can you include a test or two that ensures this works? So I imagine and election would be manually inserted via active.insert ( ..., election_behavior::optimistic ) and then upgraded to priority.

@gr0vity-dev
Copy link
Contributor Author

I added a simple testcase to validate the behavior transitioning works.

Ideally I'd like to also make sure the vote_broadcast_interval is reset and a rep votes again immediately after transitioning.
My problem is to create a network with one rep that doesn't have enough weight to confirm an election.
It seems :

  • we need to start from genesis and transfer some weight over to one rep
  • by creating a send + open block and confirm these.

If that approach is right I can create another testcase that makes sure voting happens as intended.

@gr0vity-dev gr0vity-dev force-pushed the prs/upgrade_elections branch from 42c81d5 to ac04d88 Compare December 1, 2024 14:17
@qwahzi qwahzi added this to the V28 milestone Dec 1, 2024
nano/node/election.cpp Outdated Show resolved Hide resolved
{
count_by_behavior[previous_behavior]--;
count_by_behavior[election_behavior_a]++;
result.election->transition_priority ();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this guaranteed to always succeed? This probably works but is unnecessary leaving space for subtle bugs. Should be something like:

bool transitioned = result.election->transition_priority ();
if (transitioned)
{
// success, update counts, stats, log
}
else
{
// failed for some reason, inc failed stats
}

@pwojcikdev
Copy link
Contributor

Regarding testing the voting behaviour, I don't think having two nodes is necessary. Each election should keep counters for number of vote broadcasts and confirmation requests and those can be queried directly. There is still a need to setup a rep key for node with weight higher than primary rep threshold but not enough to confirm blocks on its own, but that's rather simple.

@gr0vity-dev
Copy link
Contributor Author

Each election should keep counters for number of vote broadcasts and confirmation requests
My issue is

  • by default the testcase node is not voting and doesn't have a rep wallet in it.
  • So it doesn't broadcast a vote.
    if (!is_quorum.exchange (true) && node.config.enable_voting && node.wallets.reps ().voting > 0)

    I wanted to add a rep wallet, but
  • if I use the genesis key, blocks are confirmed by its single vote
  • So the optimistic election is not activated anymore...

@pwojcikdev
Copy link
Contributor

if I use the genesis key, blocks are confirmed by its single vote
So the optimistic election is not activated anymore...

You should be able to setup a representative with eg. 111K nano weight which will be generating votes but not be able to confirm elections. Am I missing something in this setup?

@gr0vity-dev
Copy link
Contributor Author

gr0vity-dev commented Dec 2, 2024

Yes exactly. My initial question was how to best do it. I didn't see see a helper method to set this up.
The best I found was :

  • Start from genesis and transfer some weight over to one rep by creating a send + open block and force confirm these blocks.

This requires quite a bit of code. If that's the right way, I'll go for it. Just wanted to make sure this is the right approach.

@pwojcikdev
Copy link
Contributor

There is nano::test::setup_rep ( ... ) if that's helpful.

@gr0vity-dev gr0vity-dev force-pushed the prs/upgrade_elections branch 2 times, most recently from 520f369 to ae5f991 Compare December 2, 2024 21:40
@gr0vity-dev gr0vity-dev force-pushed the prs/upgrade_elections branch from ae5f991 to d457891 Compare December 3, 2024 07:28
gr0vity added 2 commits December 3, 2024 15:30
Replace inline initializations with the new constructor syntax while maintaining the same behavior.
@pwojcikdev pwojcikdev merged commit 61e6a6b into nanocurrency:develop Dec 4, 2024
27 of 28 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 / V28.0
Development

Successfully merging this pull request may close these issues.

4 participants