-
Notifications
You must be signed in to change notification settings - Fork 75
[ETCM-316] Fast-sync branch resolver #887
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
src/main/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolver.scala
Outdated
Show resolved
Hide resolved
src/main/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolver.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general idea looks good.
One thing i would change is to remove as much logic from actor, and move it to normal class to make it possible to test whole binary search logic without akka (and timeouts). So actor would be only to communicate with other actors
src/main/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolver.scala
Outdated
Show resolved
Hide resolved
src/main/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolver.scala
Outdated
Show resolved
Hide resolved
src/main/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolver.scala
Outdated
Show resolved
Hide resolved
e2618f6
to
427934a
Compare
src/main/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolver.scala
Outdated
Show resolved
Hide resolved
src/main/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolver.scala
Outdated
Show resolved
Hide resolved
d293268
to
4c7150e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good. The tests appear to cover all of the new functionality. I don't find anything blocking, and no additional comments from what other reviewers have already covered. There are a couple of instances of the val blocksSentFromPee
which I suspect should be named blocksSentFromPeer
. Good work!
Reviewed with ❤️ by PullRequest
src/test/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolverSpec.scala
Outdated
Show resolved
Hide resolved
src/test/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolverSpec.scala
Outdated
Show resolved
Hide resolved
e2ae8bf
to
e59e0a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/main/scala/io/iohk/ethereum/blockchain/sync/PeerListSupportNg.scala
Outdated
Show resolved
Hide resolved
src/test/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolverSpec.scala
Outdated
Show resolved
Hide resolved
src/test/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolverSpec.scala
Outdated
Show resolved
Hide resolved
src/test/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolverSpec.scala
Show resolved
Hide resolved
src/main/scala/io/iohk/ethereum/blockchain/sync/fast/FastSyncBranchResolverActor.scala
Outdated
Show resolved
Hide resolved
…892) * create actor FastSyncBranchResolver * ETCM-313 Download skeleton and then batch headers in parallel * Implement HeaderSkeleton class * Validate if skeleton header matches downloaded batch * Improve validations * Handle wrong skeleton from master peer * Fix incorrect example * Validate PoW of skeleton headers * Fix bugs found with tests * Add call to the branch resolver * Add missing config entries * Fix unit tests * Apply scalafmt * Cleanup tests * Fastsync: stick with the same master peer while requesting skeleton headers. * [ETCM-313] Integrate branch resolver actor with fast sync * [ETCM-313] Fix integration tests, format error messages * create actor FastSyncBranchResolver * create actor FastSyncBranchResolver * [ETCM-316] Fast-sync branch resolver (#887) * init new actor, FastSyncBranchResolver * added searching mode in fast sync branch resolver * fix style * add schedule when don't get peers * Added ut in class FastSyncBranchResolver * change messages * fix case object... * add new unit test * add new unit test * change name getFirstCommonBlock * change name to batch * init new actor, FastSyncBranchResolver * added searching mode in fast sync branch resolver * fix style * add schedule when don't get peers * Added ut in class FastSyncBranchResolver * change messages * fix case object... * add new unit test * add new unit test * create actor FastSyncBranchResolver * change name getFirstCommonBlock * change name to batch * Reformat triggered by sbt pp * Cleanup and simplify * Handle error cases * Fix tests * [ETCM-316] Add more tests and fix binary search logic * [ETCM-316] Finish tests for branch resolving * [ETCM-316] Cleanup * [ETCM-316] Small test improvements * [ETCM-316] Log binary search state * [ETCM-316] Move some logging to improve readability * [ETCM-316] Remove unneeded errors and reformat * [ETCM-316] Handle branch resolution failure * [ETCM-316] Address PR comments * [ETCM-316] Remove unnecessary string interpolation Co-authored-by: Petra Bierleutgeb <petra.bierleutgeb@iohk.io> * [ETCM-313] Reworked header skeleton (still needs refactoring) * [ETCM-313] Remove empty method * Fix SyncController tests, add more logging * Remove logging that broke integration tests (timeout) * [ETCM-313] More refactorings * [ETCM-313] Fix integration tests * [ETCM-313] Re-request header skeleton in case of errors * [ETCM-313] Remove skeleton handler name * [ETCM-313] Small fixes and better logs * [ETCM-313] Update the default number of requested block headers to not be higher than the default max number of headers returned * [ETCM-313] Adapt branchresolver recent blocks request. Co-authored-by: Maximiliano Biandratti <maximiliano.biandratti@iohk.io> Co-authored-by: Robin Raju <robinraju@users.noreply.github.com> Co-authored-by: Petra Bierleutgeb <petra.bierleutgeb@iohk.io> Co-authored-by: biandratti <72261652+biandratti@users.noreply.github.com> Co-authored-by: Petra Bierleutgeb <328036+pbvie@users.noreply.github.com>
Description
This task has as dependence the task [ETCM-313]. When a invalid block is detected from the master peer. We need to search for a new peer potencial and validate the our chain saved with this new peer.
In this way, with this new peer we need to follow the next points.
101
to101 - X
, where x is some hyperparameter. Then we check the block one by one if they make a chain connection with our local chain. i.e first check if remote 101 is child of local 100, if not then check if remote 100 is child of local 99. If we find a match, rewind to a matched block and that starts syncing from there, and if not go to point 2.Proposed Solution
We have an actor called FastSyncBranchResolver responsible for this behavior. When FastSync actor need to resolve it branch; FastSyncBranchResolver is called with the message “StartBranchResolver”. In this way, we have the following strategy:
Pending work
We need to remove binary search logic from the actor, and put it into another normal class. Making it possible to test it completely in all the cases.✔️[task: ETCM-531] We need to change the behavior of traits BlacklistSupport and PeerListSupport. Both should be actors. Today when we switched between FastSync and FastSyncBranchResolver, we lost the list of peers in memory from blackList and handshake. So we need to uncouple this behavior and don't lose the changes. This new behavior should also work in the future to regular sync.✔️Testing
It needs a battery of integration test to cover these case.