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

Periodically check for unconfirmed frontiers and start elections #1821

Merged
merged 22 commits into from Mar 21, 2019

Conversation

@SergiySW
Copy link
Collaborator

commented Mar 13, 2019

With max limit of elections to start: 250. More frequest checks for representative nodes

Check for unconfirmed frontiers after each bootstrap and start elections
With max limit of elections to start: 1k for regular nodes & 10k for voting nodes

@SergiySW SergiySW added this to the V19.0 milestone Mar 13, 2019

@SergiySW SergiySW self-assigned this Mar 13, 2019

@SergiySW SergiySW requested a review from clemahieu Mar 13, 2019

@SergiySW SergiySW added this to CP2 (2019-03-27) in V19 Mar 13, 2019

@zhyatt zhyatt requested a review from wezrule Mar 13, 2019

@guilhermelawless

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

How about having a "queued elections" buffer? For this PR, after the limit for active elections is reached, it could queue up the other elections to be started later.

@SergiySW

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 14, 2019

@guilhermelawless main reason for this limit here is to prevent large udp fraffic so nodes can passively wait for frontiers confirmation in usual circumstances

Show resolved Hide resolved nano/node/node.cpp Outdated
@guilhermelawless

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

@guilhermelawless main reason for this limit here is to prevent large udp fraffic so nodes can passively wait for frontiers confirmation in usual circumstances

Couldn't you say the same about all these frontiers that will get elections at once? It just seems strange to confirm some and not others.

@SergiySW

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 15, 2019

1k elections is not huge traffic. 600k is
But with new changes to start only after bootstrap processed blocks or at startup pending elections queue makes sense.

SergiySW added some commits Mar 15, 2019

@clemahieu
Copy link
Collaborator

left a comment

I think this would be better to connect with the active_transactions::request_confirm loop since it's a similar functionality of collecting confirmation.

We can just call active_elections::start and then we don't need a new pending_elections container.

I think the number of election slots we put to this should be much less than the maximum, probably 1/4 instead of exactly the maximum.

Since we're attaching this to request_confirm we can use that 15 second loop timer and check frontiers less frequently for non-rep nodes and also queue up a smaller number of elections.

@SergiySW

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 20, 2019

@clemahieu what delays between frontiers scans seems reasonable?

@SergiySW SergiySW changed the title Check for unconfirmed frontiers after each bootstrap and start elections Periodically check for unconfirmed frontiers and start elections Mar 20, 2019

@clemahieu
Copy link
Collaborator

left a comment

I like this placement a lot more. Just a couple thing to simplify logic and it should be good.

Show resolved Hide resolved nano/node/node.cpp Outdated
Show resolved Hide resolved nano/node/node.cpp Outdated
Show resolved Hide resolved nano/node/node.cpp Outdated
Show resolved Hide resolved nano/node/node.hpp Outdated
Show resolved Hide resolved nano/node/node.cpp Outdated
Show resolved Hide resolved nano/node/node.hpp Outdated
Show resolved Hide resolved nano/node/node.cpp Outdated

SergiySW added some commits Mar 21, 2019

@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented on nano/node/lmdb.cpp in 5dbf5a7 Mar 21, 2019

Good catch. We should add and explicit assert for this in the genesis initialization test.

This comment has been minimized.

Copy link
Collaborator Author

replied Mar 21, 2019

Added in 2 initialization tests

@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented on 5dbf5a7 Mar 21, 2019

Nice

SergiySW added some commits Mar 21, 2019

@SergiySW SergiySW merged commit 817cb25 into nanocurrency:master Mar 21, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

guilhermelawless added a commit to guilhermelawless/nano-node that referenced this pull request Apr 15, 2019

Periodically check for unconfirmed frontiers and start elections (nan…
…ocurrency#1821)

* Check for unconfirmed frontiers after each bootstrap and start elections

With max limit of elections to start: 1k for regular nodes & 10k for voting nodes

* Prevent fast confirm_req flood for frontiers

* Disable confirm_frontiers call if bootstrap didn't process block

* confirm_frontiers_first_call not required to be atomic

* pending_elections queue

* Fix start elections from pending

* Pending elections size

* Use active_transactions request_loop for frontiers check

* Use transaction reference

* Apply changes from review

* Remove unused variable

* Decrease check time for test network

* Improve test_network_factor

* Genesis block should be confirmed

* Update tests with confirmed genesis block

* Add confirmation height check to block_store.genesis (initialization test)

and in ledger.genesis_balance

* Fix rare failures of rpc.block_confirmed test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.