-
Notifications
You must be signed in to change notification settings - Fork 39
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
IPC-520: Upgrade the IPLD Resolver for libp2p 0.53 #522
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fridrik01
approved these changes
Jan 11, 2024
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.
Happy to see this land, we should however create issues for these TODOs so we can track them.
{ | ||
pub fn new(ttl: Duration) -> Self { | ||
Self { | ||
cache: TimeCache::new(ttl), | ||
cache: LruCache::with_expiry_duration(ttl), |
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 was referenced Jan 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #520
Replaces #1
Depends on consensus-shipyard/libp2p-bitswap#1
The PR updates the
libp2p
dependency of theipld-resolver
crate to 0.53. It doesn't update thelibipld
dependency to 0.15 so that thecid
version doesn't change, and thus thefvm*
libraries don't need to be updated.There are many breaking changes in libp2p, some of which this PR did not solve:
libp2p::request_response::handler::Event
private, which broke the way rate limiting is done by the Resolver on top oflibp2p_bitswap
; I used to hijack the connection handler event going to the Bitswap behaviour, and just not propagate it if was a request and the client hit the limit. This was also the only way for me to count how many bytes have been served. Now that this is impossible, our only option is to move rate limiting into the library itself. In any case I asked for advice.Swarm::ban_peer
is gone, they introduced a new behaviour instead which one needs to add to their bouquet of behaviours. This new behaviour will return an error fromhandle_pending_outgoing_request
if the peer is banned. I haven't done this.SwarmBuilder
is gone and with it a lot of the connection limit configuration doesn't have a home. I left them commented out, but for safety reasons we should figure out where to set them.Test failures
Unfortunately the IPLD Resolver Bitswap test timed out:
RUST_LOG=debug cargo test -p ipc_ipld_resolver --test smoke resolve -- --nocapture
With some extra logging I can see that the bitswap requests and responses are coming, but the progress never reaches completion. I noticed that it works if I lower the number of entries in the HAMT to 100 or 500, but fails with 1000. The original repo works with 5000 keys, so I have to assume that the
libp2p-bitswap
PR we are building against introduced some kind of regression. We can investigate as a separate issue.