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

[WIP] Implement ipfs name follow #4462

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

[WIP] Implement ipfs name follow #4462

wants to merge 21 commits into from

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented Dec 7, 2017

Implements ipfs follow in a minimally non intrusive way; #4435

TODO:

  • UX improvements (per @lgierth)
  • Implement persistence for follows (per @magik6k)
  • tests

@ghost ghost assigned vyzo Dec 7, 2017
@ghost ghost added the status/in-progress In progress label Dec 7, 2017
@vyzo vyzo added the topic/ipns Topic ipns label Dec 7, 2017
OK bool
}

var IpnsFollowCmd = &cmds.Command{
Copy link
Member

Choose a reason for hiding this comment

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

This should really go to core/commands/name.go or core/commands/name/follow.go (along with the other ipns related commands). Imo core/commands should only contain the top-level commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

duly noted, but there are some dependencies on common helpers in the package; plus i would rather leave command refactoring out of this PR.


res.SetOutput(&ipnsFollowResult{true})
},
Arguments: []cmdkit.Argument{
Copy link
Member

@magik6k magik6k Dec 7, 2017

Choose a reason for hiding this comment

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

Most commands order these like that: Helptext/Arguments/Options/Run/Type/Marshalers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will reorder.

)

const (
followInterval = 60 * time.Minute
Copy link
Member

Choose a reason for hiding this comment

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

We might want to have this in config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

Copy link
Member

Choose a reason for hiding this comment

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

We may also want to make this a flag ipfs follow add --refresh-interval X.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The configurable flag is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added --refresh-interval parameter to add.

defer nc.mx.Unlock()

if _, ok := nc.follows[name]; ok {
return
Copy link
Member

Choose a reason for hiding this comment

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

Might want to return an error here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

defer nc.mx.Unlock()

cancel, ok := nc.follows[name]
if ok {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, If user somehow makes a type there would be no indication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

Choose a reason for hiding this comment

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

I would invert this check. if !ok { return err }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, prettier this way

return follows
}

func (nc *nameCache) followName(ctx context.Context, name string, pinit bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Having this as a single worker resolving one name each followInterval/len(nc.follows) will ensure no activity spikes when many names are followed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can coalesce in a single goroutine, more resource economical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure there is significant benefit by having a single goroutine, plus it's a lot more complicated to write if we want to ensure immediate initial resolution upon follow.
The pins will be serialized by the pin lock.

How strongly do you feel about using a single goroutine?

Copy link
Member

Choose a reason for hiding this comment

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

It was more of a suggestion, though it's still something we may want to do in the feature

name = "/ipns/" + name
}

p, err := nc.nsys.Resolve(ctx, name)
Copy link
Member

@magik6k magik6k Dec 7, 2017

Choose a reason for hiding this comment

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

Is there a built-in timeout in nsys.Resolve? If not we might want to wrap the ctx with some.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not, i'll add a timeout here.

return
}

err = nc.pinning.Pin(ctx, n, true)
Copy link
Member

Choose a reason for hiding this comment

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

Should call pinning.Update when the content wasn't pinned before previous resolve here.

Copy link
Member

Choose a reason for hiding this comment

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

Oh and take the pinning lock somewhere around here - defer n.Blockstore.PinLock().Unlock()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, the lock -- good point.
Should Update be called instead of Pin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually seems like a more complicated logic is needed:

  • In first resolution, call Pin.
  • In subsequent resolutions:
    • Do nothing if the name resolves to the same node
    • Call Update if the pin has changed

I also wonder whether we should unpin when we cancel a follow.

Copy link
Member

Choose a reason for hiding this comment

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

I also wonder whether we should unpin when we cancel a follow.

I'd say yes. Also call Pin instead of Update when the hash was pinned before the follow, so we don't unintentionally unpin it.

@vyzo
Copy link
Contributor Author

vyzo commented Dec 8, 2017

@magik6k updated per review.
The most significant change is in the pinning logic. I am still keeping one goroutine per follow, depending on how strong you feel about it.

TBD: make follow interval a configuration option.

)

