-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Rework p2p req resp protocol #11781
base: develop
Are you sure you want to change the base?
Rework p2p req resp protocol #11781
Conversation
It's not Rust but 🤷
… while we're sending a payload
(It doesn't work).
The contracts-bedrock-tests failure is unrelated I think... This is ready for review. |
Thanks @tynes, addressed your nits, and answered the TODO question so it's not lingering forever as an unknown. |
@@ -4,6 +4,11 @@ go 1.21 | |||
|
|||
require ( | |||
github.com/BurntSushi/toml v1.4.0 | |||
github.com/anacrolix/chansync v0.6.0 |
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.
How important is it to use these packages? We need to be really sure when adding new packages to be certain there aren't backdoors
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.
I definitely need chansync or I'd just end up duplicating all that again.
Some of the generic stuff is exceptionally useful for avoiding a lot of verbosity in Go.
The rest I was using for debugging.
@@ -223,6 +192,7 @@ type SyncPeerScorer interface { | |||
// If the user does sync a long range of blocks through this mechanism, | |||
// it does end up traversing through the chain, but receives the blocks in reverse order. | |||
// It is up to the user to persist the blocks for later processing, or drop & resync them if persistence is limited. | |||
// TODO: Should this be renamed to AltSyncClient? |
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.
Semgrep found 1 Potential |
Primarily addresses #11779.
Reworks p2p alt sync to:
TODO/open questions/future PRs (not blocking unless someone flags them):