Skip to content
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

Remove content routing from bitswap network #535

Open
wants to merge 1 commit into
base: move-providing-responsabilities
Choose a base branch
from

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Jan 3, 2024

Based on #534

@Jorropo Jorropo requested a review from a team as a code owner January 3, 2024 18:17
Copy link

codecov bot commented Jan 3, 2024

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (move-providing-responsabilities@4060154). Click here to learn what that means.

❗ Current head 5eb2dbc differs from pull request most recent head 2dbd42a. Consider uploading reports for the commit 2dbd42a to get more accurate results

Impacted file tree graph

@@                        Coverage Diff                         @@
##             move-providing-responsabilities     #535   +/-   ##
==================================================================
  Coverage                                   ?   65.39%           
==================================================================
  Files                                      ?      204           
  Lines                                      ?    25386           
  Branches                                   ?        0           
==================================================================
  Hits                                       ?    16600           
  Misses                                     ?     7305           
  Partials                                   ?     1481           
Files Coverage Δ
bitswap/bitswap.go 69.51% <ø> (ø)
bitswap/client/client.go 89.79% <100.00%> (ø)
...tswap/client/internal/messagequeue/messagequeue.go 83.53% <ø> (ø)
bitswap/client/internal/session/session.go 90.80% <100.00%> (ø)
bitswap/network/ipfs_impl.go 78.86% <100.00%> (ø)
bitswap/options.go 44.44% <100.00%> (ø)
bitswap/server/server.go 58.50% <100.00%> (ø)
bitswap/testnet/peernet.go 38.46% <100.00%> (ø)
blockservice/test/mock.go 100.00% <100.00%> (ø)
examples/unixfs-file-cid/main.go 41.21% <100.00%> (ø)
... and 5 more

@Jorropo Jorropo force-pushed the remove-content-routing-from-bitswap-network branch from 2a2f368 to 1347809 Compare January 5, 2024 15:56
Jorropo added a commit to ipfs/kubo that referenced this pull request Jan 5, 2024
@hacdias hacdias changed the base branch from main to move-providing-responsabilities January 8, 2024 12:59
@Jorropo Jorropo changed the base branch from move-providing-responsabilities to main January 11, 2024 17:20
@Jorropo Jorropo changed the base branch from main to move-providing-responsabilities January 11, 2024 18:07
@Jorropo
Copy link
Contributor Author

Jorropo commented Jan 11, 2024

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.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Jorropo Jorropo force-pushed the move-providing-responsabilities branch from 29ff0cc to 94be73e Compare January 11, 2024 18:10
@Jorropo Jorropo force-pushed the remove-content-routing-from-bitswap-network branch from 1347809 to 28f6549 Compare January 11, 2024 18:11
@@ -18,9 +18,14 @@ The following emojis are used to highlight certain changes:

* 🛠 `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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the title, this seems to do what it says. Just left a comment in line with a question.

@Jorropo Jorropo force-pushed the remove-content-routing-from-bitswap-network branch from 301d5b5 to d414841 Compare January 16, 2024 09:19
@Jorropo Jorropo force-pushed the move-providing-responsabilities branch from ac0f6e0 to e0d5341 Compare February 14, 2024 18:40
@Jorropo Jorropo requested a review from lidel as a code owner February 14, 2024 18:40
@Jorropo Jorropo force-pushed the remove-content-routing-from-bitswap-network branch from d414841 to 5eb2dbc Compare February 14, 2024 18:40
@Jorropo Jorropo force-pushed the remove-content-routing-from-bitswap-network branch from 5eb2dbc to 0cd80af Compare February 16, 2024 13:47
@Jorropo Jorropo changed the base branch from move-providing-responsabilities to main February 16, 2024 13:48
@Jorropo Jorropo force-pushed the remove-content-routing-from-bitswap-network branch from 0cd80af to 6740401 Compare February 16, 2024 13:48
… 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 Jorropo changed the base branch from main to move-providing-responsabilities February 16, 2024 14:09
@Jorropo Jorropo force-pushed the remove-content-routing-from-bitswap-network branch from 6740401 to 2dbd42a Compare February 16, 2024 14:09
Jorropo added a commit to ipfs/kubo that referenced this pull request Feb 16, 2024
@@ -38,7 +37,7 @@ func TestFetchIPLDPrimeNode(t *testing.T) {
})
}))

net := tn.VirtualNetwork(mockrouting.NewServer(), delay.Fixed(0*time.Millisecond))
net := tn.VirtualNetwork(delay.Fixed(0 * time.Millisecond))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define a const for delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants