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

chore: update boxo for bitswap providing refactor #10270

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Dec 29, 2023

Keeps in sync with:

I also took time to cleanup how we handle offline dag service and offline blockservice (I added them to the fx 🪄✨) as now blockservice has the side effect of providing (instead of exchange (bitswap)).

@Jorropo Jorropo added the skip/changelog This change does NOT require a changelog entry label Dec 29, 2023
@Jorropo Jorropo requested review from lidel and a team as code owners December 29, 2023 01:54
@Jorropo
Copy link
Contributor Author

Jorropo commented Dec 29, 2023

There used to be a bug where files could take minutes if not hours to be accessible after ipfs add even when using accelerateddhtclient, this has been fixed because now we go through the reprovider which does batching.

@Jorropo
Copy link
Contributor Author

Jorropo commented Dec 29, 2023

After testing I think this also fixes #10011 because we don't keep CIDs providing queue in memory (as much), the queue is handled by badger or leveldb which writes to disk.

The providing also goes much faster.

Might not fix it completely, but it certainly help.

@Jorropo
Copy link
Contributor Author

Jorropo commented Dec 29, 2023

So after adding 15k blocks:

seq 15000 | parallel -j 1000 "bash -c 'echo {} | ipfs add --raw-leaves -Q'"

Before this patch: RSS is at 512MiB and climbing.
After this patch, RSS bounces between 225MiB and 300MiB, not climbing (the peak is actually reached right after starting the node)

I don't know if I fixed #10011 but whatever this is it's a great improvement.

@Jorropo Jorropo force-pushed the bitswap/improvements branch 2 times, most recently from 20dca9d to c19ef6f Compare December 29, 2023 08:03
@Jorropo Jorropo mentioned this pull request Dec 29, 2023
@Jorropo Jorropo force-pushed the bitswap/improvements branch 3 times, most recently from 22130d0 to 311cd6c Compare December 29, 2023 10:58
@Jorropo Jorropo mentioned this pull request Jan 4, 2024
@hacdias
Copy link
Member

hacdias commented Jan 8, 2024

@Jorropo I can take a better look once you have the Go tests and the sharness passing

@Jorropo
Copy link
Contributor Author

Jorropo commented Jan 8, 2024

@hacdias I didn't because I'm doing more things (rewriting the server) and wanted to handle all of this in kubo at once.
I don't want to bother fixing bugs I might remove the feature for idk.

@aschmahmann
Copy link
Contributor

There's a large set of interlinked PRs here. Trying to understand which are the independent sets of changes here and why we want/need them.

As I understand we have:

  1. We need to fix the nopfs breakage regarding sessions and generally make sessions easier to use correctly for added efficiency. That's Difficult to use one session per request (e.g. gateway request) boxo#567
  2. We should have the providing pipeline all go through the same batch provider to be able to leverage bulk providing when using the accelerated DHT client. While this could be fixed more indirectly, lifting the providing logic out of boxo/bitswap and at least into boxo/blockservice if not higher seems reasonable. That's the goal of Move providing responsabilities from bitswap to blockservice boxo#534
  3. There's a claim that the part of the ProviderQueryManager in boxo/bitswap which limits the number of findprovider queries in flight isn't helping or is hurting the end user experience (certainly magic limits not configurable by users seems bad). The goal of Remove provider query manager boxo#536 is to remove that limit

A couple other things that got hit along the way in the PR chain:

  1. Because the ContentRouting interface from go-libp2p includes providing a more limited interface should go into boxo/bitswap. Seems reasonable given a subset of the interface can be defined at the recipient per Go interface semantics.
  2. There's a DX claim that switching the internal connection semantics in boxo/bitswap to use peer.AddrInfo rather than peer.ID is better since it aligns with go-libp2p's host.Connect semantics rather than host.NewStream semantics and we're discovering addresses from provider records (and relying on go-libp2p-kad-dht to insert addresses into the peerstore feels weird). . IIUC this is not particularly important given the set of things that would need to change for this to matter, but it should operate as an internal implementation detail anyhow.

@Jorropo does this look about right to you?

@aschmahmann
Copy link
Contributor

If I've got this right it seems like the next steps should be:

  1. I'll take the lead on Difficult to use one session per request (e.g. gateway request) boxo#567
  2. @Jorropo should refactor Move providing responsabilities from bitswap to blockservice boxo#534 to not depend on other PRs (seems like it shouldn't be a problem but lmk if I'm wrong)
  3. @Jorropo should refactor Remove provider query manager boxo#536 to not depend on other PRs (IIUC this should be fine too even without moving the code around sessions, but lmk if I'm wrong)

@Jorropo Jorropo removed their assignment Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does NOT require a changelog entry
Projects
Status: 🔎 In Review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants