Remove content routing from bitswap network#535
Closed
Jorropo wants to merge 1 commit intomove-providing-responsabilitiesfrom
Closed
Remove content routing from bitswap network#535Jorropo wants to merge 1 commit intomove-providing-responsabilitiesfrom
Jorropo wants to merge 1 commit intomove-providing-responsabilitiesfrom
Conversation
Codecov ReportAttention:
@@ Coverage Diff @@
## move-providing-responsabilities #535 +/- ##
==================================================================
Coverage ? 65.39%
==================================================================
Files ? 204
Lines ? 25386
Branches ? 0
==================================================================
Hits ? 16600
Misses ? 7305
Partials ? 1481
|
This was referenced Jan 3, 2024
Jorropo
force-pushed
the
remove-content-routing-from-bitswap-network
branch
from
2a2f368 to
1347809
Compare
Jorropo
added a commit
to ipfs/kubo
that referenced
this pull request
Jan 5, 2024
Keeps in sync with: - ipfs/boxo#534 - ipfs/boxo#535 - ipfs/boxo#536
Contributor
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Jorropo
force-pushed
the
move-providing-responsabilities
branch
from
29ff0cc to
94be73e
Compare
Jorropo
force-pushed
the
remove-content-routing-from-bitswap-network
branch
from
1347809 to
28f6549
Compare
hacdias
reviewed
Jan 12, 2024
| * 🛠 `pinning/pinner`: you can now give a custom name when pinning a CID. To reflect this, the `Pinner` has been adjusted. | ||
| - `blockservice` now have a `WithProvider` option, this allows to recreate the behavior of advertising added blocks the bitswap server used to do. | ||
| - `bitswap` & `bitswap/client` now have a `WithContentSearch` option, this pickup the content routing job from `bitswap/network`. | ||
| It used to be a commun pattern for consumers which do not need external content routing to pass a [`routinghelpers.Null`](https://pkg.go.dev/github.com/libp2p/go-libp2p-routing-helpers#Null), now this can be ommited completely which is more efficient. |
Member
There was a problem hiding this comment.
Suggested change
| It used to be a commun pattern for consumers which do not need external content routing to pass a [`routinghelpers.Null`](https://pkg.go.dev/github.com/libp2p/go-libp2p-routing-helpers#Null), now this can be ommited completely which is more efficient. | |
| It used to be a common pattern for consumers which do not need external content routing to pass a [`routinghelpers.Null`](https://pkg.go.dev/github.com/libp2p/go-libp2p-routing-helpers#Null), now this can be omitted completely which is more efficient. |
hacdias
reviewed
Jan 12, 2024
Comment on lines
+26
to
+29
| - 🛠 `bitswap/network` no longer manages content routing, related Methods and function Arguments have been removed. | ||
| - `Network.ConnectTo` method has been changed from [`peer.ID`](https://pkg.go.dev/github.com/libp2p/go-libp2p/core/peer#ID) to [`peer.AddrInfo`](https://pkg.go.dev/github.com/libp2p/go-libp2p/core/peer#AddrInfo), given adding addresses hints used to be a side effect of the network. Theses now need to be passed in as values. |
Member
There was a problem hiding this comment.
Suggested change
| - 🛠 `bitswap/network` no longer manages content routing, related Methods and function Arguments have been removed. | |
| - `Network.ConnectTo` method has been changed from [`peer.ID`](https://pkg.go.dev/github.com/libp2p/go-libp2p/core/peer#ID) to [`peer.AddrInfo`](https://pkg.go.dev/github.com/libp2p/go-libp2p/core/peer#AddrInfo), given adding addresses hints used to be a side effect of the network. Theses now need to be passed in as values. | |
| - 🛠 `bitswap/network` no longer manages content routing, related methods and function arguments have been removed. | |
| - `Network.ConnectTo` method has been changed from [`peer.ID`](https://pkg.go.dev/github.com/libp2p/go-libp2p/core/peer#ID) to [`peer.AddrInfo`](https://pkg.go.dev/github.com/libp2p/go-libp2p/core/peer#AddrInfo), given adding addresses hints used to be a side effect of the network. Theses now need to be passed in as values. |
hacdias
reviewed
Jan 12, 2024
hacdias
approved these changes
Jan 12, 2024
Member
hacdias
left a comment
hacdias
left a comment
There was a problem hiding this comment.
Considering the title, this seems to do what it says. Just left a comment in line with a question.
Jorropo
force-pushed
the
move-providing-responsabilities
branch
from
94be73e to
ac0f6e0
Compare
Jorropo
force-pushed
the
remove-content-routing-from-bitswap-network
branch
from
28f6549 to
301d5b5
Compare
This was referenced Jan 16, 2024
Merged
Closed
Jorropo
force-pushed
the
remove-content-routing-from-bitswap-network
branch
from
301d5b5 to
d414841
Compare
Jorropo
force-pushed
the
move-providing-responsabilities
branch
from
ac0f6e0 to
e0d5341
Compare
Jorropo
force-pushed
the
remove-content-routing-from-bitswap-network
branch
from
d414841 to
5eb2dbc
Compare
Jorropo
force-pushed
the
remove-content-routing-from-bitswap-network
branch
from
5eb2dbc to
0cd80af
Compare
Jorropo
changed the base branch from
move-providing-responsabilities
to
main
Jorropo
force-pushed
the
remove-content-routing-from-bitswap-network
branch
from
0cd80af to
6740401
Compare
… to an option of the client Given that the previous commit remove the content advertising from the server, it did not made sense to share these paths on the network. The code has been reworked: - addresses aren't magically added to the peerstore as a side-effect of calling `Network.FindProvidersAsync`. Instead they are passed as hints to ConnectTo which copies libp2p `host.ConnectTo` API. - the providerquerymanager is completely shutdown when not using `WithContentSearch` option, this helps usecase where `routinghelpers.Null` is used for content routing and the consumer exclusively rely on broadcast, like networks where most peoples have all the content (Filecoin, Celestia, ...).
Jorropo
changed the base branch from
main
to
move-providing-responsabilities
Jorropo
force-pushed
the
remove-content-routing-from-bitswap-network
branch
from
6740401 to
2dbd42a
Compare
Jorropo
added a commit
to ipfs/kubo
that referenced
this pull request
Feb 16, 2024
Keeps in sync with: - ipfs/boxo#578 - ipfs/boxo#534 - ipfs/boxo#535 - ipfs/boxo#536
Member
|
Closing this as the work was completed through a different path. The goals here (removing content routing from bitswap network) were achieved via:
These changes shipped in boxo v0.25.0+ and v0.34.0+ respectively. Thanks @Jorropo for the initial work that informed the final design. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Based on #534