Clean up loadBlocksFromNetwork function from loader module - Closes #2292 & #2384 #2555
Conversation
When a timeout is triggered from loadBlocksFromPeer, loaded value changes to true incorrectly stopping the synch process without a proper reason. Besides the number of errors to stop the process is too little due to most of them are timeouts.
This metthod is currently used privately in Loader module. Instead of getting all the peers, we get a random one.
If the function is called with height 1 which means the genesis block, it has to call the callback without error returning block undefined.
It adds Try count to follow the failed attemps and decreases the limit to 5 again.
modules/blocks/process.js
Outdated
| @@ -261,6 +261,11 @@ __private.receiveForkFive = function(block, lastBlock, cb) { | |||
| Process.prototype.getCommonBlock = function(peer, height, cb) { | |||
| let comparisonFailed = false; | |||
|
|
|||
| // Allowing blocks to be loaded from genesis | |||
There was a problem hiding this comment.
Why we need this actually?
There was a problem hiding this comment.
I move it back to the former method.
modules/loader.js
Outdated
| @@ -846,86 +842,92 @@ __private.snapshotFinished = err => { | |||
| * @todo Add description for the params | |||
| */ | |||
| __private.loadBlocksFromNetwork = function(cb) { | |||
| let errorCount = 0; | |||
| // Number of failed attempts to load from peers. | |||
| let tries = 0; | |||
There was a problem hiding this comment.
Can we reset this when an attempt is successful? Also, the variable name can be more descriptive.
modules/loader.js
Outdated
| ); | ||
| errorCount += 1; | ||
| return next(); | ||
| if (!commonBlock && lastBlock.height != 1) { |
There was a problem hiding this comment.
When lastBlock.height is 1 we will not log anything, it this behavior correct?
modules/loader.js
Outdated
| }, | ||
| ], | ||
| err => { | ||
| if (err) { |
There was a problem hiding this comment.
This will never happen because we always call waterfallCb without error.
diego-G
force-pushed
the
2384-improve_performance_synchronisation_process
branch
from
7b75776
to
9defa79
Compare
There was a problem hiding this comment.
- Use function names in async steps, to have clear understanding.
- Use error object names to avoid error shadowing in the code
- Add proper explanation comments for better understanding
The actual issue #2292 describe the problem as:
The function adds a lot of obscurity in the understanding of the functionality, it jumps control flow in multiple ways and is missing useful comments.
I feel the function loadBlocksFromNetwork still have same problems mentioned as above. I suggest to ask author of the issue @SargeKhan to review it and see if his concerns are covered.
|
@nazarhussain I don't think we need more people reviewing the PR. I've added another commit to address the point 1. What I feel it is that we need a proper refactoring across the entire code to tackle these kind of concerns plus having any kind of prettifier/formatter. This will speed up our review process. |
|
The issue #2384 is covered by this PR only partially. I think we shouldn't close it yet. |
| @@ -1054,13 +1041,13 @@ Loader.prototype.findGoodPeers = function(peers) { | |||
|
|
|||
| // Public methods | |||
| /** | |||
| * Gets a list of "good" peers from network. | |||
There was a problem hiding this comment.
Please extend the comment and explain what does it mean to be a good peer.
There was a problem hiding this comment.
A good peer is what findGoodPeers() decides. I haven't modified that function. It performs a complex calculation that cannot be explained easily:
- It filters unreachable peers and those ones below the node's height itself.
- It orders peers by descending height
- It creates a histogram of peer's heights
- It removes outliers
I'd open another PR if we need to review that process.
MaciejBaj
deleted the
2384-improve_performance_synchronisation_process
branch
What was the problem?
The flow of the function was not easy to understand obscuring errors.
How did I fix it?
loader.getNetwork()byloader.getRandomPeerFromNetwork()881fba2async.waterfallinstead of callback 2 callbackHow to test it?
Review checklist