type ipnsFollowResult struct {
OK bool
Copy link
Member

Choose a reason for hiding this comment

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

maybe instead of a boolean, we just have a State field thats a string, since you convert it into a string later anyways

Copy link
Member

Choose a reason for hiding this comment

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

We generally use SetError to indicate that the command has failed anyways. We should probably do something like what we do in ipfs pin add: {"Follows": [CIDs]}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it adds much though -- in reality it's just a void command that can error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think it's best to just use an empty struct for void and just output ok.
As @Stebalien points out, we indicate errors with SetError anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hrm, can't have it be just void. I will make it a string as why suggested.

nc.mx.Lock()
defer nc.mx.Unlock()

follows := make([]string, 0)
Copy link
Member

Choose a reason for hiding this comment

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

could do:

follows := make([]string, 0, len(nc.follows))
``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@whyrusleeping
Copy link
Member

@vyzo I think its probably worth modifying the namesys interface to return TTLs from resolve calls so we can use them here.

@vyzo
Copy link
Contributor Author

vyzo commented Dec 9, 2017

Ok, make sense.
I will simply add an extended resolve method to the namesys api instead of changing the old one (that is too much work).

if output.OK {
state = "ok"
} else {
state = "wtf"
Copy link
Member

Choose a reason for hiding this comment

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

nit - Is that actually what we want to return to the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

certainly no, placeholder.

Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

First pass.

This PR ignores all of the design discussion from #4435. Why?

)

type ipnsFollowResult struct {
OK bool
Copy link
Member

Choose a reason for hiding this comment

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

We generally use SetError to indicate that the command has failed anyways. We should probably do something like what we do in ipfs pin add: {"Follows": [CIDs]}.

},
Subcommands: map[string]*cmds.Command{
"add": ipnsFollowAddCmd,
"pin": ipnsFollowPinCmd,
Copy link
Member

Choose a reason for hiding this comment

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

I'd collapse pin and add to add --pin or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return
}

err = n.Namecache.Follow(req.Arguments()[0], false)
Copy link
Member

Choose a reason for hiding this comment

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

Should allow multiple arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

)

