masternode|net|rpc: Improve masternode sync process by xdustinface · Pull Request #3690 · dashpay/dash

xdustinface · 2020-09-07T23:53:55Z

This PR fixes some inconsistency issues in the masternode sync process e.g. if connections dropped or network has been disabled. It also combines the two states

MASTERNODE_SYNC_INITIAL
MASTERNODE_SYNC_WAITING

into one: MASTERNODE_SYNC_BLOCKCHAIN.

Just didn't see a reason to stick with the initial state as it seems simpler to me with only having states representing what actually happens at the time. If that has some deeper idea i didn't see yet, let me know.

7d9ab2199869e177ce111ecac71160bb8303eee2 decreases the timeout (30s to 6s) between the transition MASTERNODE_SYNC_BLOCKCHAIN -> MASTERNODE_SYNC_GOVERNANCE if the client has more than three connected peers. Idea is that i guess we can assume, if one has more than 3 connections and there was no update for 6 seconds chances are very low that there is more to come, no? And if there is more to come the process will either get a reset or will just catch up properly later. Will still be 30s for less than three tried peers.

bcafa734f078cf2ca86dd7394f99560d60bc34bc makes sure the UI gets an update if the masterrnode sync receives a reset. This is a preparation for a Qt-PR i have to come next. If someone feels this should rather be moved to the Qt-PR then, please cry. For me it felt better to put it in here.

Rest should be obvious, see commits.

UdjinM6 · 2020-09-08T10:03:34Z

Need rebase

xdustinface · 2020-09-09T03:07:39Z

Need rebase

Done, and tests should pass too now! 🤞

I would say its enough to only wait 1 tick if we have more than 3 peers before we move over to governance sync.

Without this it takes one iteration more for the UI to receive the update.

Give it MASTERNODE_SYNC_RESET_SECONDS (600) seconds time after the last UpdateBlockTip call.

This will reset the sync process if its outdated in the following cases: - If the connections dropped to zero - If the connections went from zero to one - If the network has been enabled or disabled

UdjinM6

Makes sense overall, few suggestions

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

UdjinM6

ACK

Hmmm... feature_llmq_simplepose.py is failing even locally now, need to investigate a bit

In general it doesn't make sense to connect to masternodes before due to MNAUTH requires blockchain sync. This could lead to failing quorum connections/failing masternode probing.. if a just restarted node/a out of sync node would hit a dkg block.. Then they would not try to open those llmq/probing connections for the next 60s (nLLMQConnectionRetryTimeout). Thats basically what happens in tests right now and they fail without this commit.

xdustinface · 2020-09-10T03:16:20Z

Yeah i was on the failing test for a while now...see commit message of f3baf15 which is the fix. Actually it feels like it should be a separate PR as its kind of unrelated but i added it here now. If someone still wants to have it separated let me know.

xdustinface · 2020-09-10T04:14:54Z

Damn i really felt it would work without 0cdd64e now.... but looks like it doesn't :D

UdjinM6 · 2020-09-10T04:42:20Z

I don't think mine_quorum is the right place for the fix, should be more like UdjinM6@b6435920ef imo

Their sync might be out of date otherwise due to bigger mocktime bumps

xdustinface · 2020-09-10T11:45:32Z

I think i got the right place to force the sync now in de4318c

UdjinM6

re-ACK

PastaPastaPasta

utACK

* masternode: Replace sync states INITIAL and WAITING with BLOCKCHAIN * masternode: Peer dependent "assume tip" timeout I would say its enough to only wait 1 tick if we have more than 3 peers before we move over to governance sync. * masternode: Notify the UI instantly if switched to governance sync Without this it takes one iteration more for the UI to receive the update. * masternode: Notify the UI about CMasternodeSync::Reset calls * masternode: Don't instantly reset the sync process Give it MASTERNODE_SYNC_RESET_SECONDS (600) seconds time after the last UpdateBlockTip call. * rpc: Don't switch to next asset in "mnsync reset" * rpc: Force the reset in "mnsync reset" * net: Make sure the sync gets a reset if required after network changes This will reset the sync process if its outdated in the following cases: - If the connections dropped to zero - If the connections went from zero to one - If the network has been enabled or disabled * Apply suggestions from code review Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> * net: Only open masternode connections if the blockchain is synced In general it doesn't make sense to connect to masternodes before due to MNAUTH requires blockchain sync. This could lead to failing quorum connections/failing masternode probing.. if a just restarted node/a out of sync node would hit a dkg block.. Then they would not try to open those llmq/probing connections for the next 60s (nLLMQConnectionRetryTimeout). Thats basically what happens in tests right now and they fail without this commit. * test: Make sure nodes are synced when they get restored after isolation Their sync might be out of date otherwise due to bigger mocktime bumps Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

xdustinface force-pushed the pr-masternode-improve-sync branch 2 times, most recently from 68f01ba to 41288bd Compare September 8, 2020 01:41

masternode: Replace sync states INITIAL and WAITING with BLOCKCHAIN

5c683cc

xdustinface force-pushed the pr-masternode-improve-sync branch from 41288bd to 86ffb1b Compare September 9, 2020 03:03

xdustinface changed the title ~~masternode|net: Improve masternode sync process~~ masternode|net|rpc: Improve masternode sync process Sep 9, 2020

xdustinface added 7 commits September 9, 2020 22:58

masternode: Peer dependent "assume tip" timeout

f63242b

I would say its enough to only wait 1 tick if we have more than 3 peers before we move over to governance sync.

masternode: Notify the UI instantly if switched to governance sync

063467c

Without this it takes one iteration more for the UI to receive the update.

masternode: Notify the UI about CMasternodeSync::Reset calls

24f0b23

masternode: Don't instantly reset the sync process

814fa08

Give it MASTERNODE_SYNC_RESET_SECONDS (600) seconds time after the last UpdateBlockTip call.

rpc: Don't switch to next asset in "mnsync reset"

003de34

rpc: Force the reset in "mnsync reset"

3363e8a

net: Make sure the sync gets a reset if required after network changes

9f5d280

This will reset the sync process if its outdated in the following cases: - If the connections dropped to zero - If the connections went from zero to one - If the network has been enabled or disabled

xdustinface force-pushed the pr-masternode-improve-sync branch from 7d116fe to 9f5d280 Compare September 9, 2020 21:08

UdjinM6 reviewed Sep 10, 2020

View reviewed changes

Comment thread src/masternode/masternode-sync.cpp Outdated

Comment thread src/masternode/masternode-sync.cpp Outdated

Comment thread src/net.cpp Outdated

Apply suggestions from code review

362b6d3

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

UdjinM6 previously approved these changes Sep 10, 2020

View reviewed changes

test: Make sure nodes are synced when they get restored after isolation

de4318c

Their sync might be out of date otherwise due to bigger mocktime bumps

xdustinface force-pushed the pr-masternode-improve-sync branch from 0cdd64e to de4318c Compare September 10, 2020 11:44

UdjinM6 approved these changes Sep 10, 2020

View reviewed changes

UdjinM6 added this to the 16 milestone Sep 10, 2020

UdjinM6 added the backport-candidate-16.x label Sep 10, 2020

xdustinface mentioned this pull request Sep 11, 2020

qt: Make sure the statusbar reflects internal states correct #3698

Merged

PastaPastaPasta approved these changes Sep 11, 2020

View reviewed changes

UdjinM6 merged commit 5176a26 into dashpay:develop Sep 11, 2020

UdjinM6 removed the backport-candidate-16.x label Sep 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

masternode|net|rpc: Improve masternode sync process#3690

masternode|net|rpc: Improve masternode sync process#3690
UdjinM6 merged 11 commits into
dashpay:developfrom
xdustinface:pr-masternode-improve-sync

xdustinface commented Sep 7, 2020 •

edited

Loading

Uh oh!

UdjinM6 commented Sep 8, 2020

Uh oh!

xdustinface commented Sep 9, 2020

Uh oh!

UdjinM6 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

UdjinM6 left a comment

Uh oh!

xdustinface commented Sep 10, 2020

Uh oh!

xdustinface commented Sep 10, 2020

Uh oh!

UdjinM6 commented Sep 10, 2020 •

edited

Loading

Uh oh!

xdustinface commented Sep 10, 2020

Uh oh!

UdjinM6 left a comment

Uh oh!

PastaPastaPasta left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

xdustinface commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 commented Sep 8, 2020

Uh oh!

xdustinface commented Sep 9, 2020

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

xdustinface commented Sep 10, 2020

Uh oh!

xdustinface commented Sep 10, 2020

Uh oh!

UdjinM6 commented Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xdustinface commented Sep 10, 2020

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xdustinface commented Sep 7, 2020 •

edited

Loading

UdjinM6 commented Sep 10, 2020 •

edited

Loading