-
Notifications
You must be signed in to change notification settings - Fork 775
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
[Merged by Bors] - Update to tokio 1.1 #2172
Conversation
inner: Box::pin(stream), | ||
} | ||
} | ||
} |
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.
Notice that I have updated code after some PR review, perhaps you can update here as well? It might be a better implementation now. tokio-rs/tokio#3384
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.
Thanks for pointing this out! and thanks a lot for adding the feature to tokio, it's much appreciated!
Can you make a PR to discv5 with the latest updates you need and then we can make a release |
Got one here: sigp/discv5#58 |
Hey @realbigsean, Sorry for the late reply. I've made a new discv5 version: v0.1.0-beta.3 which should include the tokio 0.1 update. There was also an update to ENR I've been waiting to propagate through, so I've added it to this release. The updates the RLP and crypto libraries which has caused some small headaches. To integrate this version, you may need to update some crypto libraries and update ENR version in Lighthouse. Let me know if you get stuck anywhere. |
The crypto updates should be fine, but I'd recommend doing some tests on a testnet with these updates to make sure they can still discover peers etc as expected. |
@AgeManning I've been running this with the new discv5 release and haven't had any issues on pyrmont. I tried deleting the network dir to generate a new ENR and it seemed to also work fine. Let me know if you want me to test anything else. |
## Issue Addressed NA ## Proposed Changes Ignores a [hyper vuln](https://rustsec.org/advisories/RUSTSEC-2021-0020) that will be fixed in #2172. I am comfortable with ignoring this because we have a fix in the works and the impact of the vuln is low to negligible. ## Additional Info NA
…date-to-tokio-1.0
I did a bit of discv5 testing also. If she's running smooth, lgtm |
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.
Nice one @realbigsean, big effort!
This looks good to me. I'll test it on all the testnets in the process of cutting the next release :)
bors r+
## Issue Addressed resolves #2129 resolves #2099 addresses some of #1712 unblocks #2076 unblocks #2153 ## Proposed Changes - Updates all the dependencies mentioned in #2129, except for web3. They haven't merged their tokio 1.0 update because they are waiting on some dependencies of their own. Since we only use web3 in tests, I think updating it in a separate issue is fine. If they are able to merge soon though, I can update in this PR. - Updates `tokio_util` to 0.6.2 and `bytes` to 1.0.1. - We haven't made a discv5 release since merging tokio 1.0 updates so I'm using a commit rather than release atm. **Edit:** I think we should merge an update of `tokio_util` to 0.6.2 into discv5 before this release because it has panic fixes in `DelayQueue` --> PR in discv5: sigp/discv5#58 ## Additional Info tokio 1.0 changes that required some changes in lighthouse: - `interval.next().await.is_some()` -> `interval.tick().await` - `sleep` future is now `!Unpin` -> tokio-rs/tokio#3028 - `try_recv` has been temporarily removed from `mpsc` -> tokio-rs/tokio#3350 - stream features have moved to `tokio-stream` and `broadcast::Receiver::into_stream()` has been temporarily removed -> `tokio-rs/tokio#2870 - I've copied over the `BroadcastStream` wrapper from this PR, but can update to use `tokio-stream` once it's merged tokio-rs/tokio#3384 Co-authored-by: realbigsean <[email protected]>
Pull request successfully merged into unstable. Build succeeded: |
Issue Addressed
resolves #2129
resolves #2099
addresses some of #1712
unblocks #2076
unblocks #2153
Proposed Changes
Updates all the dependencies mentioned in Update to tokio v1.0 #2129, except for web3. They haven't merged their tokio 1.0 update because they are waiting on some dependencies of their own. Since we only use web3 in tests, I think updating it in a separate issue is fine. If they are able to merge soon though, I can update in this PR.
Updates
tokio_util
to 0.6.2 andbytes
to 1.0.1.We haven't made a discv5 release since merging tokio 1.0 updates so I'm using a commit rather than release atm. Edit: I think we should merge an update of
tokio_util
to 0.6.2 into discv5 before this release because it has panic fixes inDelayQueue
--> PR in discv5: update dependencies discv5#58Additional Info
tokio 1.0 changes that required some changes in lighthouse:
interval.next().await.is_some()
->interval.tick().await
sleep
future is now!Unpin
-> timer:sleep
future should become !Unpin tokio-rs/tokio#3028try_recv
has been temporarily removed frommpsc
-> Add back mpsc try_recv tokio-rs/tokio#3350tokio-stream
andbroadcast::Receiver::into_stream()
has been temporarily removed -> `stream: coordinating Tokio 1.0 with the availability of Stream in std tokio-rs/tokio#2870BroadcastStream
wrapper from this PR, but can update to usetokio-stream
once it's merged tokio-stream: add wrapper for broadcast and watch tokio-rs/tokio#3384