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

feat: ipfs: remove kubo client backend #11661

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Feb 27, 2024

Related Issues

Proposed Changes

Remove the kubo client backend. This appears to be deprecated and unused? Basically, I'm trying to kill off https://github.com/filecoin-project/kubo-api-client so I don't have to update it.

Additional Info

This was used to run an internal IPFS node, for some kind of client deal thing.

Maybe we can also get rid of:

// TODO this should be removed.
func ClientBlockstore() dtypes.ClientBlockstore {
// in most cases this is now unused in normal operations -- however, it's important to preserve for the IPFS use case
return blockstore.WrapIDStore(blockstore.FromDatastore(datastore.NewMapDatastore()))
}

But... I'm not sure what to do about:

lsys := storeutil.LinkSystemForBlockstore(clientBs)

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@Stebalien Stebalien mentioned this pull request Feb 28, 2024
8 tasks
@Stebalien Stebalien requested review from rvagg and removed request for hannahhoward February 28, 2024 00:44
@Stebalien
Copy link
Member Author

@rvagg I'm also looking for insight into:

  • Whether or not it's safe to remove this code (some history, if you have it).
  • If we can maybe remove the client retrieval code entirely? Could be done in a new PR.

@rvagg
Copy link
Member

rvagg commented Feb 29, 2024

I don't have enough context but my understanding from a distance was that we are no longer recommending people use the retrieval code in here; you just need to look at the last updates to go-fil-markets to see how stale it all is. Boost forked it, trimmed it down to the bone, and have just removed it entirely from their stack. Lassie has been integrated in various places that do retrievals now and could be done here as Hannah said.

But, I just don't know the current state of our recommendations to users, regarding either side of the market, we still have code laying around to make deals and to perform retrievals. Do we have consistent messaging? We should just rip it all out, but we need good answers for people as a replacement.

I think we can also rip out graphsync while in there, afaik we don't use it for the chain and I think even the lotus-miner recommendations these days is to defer to separate markets software.

🤷 I suspect that @magik6k might have more context on this and could help? Otherwise I say just do it.

@hannahhoward
Copy link
Contributor

Seems like:

  1. You can remove the Kubo Blockstore to remove the annoying side repo immediately
  2. You SHOULD probably remove all the client retrieval software, which would allow you to remove a lot of other things (like the client blockstore and graphsync). But perhaps that needs slightly more planning and at least an FYI shared with the Lotus community?

I would try to just remove the client blockstore seperately.

@Stebalien Stebalien marked this pull request as ready for review March 1, 2024 01:40
@Stebalien Stebalien requested a review from a team as a code owner March 1, 2024 01:40
@rvagg
Copy link
Member

rvagg commented Mar 1, 2024

+1 to that, and we can stick lassie in there for CLI retrieval if people still want/need a client in here somewhere. Mostly we should just refer people to lassie itself I think.

@Stebalien
Copy link
Member Author

Stebalien commented Mar 5, 2024

Ok, so, this code isn't used for retrieval, it's used to make deals. Specifically, it means you can:

  1. ipfs add -r foo to get a $CID.
  2. lotus client deal $CID ....

As described in https://lotus.filecoin.io/tutorials/lotus/import-data-from-ipfs/. That actually seems like it might still be useful? But it also means that the current kubo dependency is overkill: all we need is the ability to read/write blocks. On the other hand... I'm now really annoyed that those repos got merged.

@magik6k
Copy link
Contributor

magik6k commented Apr 4, 2024

This dealmaking pathway is deprecated anyways, the new version of boost is dropping go-fil-markets entirely anyways, so it won't even be possible to make deals with SPs through this mechanism anymore.

Dropping deals-from-ipfs SGTM

@Stebalien Stebalien force-pushed the steb/remove-ipfs-blockstore branch 2 times, most recently from 8004e47 to e914398 Compare April 4, 2024 14:03
This was used to run an internal IPFS node...
@Stebalien Stebalien force-pushed the steb/remove-ipfs-blockstore branch from e914398 to 4d0b1f9 Compare April 4, 2024 14:05
@Stebalien Stebalien merged commit 7bd21bc into master Apr 4, 2024
186 checks passed
@Stebalien Stebalien deleted the steb/remove-ipfs-blockstore branch April 4, 2024 14:35
ribasushi pushed a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request May 11, 2024
This was used to run an internal IPFS node...
ribasushi pushed a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request May 16, 2024
This was used to run an internal IPFS node...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

4 participants