const (
followInterval = 60 * time.Minute
Copy link
Member

Choose a reason for hiding this comment

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

We may also want to make this a flag ipfs follow add --refresh-interval X.

@vyzo
Copy link
Contributor Author

vyzo commented Dec 12, 2017

@Stebalien

This PR ignores all of the design discussion from #4435. Why?

Not true. It has been discussed in slack a bit, as the initial design would require very invasive changes to implement. So as a first pass, I opted for the simplest possible implementation in terms of invasiveness
and get there incrementally.

@Stebalien
Copy link
Member

It has been discussed in slack a bit

Policy: If it didn't happen on GitHub, it didn't happen. Those decisions need to go on the tracking issue.

So as a first pass, I opted for the simplest possible implementation in terms of invasiveness
and get there incrementally.

Then we:

  1. Need some form of experimental flag (default to off). Otherwise, users will come to rely on these commands. We're adding features to the API that we intend to take away.
  2. Need issues to track what needs to be done, probably in that tracking issue (maybe with multiple linked issues).

nc.mx.Lock()
defer nc.mx.Unlock()

follows := make([]string, len(nc.follows))[:0]
Copy link
Member

Choose a reason for hiding this comment

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

make([]string, 0, len(nc.follows)). Yes, technically this works but it's weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

func (nc *nameCache) resolve(ctx context.Context, name string) (path.Path, error) {
log.Debugf("resolving %s", name)

if !strings.HasPrefix(name, "/ipns/") {
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this at a higher layer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, but does it matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sanitize data as soon as it enters the program. I also advocate moving this into the command's Run function above.

Copy link
Contributor Author

@vyzo vyzo Dec 13, 2017

Choose a reason for hiding this comment

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

The Run function is perhaps inappropriate, it doesn't cover direct uses of the API.
If we are to enforce the /ipns/ prefix then it should be done at the Namecache interface.

Still, it will have to be done in 3 2 places instead of 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it to the Namecache interface methods (Add and Cancel).

@Stebalien Stebalien requested a review from keks December 12, 2017 18:15
@Stebalien
Copy link
Member

Otherwise..., LGTM. Thanks for implementing this (although I do worry that we're accumulating too many half-finished features).

@vyzo
Copy link
Contributor Author

vyzo commented Dec 12, 2017

Need some form of experimental flag (default to off). Otherwise, users will come to rely on these commands. We're adding features to the API that we intend to take away.

Well, not necessarilly take away but potentially change behaviour. So agreed on making an experimental flag for the namecache.

@vyzo
Copy link
Contributor Author

vyzo commented Dec 12, 2017

Need issues to track what needs to be done, probably in that tracking issue (maybe with multiple linked issues).

Sure, will open a bunch once we are ready to merge.

@vyzo
Copy link
Contributor Author

vyzo commented Dec 13, 2017

Added experimental flag --enable-follow-experiment.

@vyzo
Copy link
Contributor Author

vyzo commented Dec 13, 2017

@Stebalien
A few things that are part of the design discussion in #4435 but not implemented by this PR:

  • Offline resolution and resolution of expired names
  • TTL-based refresh interval
  • Persistence of follows

Copy link
Contributor

@keks keks left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

"strings"
"time"

cmds "github.com/ipfs/go-ipfs/commands"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you use go-ipfs/commands instead of go-ipfs-cmds? That is a legacy package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a subcommand of name, which is an old style command and needs references to its children.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. I think once the commands refactor has settled a bit we should update the parent command to use the new cmds lib instead though. For now I think it's okay because that process is still ongoing.

func (nc *nameCache) resolve(ctx context.Context, name string) (path.Path, error) {
log.Debugf("resolving %s", name)

if !strings.HasPrefix(name, "/ipns/") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sanitize data as soon as it enters the program. I also advocate moving this into the command's Run function above.

@ghost
Copy link

ghost commented Dec 18, 2017

This looks pretty good to me 👍 Thanks a lot :)

  • Minor issue: should we call the cancel command rm instead?
    • It's technically correct in that it cancels an ongoing repeated operation, but the opposite of add that's used in other places is rm.
  • UX issue: I ran ipfs name follow add /ipns/ipfs.io/docs/examples to try this out, then noticed that it wasn't fetching anything. I then ran the command again with --pin, and got an "already following" error.
    1. It would be fine for additional invocations of the same command to override the previous invocations.
    2. I'm thinking we should fetch the nodes regardless of --pin, i.e. do the equivalent of ipfs refs -r.

@ghost
Copy link

ghost commented Dec 18, 2017

By the way, it's great that this works with fully-qualified paths a la /ipns/ipfs.io/docs/examples, and not just with plain PeerIDs :)

@vyzo
Copy link
Contributor Author

vyzo commented Dec 18, 2017

Minor issue: should we call the cancel command rm instead?
It's technically correct in that it cancels an ongoing repeated operation, but the opposite of add that's used in other places is rm.

If we call it rm in other places, then it makes sense to call it rm here too.

@vyzo
Copy link
Contributor Author

vyzo commented Dec 18, 2017

It would be fine for additional invocations of the same command to override the previous invocations.

Agreed, just a little tricky to implement. Having to manually cancel and re-add is not great UX.

I'm thinking we should fetch the nodes regardless of --pin, i.e. do the equivalent of ipfs refs -r

So you want to traverse the object for references and resolve those too?
Maybe we should -- fetching regardless of pin seems like desirable behaviour for a cache.

@eingenito
Copy link
Contributor

eingenito commented Jan 15, 2019

@vyzo sorry - I see that it was @magik6k that picked it back up. Please ignore.

vyzo and others added 19 commits February 5, 2019 20:28
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
Use update when moving a pin, unpin on cancel when owning a pin

License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
- coalesces pin into add --pin
- add and cancel accept multiple names

License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
Also adds --refresh-interval to follow command.

License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
// just iterate over all nodes
walker := ipld.NewWalker(ctx, ipld.NewNavigableIPLDNode(toFetch, nc.dag))
if err := walker.Iterate(func(node ipld.NavigableNode) error {
return nil
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to add cids of nodes we iterate to some set and skip children we have already seen, but I'm not sure what's the best way to do this.

cc @schomatis

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to add cids of nodes we iterate to some set and skip children we have already seen, but I'm not sure what's the best way to do this.

  1. Solution at the Visitor level.

The simplest way would be for the caller to keep that set and use it in the Visitor, in there we could check if the CID of the current child the Walker is pointing to has already been visited and if so, skip it, something like:

// For every child of the current `node`:
    childCid := node.Links()[walker.ActiveChildIndex()]
    if _, ok := visited[childCid]; ok {
        walker.NextChild();
        // Skip this child, already visited.
    } else {
        break; // out of the `for`, so when this call to `Visit` ends the `Walker`
        // will iterate into this child we haven't visited.
    }

// If we have skipped all of the children the `Walker` will just continue
// the iteration going up to the current `node`'s parent.

The downside of this approach is that NavigableIPLDNode still prefetches many children in advance, so if we didn't skip all of the children, if we request at least one, NavigableIPLDNode will also start prefetching many more after that one (that we may end up actually skipping).

  1. Solution at the NavigableIPLDNode level.

In that case NavigableIPLDNode should be extended to handle this scenario (which would actually be a very nice feature). (Note here that Walker has no idea what an IPLD node is, it only operates on the very abstract notion of a DAG of any kind of node that implements the NavigableNode interface; the motivation behind this decoupling was to allow the possibility of using the Walker for other types of DAG like the dependency graphs that Gx iterates over, although at the moment IPLD is the only type of node that the Walker knows).

The way I think this could be implemented (but this is just an untested idea) would be to modify the preload function to, instead of prefetching a contiguous batch of preloadSize nodes through GetNodes, iterate the childCIDs one at a time and skip the ones already visited, calling each prefetch separately with GetNode (singular).

How should the map of visited nodes be handled in that case? Every NavigableIPLDNode is independent of its parent and children, there's no central authority, the way they communicate with each other is through the context.Context that was used in NewWalker, that context will be passed to every FetchChild issued by the Walker when going down the DAG. (Maybe this is too coupled and the NavigableIPLDNode should be extended to have its own context now, that gets passed from parent to child.) So the map could be stored there, and there could be a check that if there's no map of visited nodes, then just prefetch everything as preload does now. At the moment the Walker doesn't visit the DAG in parallel so this map would be safe (but it should be documented that in future refactorings it may need to be protected from multiple accesses).

/cc @hsanjuan who is interested in something very similar, so maybe you two could join forces here.

Copy link
Contributor

Choose a reason for hiding this comment

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

(@hsanjuan in your case, inside the Visitor you could artificially add the CIDs of the nodes you want to skip entirely in this map, which would result in the same effect.)

@magik6k
Copy link
Member

magik6k commented Feb 6, 2019

Still WIP, Pushed the prefetch variant

@npfoss npfoss mentioned this pull request Jul 23, 2019
7 tasks
@remon-nashid
Copy link

remon-nashid commented Jan 24, 2021

This feature could open IPNS to many more uses case. I wonder if the community is still interested in this implementation, or if there are newer alternatives to pinning ipns names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/in-progress In progress topic/ipns Topic ipns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants