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

merge: 2.6 #3542

Merged
merged 274 commits into from
Feb 28, 2020
Merged

merge: 2.6 #3542

merged 274 commits into from
Feb 28, 2020

Conversation

air1one
Copy link
Contributor

@air1one air1one commented Feb 27, 2020

Summary

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

dated and others added 30 commits October 25, 2019 15:33
* chore(core-p2p): rename syncWithNetwork() to downloadBlocksFromHeight()

The new name better describes what the method does.

* fix(core-p2p): improve parallel block download

* Disallow gaps in the returned array of blocks.

* Cancel running jobs if some job fails.

* Do not discard downloaded blocks, lower than the ones supposed to be
  downloaded by a failed job.

* Download 10 chunks in parallel instead of 25 - it should be enough to
  saturate the connection and still not cause socketcluster timeouts.

* chore: print more details in log messages

* Make sure we dont try the same peer for different chunks at 1st

* Rename syncWithNetwork()->downloadBlocksFromHeight() also in __tests__/

* Adjust test

* Add a comment about AsyncFunction vs Promise

* Fix off-by-one error in printout

* Rewrite downloadBlocksFromHeight() tests

* Cache the downloaded chunks that were not returned to caller

* Remove redundant method PeerCommunicator.downloadBlocks()

Also improve error message printouts

* Remove unused import
* fix: dont rollback while accepted blocks have not been written to database yet

The `AcceptBlockHandler` already alters `blocksInCurrentRound` while blocks
aren't written to database until the current batch finished processing.

In the case of a fork, we'd stop while blocks have been accepted, but not
written to database yet. This led to inconsistencies and resulted
in an assert being triggered which then wrecked the state machine
effectively stopping the node.

```
AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
  assert_1.default(this.blocksInCurrentRound.pop().data.id === block.data.id)
    at DatabaseService.revertBlock (/home/ark/.config/yarn/global/node_modules/@arkecosystem/core-database/dist/database-service.js:312:25)
    at async revertLastBlock (/home/ark/.config/yarn/global/node_modules/@arkecosystem/core-blockchain/dist/blockchain.js:236:13)
    at async __removeBlocks (/home/ark/.config/yarn/global/node_modules/@arkecosystem/core-blockchain/dist/blockchain.js:260:13)
    at async __removeBlocks (/home/ark/.config/yarn/global/node_modules/@arkecosystem/core-blockchain/dist/blockchain.js:261:13)
    at async Blockchain.removeBlocks (/home/ark/.config/yarn/global/node_modules/@arkecosystem/core-blockchain/dist/blockchain.js:270:9)
    at async startForkRecovery (/home/ark/.config/yarn/global/node_modules/@arkecosystem/core-blockchain/dist/state-machine.js:204:9)
```

This PR ensures that a fork only starts after all accepted blocks are
in the database. Tested manually, before I was able to reproduce it every
time, now I can no longer reproduce it.
@faustbrian faustbrian marked this pull request as ready for review February 27, 2020 15:17
@codecov
Copy link

codecov bot commented Feb 28, 2020

Codecov Report

Merging #3542 into 3.0 will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              3.0    #3542   +/-   ##
=======================================
  Coverage   47.52%   47.52%           
=======================================
  Files         548      548           
  Lines       13861    13861           
  Branches     1902     1902           
=======================================
  Hits         6587     6587           
  Misses       7245     7245           
  Partials       29       29           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c71cc33...c71cc33. Read the comment docs.

@faustbrian faustbrian merged commit c71cc33 into 3.0 Feb 28, 2020
@ghost ghost deleted the merge/3.0-2.6 branch February 28, 2020 08:17
@ghost ghost removed the Status: Needs Review label Feb 28, 2020
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.