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

fix: only traverse relevant part of HAMT #448

Closed
wants to merge 4 commits into from

Conversation

aschmahmann
Copy link

Description

Switched the UnixFS resolver code to do resolve only the relevant part of a HAMT rather than potentially enumerating all of it.

Fixes: ipfs/service-worker-gateway#18

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@aschmahmann aschmahmann requested a review from a team as a code owner February 23, 2024 17:55
Comment on lines 72 to 76
if (result.unixfs?.type === 'hamt-sharded-directory') {
// special case - unixfs v1 hamt shards
dirCid = await findShardCid(result.node, part, blockstore)
} else {
dirCid = findLinkCid(result.node, part)
Copy link
Author

@aschmahmann aschmahmann Feb 23, 2024

Choose a reason for hiding this comment

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

This code is straight-up copy-pasted from https://github.com/ipfs/js-ipfs-unixfs/blob/4749d9a7c1eddd86b8fc42c3fa47f88c7b1b75ae/packages/ipfs-unixfs-exporter/src/resolvers/unixfs-v1/index.ts#L59 which makes me wonder why we can't just use walkPath from ipfs-unixfs-exporter directly and pass a path

Copy link
Member

Choose a reason for hiding this comment

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

I think using ipfs-unixfs-exporter instead of copy-pasting the code is a better idea.

@@ -1,12 +1,14 @@
import { logger } from '@libp2p/logger'
import { exporter } from 'ipfs-unixfs-exporter'
import findShardCid from 'ipfs-unixfs-exporter/dist/src/utils/find-cid-in-shard.js'
Copy link
Author

Choose a reason for hiding this comment

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

This isn't right. We could copy-paste or export the code if needed, but wonder if we could use more of the resolver code from ipfs-unixfs-exporter as-is

Copy link
Member

@achingbrain achingbrain Feb 26, 2024

Choose a reason for hiding this comment

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

FWIW you can't do deep imports of files like this any more, it's prevented by the exports map in the module's package.json which defines what's importable from a module and what isn't.

SgtPooki added a commit that referenced this pull request Feb 23, 2024
@achingbrain
Copy link
Member

I have opened a PR that uses the unixfs-exporter to traverse the DAG here - #455

It should also solve the issue reported in ipfs/service-worker-gateway#18

@SgtPooki
Copy link
Member

Closing in favor of #455

@SgtPooki SgtPooki closed this Feb 27, 2024
@SgtPooki SgtPooki deleted the fix/hamt-overqueried branch February 27, 2024 17:08
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.

Enumerating HAMT directory retrieves too many blocks?
3 participants