-
Notifications
You must be signed in to change notification settings - Fork 75
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
[ETCM-531] Cache-based blacklist implementation [!pr] #921
Conversation
f9dfa56
to
dbde188
Compare
src/main/scala/io/iohk/ethereum/blockchain/sync/Blacklist.scala
Outdated
Show resolved
Hide resolved
import scala.concurrent.ExecutionContext | ||
import scala.concurrent.duration._ | ||
|
||
trait PeerListSupportNg { self: Actor with ActorLogging => |
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.
Mostly copied from PeerListSupport
with a little bit of cleanup. PeerListSupport
will go away eventually and then this will be renamed to PeerListSupport
.
src/main/scala/io/iohk/ethereum/blockchain/sync/Blacklist.scala
Outdated
Show resolved
Hide resolved
src/main/scala/io/iohk/ethereum/blockchain/sync/Blacklist.scala
Outdated
Show resolved
Hide resolved
src/main/scala/io/iohk/ethereum/blockchain/sync/Blacklist.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.
✅ This pull request was sent to the PullRequest network.
@pbvie you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.
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.
Looks like this should properly implement the cache-based blacklist 👍 It looks like other reviewers already did a thorough job, so I didn't have much to add other than some minor non-blocking comments.
Please let me know if you want me to look at something in more detail!
Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.
src/main/scala/io/iohk/ethereum/blockchain/sync/PeerListSupportNg.scala
Outdated
Show resolved
Hide resolved
src/main/scala/io/iohk/ethereum/blockchain/sync/PeerListSupportNg.scala
Outdated
Show resolved
Hide resolved
d8b5fe9
to
b8f1474
Compare
src/main/scala/io/iohk/ethereum/blockchain/sync/Blacklist.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.
This pull request would normally be cancelled because the title contains "[!pr]"; however since the review was requested manually from the PullRequest dashboard, it was sent to our network to be reviewed. If you wish to cancel PullRequest review, you can do so manually from the PullRequest dashboard.
Which asks permissions to "act on your behalf" and view all your email addresses when all I wanna do is unsubscribe... 👎 |
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.
This code looks quite good overall. I left a couple of inline comments on updatePeers
pointing out some additional considerations there. It also seems like it may be worthwhile to check the thread safety of this implementation. Aside from those, this code looks good to go.
Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.
src/test/scala/io/iohk/ethereum/blockchain/sync/CacheBasedBlacklistSpec.scala
Show resolved
Hide resolved
src/test/scala/io/iohk/ethereum/blockchain/sync/CacheBasedBlacklistSpec.scala
Outdated
Show resolved
Hide resolved
src/test/scala/io/iohk/ethereum/blockchain/sync/CacheBasedBlacklistSpec.scala
Outdated
Show resolved
Hide resolved
d537010
to
248907c
Compare
Description
The current implementation doesn't allow the blacklist to be shared between multiple actors (e.g. fast-sync and upcoming branch resolver). This PR adds a new and thread-safe implementation that can be access and modified by multiple actors.
Proposed Solution
Requirements of the blacklist include thread-safety, time-based expiration and size-based eviction of elements. Using a cache like Caffeine provides all the required features and introduces less complexity than implementing an actor-based DIY solution.
In the future, regular sync will also be moved to this new approach. The current/old blacklist and peerlist implementations will then be removed.
Important Changes Introduced
Testing