boxo icon indicating copy to clipboard operation
boxo copied to clipboard
verified publisher icon ipfs

provider: duplicated CIDs sent to provide queue

Open gammazero opened this issue 1 year ago • 10 comments

part of https://github.com/ipshipyard/roadmaps/issues/6

The provide queue is getting many duplicate CIDs. It turns out that normal ipfs add and mfs are both adding blocks to the blockservice, and causing duplicated CIDs to be provided.

This may be tolerable since the CIDs are deduplicated after being read from the queue, here, but it does mean the queue has to hold double the CIDs that are actually provided.

It cases where things are added in such a way that follows both call paths, it may be worth passing some parameter that suppresses providing in one of the paths.

Call stack from normal ipfs-add path:

github.com/ipfs/boxo/provider.(*reprovider).Provide(0x14001585080?, {0x10535d940?, 0x14001589e90?}, {{0x14000635ef0?, 0x1?}}, 0x1?)
        github.com/ipfs/[email protected]/provider/reprovider.go:464 +0x2c
github.com/ipfs/boxo/exchange/providing.(*Exchange).NotifyNewBlocks(0x14000d4e6a0, {0x10535d940, 0x14001589e90}, {0x140015ba5a0, 0x1, 0x1?})
        github.com/ipfs/[email protected]/exchange/providing/providing.go:41 +0xbc
github.com/ipfs/boxo/blockservice.(*blockService).AddBlocks(0x1400158cc40, {0x10535d978, 0x14000b67b80}, {0x140015ba5a0, 0x1, 0x1})
        github.com/ipfs/[email protected]/blockservice/blockservice.go:223 +0x3ac
github.com/ipfs/boxo/ipld/merkledag.(*dagService).AddMany(0x14000b3f5a8, {0x10535d978, 0x14000b67b80}, {0x140015ba580, 0x1, 0x10327cdd4?})
        github.com/ipfs/[email protected]/ipld/merkledag/merkledag.go:71 +0xe4
github.com/ipfs/go-ipld-format.(*Batch).asyncCommit.func1(...)
        github.com/ipfs/[email protected]/batch.go:100
created by github.com/ipfs/go-ipld-format.(*Batch).asyncCommit in goroutine 380
        github.com/ipfs/[email protected]/batch.go:98 +0x194
(edited)

Call stack from mfs path:

github.com/ipfs/boxo/provider.(*reprovider).Provide(0x140014ce480?, {0x106c3d940?, 0x14001685860?}, {{0x140016a0930?, 0x1?}}, 0x1?)
        github.com/ipfs/[email protected]/provider/reprovider.go:464 +0x2c
github.com/ipfs/boxo/exchange/providing.(*Exchange).NotifyNewBlocks(0x14000f77ca0, {0x106c3d940, 0x14001685860}, {0x140016a62c0, 0x1, 0x1?})
        github.com/ipfs/[email protected]/exchange/providing/providing.go:41 +0xbc
github.com/ipfs/boxo/blockservice.(*blockService).AddBlock(0x14001409140, {0x106c3d978, 0x14001421c20}, {0x14fcc9740, 0x140014ce480})
        github.com/ipfs/[email protected]/blockservice/blockservice.go:177 +0x250
github.com/ipfs-shipyard/nopfs/ipfs.(*BlockService).AddBlock(0x14001424b70, {0x106c3d978, 0x14001421c20}, {0x14fcc9740, 0x140014ce480})
        github.com/ipfs-shipyard/nopfs/[email protected]/blockservice.go:78 +0x1a8
github.com/ipfs/boxo/ipld/merkledag.(*dagService).Add(0x14001424ba0?, {0x106c3d978?, 0x14001421c20?}, {0x106c53a10?, 0x140014ce480?})
        github.com/ipfs/[email protected]/ipld/merkledag/merkledag.go:63 +0x94
github.com/ipfs/boxo/mfs.(*Directory).getNode(0x140014ce4e0, 0xa8?)
        github.com/ipfs/[email protected]/mfs/dir.go:404 +0x110
