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

Temporary pins when adding looks buggy #4559

Open
Stebalien opened this issue Jan 8, 2018 · 8 comments
Open

Temporary pins when adding looks buggy #4559

Stebalien opened this issue Jan 8, 2018 · 8 comments
Labels
exp/expert Having worked on the specific codebase is important kind/bug A bug in existing code (including security flaws)

Comments

@Stebalien
Copy link
Member

First, we don't appear to actually mark them as temporary. The following code appears dead in practice:

https://github.com/ipfs/go-ipfs/blob/1a0d6ec2ba1e114293cbb0a7d2394d7b48a554c2/core/coreunix/add.go#L198-L204

Second, this is not crash safe. We need a way to add in-memory pins.

@schomatis
Copy link
Contributor

@Stebalien Is someone working on this? How hard do you think it would be to implement this? Is it apt for a beginner to IPFS?

@Stebalien
Copy link
Member Author

@schomatis probably not a good beginner project. It requires making deep modifications to the pin system.

@Stebalien Stebalien added kind/bug A bug in existing code (including security flaws) exp/expert Having worked on the specific codebase is important labels Feb 27, 2018
@schomatis
Copy link
Contributor

@Stebalien Thanks for the heads up. I have a couple of issues in line regarding locks, maybe after that I can give this a try.

@schomatis
Copy link
Contributor

@Stebalien Is this still a relevant issue to work on? I'm a bit confused, you mentioned it in the addAllAndPin issue, but I'm also seeing other discussions that mention that the current pin system may be deprecated in the near future (#4763, #4757, #4675).

Could you point me in the right direction of what should I be reading to address this?

@Voker57
Copy link
Contributor

Voker57 commented Mar 8, 2018

I think temporary in-memory pins can implemented in non-invasive way by storing them in Pinner https://github.com/ipfs/go-ipfs/blob/master/pin/pin.go#L183 and adding temporary pins to ColoredSet here: https://github.com/ipfs/go-ipfs/blob/master/pin/gc/gc.go#L171.

That would not be interfering with #4757 proposal.

@Stebalien
Copy link
Member Author

@schomatis we're discussing various ways to fix the pin system but haven't decided on any yet. In the foreseeable future, the pinning system will probably stay as it is. @Voker57's right on this, we can probably add them to the pinner. I assumed this would require a deep modification but that may not be the case.

@schomatis
Copy link
Contributor

@Stebalien I understand. Could it be useful to prepare a PoC PR to start exploring @Voker57 's option? Or should I wait for the proposal to mature some more?

@Ericson2314
Copy link

I happened on this while thinking about the best way to implement our https://github.com/NixOS/nix/blob/965b80347e97169f266466603e29a57359c4083c/src/libstore/store-api.hh#L490 --- never-mind the messary particular our Store interface there, but it might make sense to not just have temporary pins for IPFS itself, but also for the user.

My thoughts are fuzzy, but I'm imagining something like mutable reference associated with a temporary pin set used with a transaction like interface. In fact, if there were transactions for updating a bunch of IPNS at once, I suppose the pin set reference would be the analogue to a temporary, to keep a consistent IPNS<->table / SQL transactions in particular analogy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/expert Having worked on the specific codebase is important kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

No branches or pull requests

4 participants