Update discovery and update time to get latest information - Closes #2073

[shuse2]

What was the problem?

Because peer information update only happens for random 100 peers, and only fetching the peer information every 30s, consensus calculation is most likely to be off.
It causes delegates not to be able to forge right away.

How did I fix it?

Revert the increased discovery time to 10s.

How to test it?

Observe update of peers happen every 10s.

Review checklist

• The PR solves Adjust the frequency of P2P discovery #2076
• All new code is covered with unit tests
• All new code was formatted with Prettier
• Linting passes
• Tests pass
• Commit messages follow the commit guidelines
• Documentation has been added/updated

[shuse2]

Since we have 10s window for forging, I think changing it to 5s is relevant.

[MaciejBaj]

10 sec discovery time will be compatible with the 0.9.* version.
https://github.com/LiskHQ/lisk/blob/master/modules/peers.js#L575

[MaciejBaj]

Please open the similar issue to #1943 which will refer to discovery time directly. We are obtaining to have the transparent flow of changes we've made during Betanet tests.

[MaciejBaj]

Please open the corresponding issue.

[4miners]

The process of updating information about peers, even with changes proposed in this PR - will still be less efficient than in 0.9.x. It's because in 0.9.x with every connection to other peer we also send our headers. In version 1.0.0 we need to make separate call to notify other peers about our headers peer.rpc.updateMyself, as described above we call that only for 100 peers. During peers discovery we're calling peer.rpc.status, which will perform one way (1.0.0) exchange of headers instead of two-way (0.9.x).

So proposed changes:

• are not consistent with 0.9.x (they cannot be consistent because the way how it works is different)
• will not fix issue described in What was the problem? section, it can mitigate it - slightly

[MaciejBaj]

@4miners yes, the process of updating information about peers works differently now. The current PR mitigates only the discovery time interval being too big as revealed during Alphanet test rounds - setting it back to 10 sec and reverting changes introduced by #1944.

[jondubois]

I think that this should fix the issue with peer headers being out of date (which causes the consensus to be miscalculated) but we still have the deeper issue that currently every node needs to get a status update from every other node in the network every x seconds; this behaviour gets more expensive as more nodes are added to the network.

Also, I'm not sure if updateMyself adds any value to our algorithm. What is the reason for having two different ways of sharing peer headers?

I think that this PR is not harmful and there is a good chance that it will solve our current problem but we should also solve the root issue at some point.

[shuse2]

Created #2076, and changed the closing issue.

As @4miners pointed, even if we change it back to 10s, it will not be the same result as 0.9.x, since protocol is totally different. Also, this is very inefficient and costly.

However, as @jondubois pointed out, I believe this has good chance to "fix" it. but we definitely need to think about the root cause.

Also,

updateMyself adds any value to our algorithm

I think I don't think we have good reason to keep it

[MaciejBaj]

I've opened the issue #2078 that describes the headers propagation problem.

The discovery process is still valid and I think it's important not to change its interval from 10 sec to 30 sec in 1.0.0 version without the solid argumentation.

[MaciejBaj]

Close until #2080 solved.