github.com/ipfs/boxo/mfs.(*Directory).GetNode(...)
        github.com/ipfs/[email protected]/mfs/dir.go:387
github.com/ipfs/boxo/mfs.(*Root).Close(0x140014d8c50)
        github.com/ipfs/[email protected]/mfs/root.go:197 +0x34
github.com/ipfs/kubo/core/node.Files.func2({0x10675f880?, 0x106c27180?})
        github.com/ipfs/kubo/core/node/core.go:208 +0x20

gammazero avatar Mar 27 '25 16:03 gammazero

Providing is happening 3X

  • Provides from adding the file to a buffered DAG service
  • Provides from patching the file into a temp MFS tree
  • Provides from Pinning the MFS tree

In the case of folders they are provided 3x too, also the wrapping folder that is discarded when not wrapping. But instead of the buffered provide, the last provide is when closing the MFS tree.

gammazero avatar Apr 02 '25 19:04 gammazero

Ok, so we have multiple issues the way things work right now:

  • The exchange interface must implement a NotifyNewBlocks
  • We have a "providing" exchange which plugs the reprovider to anything that uses the exchange. Particulary the blockservice calls this on AddBlock() and when getting new blocks.
  • The Blockservice is the underlying layer for DAGService which are used for adding...

This means we enqueue to announce:

  • Any block that is fetched for whatever reason.
  • Any block that is added via DagService
  • Any block that is part of MFS
  • Any MFS directory that is flushed
  • Any block that is part of temporary MFS DAGs when adding

When adding particularly, the root CID, and the temp-MFS root CID are submitted several times to the DAG service. If the DAG has folders, they are also submitted several times due to cacheSync operations on MFS (for example to print added directories while adding, which requires reading nodes, which in turns syncs the cache to disk). The issue is not so bad when adding big files (internal blocks are written only once).

All of this is regardless on whether our providing strategy is "all" or "roots" or (soon) "mfs"... so we will be all blocks added even if not intended.

Potential approach

Seeing that most of the issue when adding comes from the temp-MFS, I believe

  • we should create the temporary-MFS DAGs on an offline blockservice that does not to provide, immediately. Once we have finalized adding, there is an outputDirs function which recursively visits the temp-MFS tree to print-out directories (leaving out raw-leaves). We can take the chance that we are reading the temp-MFS tree directories to provide them as we go only once.

  • Since we know whether the added content will be pinned or put on MFS, we can additionally use an offline dagservice when adding content that is not going to be pinned when our strategy is "pinned" or "roots" etc. As a way to avoid providing blocks unnecessarily and take into account the configured strategy.

hsanjuan avatar Apr 02 '25 22:04 hsanjuan

@gammazero do you think this can be closed based on latest improvements where we respect providing strategy? It may be that the reprovider still receives duplicates, for example if something is a pin and also is in mfs. I don't have a solution for that. But I think we may have resolve some of the duplicates though, i.e. when adding it should only be provided from one place (i think).

hsanjuan avatar Sep 12 '25 08:09 hsanjuan

I suspect there is a bug in the boxo/provide code, which duplicates keys that are provided.

In a recent benchmark, I monitored the provide process with the boxo/provider/reprovider, and found that the system sends out 40 ADD_PROVIDER RPCs for each multihash. The system is expected to allocate the provider records to the DHT 20 closest nodes only.

The benchmark didn't use kubo at all, so there was no provide strategy involved, just multihashes/CIDs passed to the provider. It provides support that each CID seems to be provided (=20 ADD_PROVIDER) twice.

Image

Also, we added a deduplication filter to the provider queue in https://github.com/ipfs/boxo/pull/910, which is expected to reduce the duplicate CIDs caused by the kubo provide strategy.

guillaumemichel avatar Sep 12 '25 21:09 guillaumemichel

@guillaumemichel Triage question for Kubo 0.39: will this effectively be ok to close once we switch to Provide.DHT.SweepEnabled=true by default, or is there additional work that needs to happen here?

lidel avatar Oct 01 '25 14:10 lidel

The problem hasn't been addressed. We still don't know the root cause of this issue.

The new provide system that will be the default in kubo v0.39 will either avoid or hide the issue when providing to the DHT.

However, the problem may still occur when using the legacy provider to provide to an HTTP endpoint.

We can either decrease priority of the issue or close as "won't fix" after v0.39.

guillaumemichel avatar Oct 02 '25 07:10 guillaumemichel

Ack. I think we will just close this in PR that switches the default to the new system + add warning in docs that SweepEnabled=false may cause duplicate announcements. No point in fixing old system that we want to get rid of anyway.

lidel avatar Oct 02 '25 18:10 lidel

Wouldn't this be causing duplicated ProvideNow() calls even if the effects are hidden? I will have a look see if I can locate where it comes from...

hsanjuan avatar Oct 07 '25 14:10 hsanjuan

Triage note:

  • once sweep is enabled by default in 0.39, we will double-check and decide if ok to close it (and leave old as-is).

lidel avatar Oct 14 '25 13:10 lidel

Looking into this. When adding:

  • With Sweep Enabled (I think irrelevant)
  • Adding with normal params a file of size <256KiB (single block)

There are 7 calls to provide via the blockstore:

  1. The actual block, comes from go-ipld-format committing an async batch of blocks. This happens when splitting the file into blocks in https://github.com/ipfs/kubo/blob/f9dc7399334df37fcda50b60b608c419b5ebc412/core/coreunix/add.go#L159
  2. The second is unixfs empty cid QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn. I don't know if it's ignored at the providing layer or somewhere. It comes from mfs.NewEmptyDirectory(). This happens as a temporary MFS root is created to hold the Merkledag of the files that are being added.
  3. The actual block again, this time after mfs.AddChild(), since the file needs to be added to the temporary MFS.
  4. and 5. The CID of a wrapping folder, which must be the root of the temporary MFS that we just added the file onto, due to Flush(). The first one due to the dagService.Add() when getting the wrapping folder Node, the second due to dagService.Add() of the MFS root node itself (which happens to be the same).
  5. The MFS root AGAIN, due to the mfs.Close() operation after we have finished adding.
  6. The actual block (3rd time), part of the "PinRoot" step when finishing adding, which also does a dagService.Add().

While this seems repetitive I am not sure that we can declare any of these calls superfluous:

  • MFS adds any block it works with, it does not necessarily know if they were written before.
  • Perhaps it is superflous to call dagService.Add() on the root CID that will be pinned, because we can assume that MFS added it already. But in this case the PinRoot function seems fairly independent from the dag-building stuff that uses MFS, and it is exported (probably shouldn't)...
  • Perhaps we could make mfs.Close() skip the dagService.Add() but this happens via GetNode(), so there might be unintended consequences if someone the MFS DAG has not been flushed before.
  • Empty unixfs CID should be ignored if it isn't.
  • The blockstore has a WriteThrough feature, which is enabled by default (IIRC), and which means that it doesn't check if a file exists before writing it, and writes all of them. This is because reading before writing can be worse than just writing.
    • Even when WriteThrough disabled, we provide all the blocks, not just those that are written, so that could be fixed but it would mean no immediate [re]Provide() for a block that is re-added (i.e. a root CID of something), which may also break stuff that just seemingly worked before thanks to that Reprovide().

Assuming a larger file or a more complex directory structure, we will not have so many duplicates as there will be intermediary folders/nodes that will not be roots at the same time. File-chunks will not added twice either, as only the root of the file and not its chunks is added to MFS, so not every block is provided twice on larger files (I verified this).

Conclusion: I think the problem is mostly related to small, single-block files so impact would be mostly felt when adding single-block things.

hsanjuan avatar Oct 20 '25 13:10 hsanjuan