Skip to content

Faster opening of masternode connections#3375

Merged
codablock merged 1 commit into
dashpay:developfrom
codablock:pr_faster_mnopen
Mar 26, 2020
Merged

Faster opening of masternode connections#3375
codablock merged 1 commit into
dashpay:developfrom
codablock:pr_faster_mnopen

Conversation

@codablock

Copy link
Copy Markdown

Only sleep 100ms when we previously tried to connect a MN. The back-off
logic in ThreadOpenMasternodeConnections will prevent too many unsuccessful
connects to offline/bad nodes.

Only sleep 100ms when we previously tried to connect a MN. The back-off
logic in ThreadOpenMasternodeConnections will prevent too many unsuccessful
connects to offline/bad nodes.
@codablock codablock added this to the 16 milestone Mar 24, 2020

@nmarley nmarley left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

utACK

@PastaPastaPasta PastaPastaPasta left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

a thought and a nit, otherwise utACK

Comment thread src/net.cpp
int sleepTime = 1000;
if (didConnect) {
sleepTime = 100;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could be, sleepTime = didConnect ? 100 : 1000;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Nah, I'm not a fan of the ternary operator. It's horrible to read and understand.

Comment thread src/net.cpp
if (didConnect) {
sleepTime = 100;
}
if (!interruptNet.sleep_for(std::chrono::milliseconds(sleepTime)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same line or brackets

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not in the scope of this PR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not really out of scope since you changed 1000 to sleepTime, but this is non-blocking.

@UdjinM6

UdjinM6 commented Mar 26, 2020

Copy link
Copy Markdown

I'm not sure I understand the reason (and the logic) behind this change... Why do we want smaller sleepTime for mn connections? If there is a reason, why don't we just reduce sleepTime instead, why if..else if it only going to work once (1000 before the first connection is made and 100 afterwards) (EDIT: I got this part wrong, sorry 🙈 )?

@codablock

Copy link
Copy Markdown
Author

@UdjinM6 We need a smaller sleep time because of #3380, which needs to open connections ASAP. With 1000ms, this can easily take longer then block generation and thus risks a failing DKG. Regarding the if..else, I was unsure if doing all the mutex locking so often wouldn't degrade performance in other places, so I decided to play safe. Now it will only run the loop faster when there is actually work to do.

@UdjinM6 UdjinM6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

utACK

@codablock
codablock merged commit b6bdb8b into dashpay:develop Mar 26, 2020
@codablock
codablock deleted the pr_faster_mnopen branch March 26, 2020 12:24
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Feb 20, 2022
Only sleep 100ms when we previously tried to connect a MN. The back-off
logic in ThreadOpenMasternodeConnections will prevent too many unsuccessful
connects to offline/bad nodes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants