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.

Sign up for GitHub

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

Jump to bottom

Remove content routing from bitswap network #535

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

Remove content routing from bitswap network #535

wants to merge 1 commit into 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
@codecov Codecov
Copy link

codecov bot commented Jan 3, 2024 •
edited

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

This was referenced Jan 3, 2024
Open
Closed
@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
@Jorropo
chore: update boxo for bitswap providing refactor
732d56c 
Keeps in sync with:
- ipfs/boxo#534
- ipfs/boxo#535
- ipfs/boxo#536
@Jorropo Jorropo mentioned this pull request Jan 5, 2024
Open
@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 •
edited

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.

Join @Jorropo and the rest of your teammates on Graphite Graphite

@Jorropo Jorropo mentioned this pull request Jan 11, 2024
Open
@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
hacdias
hacdias reviewed Jan 12, 2024
View reviewed changes
CHANGELOG.md
@@ -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.

hacdias
hacdias reviewed Jan 12, 2024
View reviewed changes
CHANGELOG.md
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.

hacdias
hacdias approved these changes Jan 12, 2024
View reviewed changes
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 move-providing-responsabilities branch from 94be73e to ac0f6e0 Compare January 16, 2024 09:05
@Jorropo Jorropo force-pushed the remove-content-routing-from-bitswap-network branch from 28f6549 to 301d5b5 Compare January 16, 2024 09:05
This was referenced Jan 16, 2024
Merged
Merged
Merged
Closed
Closed
Closed
Merged
Closed
Open
Open
@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 mentioned this pull request Feb 14, 2024
Open
@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
@Jorropo
bitswap/network,bitswap/client: move content routing responsabilities…
2dbd42a 
… 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
@Jorropo
chore: update boxo for bitswap providing refactor
dae471e 
Keeps in sync with:
- ipfs/boxo#578
- ipfs/boxo#534
- ipfs/boxo#535
- ipfs/boxo#536
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hacdias hacdias hacdias approved these changes

@lidel lidel Awaiting requested review from lidel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Jorropo @hacdias