-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
update libp2p to v0.48 #661
Conversation
…l-core into leviathanbeak/update_libp2p
e15744e
to
78cd25e
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.
Looks good from what I understand. I think the mdns might be the wrong feature though
fuel-p2p/Cargo.toml
Outdated
libp2p = { version = "0.44", default-features = false, features = [ | ||
"dns-async-std", "gossipsub", "identify", "kad", "mdns", "mplex", "noise", | ||
libp2p = { version = "0.48", default-features = false, features = [ | ||
"dns-async-std", "gossipsub", "identify", "kad", "mdns-async-io", "mplex", "noise", |
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.
Should this be mdns-tokio
?
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.
Not necessarily, we currently use dns-async
, tcp-async
and mdn-async
so should be fine, we might want to move them to tokio but previously it wasn't as stable as the async ones.
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.
Won't this be running as part of the fuel-core binary though? Which does use tokio. The problem with using the wrong runtime is you end up pulling in the whole async-io runtime which really increases compile times.
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.
ah man, for some silly reason I didn't think about these implementations that they would include different runtimes, that is bad, not only on compile times but having 2 different async runtimes is inviting trouble - so yeah I'll move to tokio ones, thanks
This issue existed before the upgrade, but when I startup fuel-core with p2p enabled:
and then use
|
... |
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 don't fully understand this code but with that in mind it looks good.
dismissing since all has been marked as resolved by the reviewer
Closes #530 #644.
Also this introduces dependency on
protoc
for compiling Protobuf files, previouslyprost
library bundledprotoc
within the lib, but they've recently moved away from that approach.Changes
libp2p
has made some breaking changes in recent updates, main change was how you handle behaviour events - this means that the Behaviour, in our case FuelBehaviour, would be stateless apart from holding other inner behaviours and all the stateful logic would be moved to FuelP2PService that holds the FuelBehaviour.Usually this would just mean a lot of cut & paste (check the
service.rs
andbehaviour.rs
files), but in our case FuelBehaviour was generic over a NetworkCodec, and leaving it still generic would imply that our FuelBehaviourEvent enum would also need to be generic which made for some unwieldy code.I decided to create a wrapper struct
from this wrapper we would easily get the inner event and handle it in
FuelP2PService
.Also now
FuelP2PService
needed to become generic over NetworkCodec since it's now the one that handles encoding and decoding of the network messages.