-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
base: master
Are you sure you want to change the base?
Conversation
core/commands/follow.go
Outdated
OK bool | ||
} | ||
|
||
var IpnsFollowCmd = &cmds.Command{ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
core/commands/follow.go
Outdated
|
||
res.SetOutput(&ipnsFollowResult{true}) | ||
}, | ||
Arguments: []cmdkit.Argument{ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will reorder.
namecache/namecache.go
Outdated
) | ||
|
||
const ( | ||
followInterval = 60 * time.Minute |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
namecache/namecache.go
Outdated
defer nc.mx.Unlock() | ||
|
||
if _, ok := nc.follows[name]; ok { | ||
return |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
namecache/namecache.go
Outdated
defer nc.mx.Unlock() | ||
|
||
cancel, ok := nc.follows[name] | ||
if ok { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
There was a problem hiding this comment.
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 }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, prettier this way
namecache/namecache.go
Outdated
return follows | ||
} | ||
|
||
func (nc *nameCache) followName(ctx context.Context, name string, pinit bool) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
namecache/namecache.go
Outdated
name = "/ipns/" + name | ||
} | ||
|
||
p, err := nc.nsys.Resolve(ctx, name) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
namecache/namecache.go
Outdated
return | ||
} | ||
|
||
err = nc.pinning.Pin(ctx, n, true) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@magik6k updated per review. TBD: make follow interval a configuration option. |
core/commands/follow.go
Outdated
) | ||
|
||
type ipnsFollowResult struct { | ||
OK bool |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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]}
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
namecache/namecache.go
Outdated
nc.mx.Lock() | ||
defer nc.mx.Unlock() | ||
|
||
follows := make([]string, 0) |
There was a problem hiding this comment.
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))
``
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@vyzo I think its probably worth modifying the namesys interface to return TTLs from resolve calls so we can use them here. |
Ok, make sense. |
core/commands/follow.go
Outdated
if output.OK { | ||
state = "ok" | ||
} else { | ||
state = "wtf" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
certainly no, placeholder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this 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?
core/commands/follow.go
Outdated
) | ||
|
||
type ipnsFollowResult struct { | ||
OK bool |
There was a problem hiding this comment.
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]}
.
core/commands/follow.go
Outdated
}, | ||
Subcommands: map[string]*cmds.Command{ | ||
"add": ipnsFollowAddCmd, | ||
"pin": ipnsFollowPinCmd, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
core/commands/follow.go
Outdated
return | ||
} | ||
|
||
err = n.Namecache.Follow(req.Arguments()[0], false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should allow multiple arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
namecache/namecache.go
Outdated
) | ||
|
||
const ( | ||
followInterval = 60 * time.Minute |
There was a problem hiding this comment.
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
.
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 |
Policy: If it didn't happen on GitHub, it didn't happen. Those decisions need to go on the tracking issue.
Then we:
|
namecache/namecache.go
Outdated
nc.mx.Lock() | ||
defer nc.mx.Unlock() | ||
|
||
follows := make([]string, len(nc.follows))[:0] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
namecache/namecache.go
Outdated
func (nc *nameCache) resolve(ctx context.Context, name string) (path.Path, error) { | ||
log.Debugf("resolving %s", name) | ||
|
||
if !strings.HasPrefix(name, "/ipns/") { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
).
Otherwise..., LGTM. Thanks for implementing this (although I do worry that we're accumulating too many half-finished features). |
Well, not necessarilly take away but potentially change behaviour. So agreed on making an experimental flag for the namecache. |
Sure, will open a bunch once we are ready to merge. |
Added experimental flag |
@Stebalien
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
core/commands/follow.go
Outdated
"strings" | ||
"time" | ||
|
||
cmds "github.com/ipfs/go-ipfs/commands" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
namecache/namecache.go
Outdated
func (nc *nameCache) resolve(ctx context.Context, name string) (path.Path, error) { | ||
log.Debugf("resolving %s", name) | ||
|
||
if !strings.HasPrefix(name, "/ipns/") { |
There was a problem hiding this comment.
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.
This looks pretty good to me 👍 Thanks a lot :)
|
By the way, it's great that this works with fully-qualified paths a la |
If we call it rm in other places, then it makes sense to call it rm here too. |
Agreed, just a little tricky to implement. Having to manually cancel and re-add is not great UX.
|
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>
9fbd159
to
4f56358
Compare
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
- 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).
- 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.
There was a problem hiding this comment.
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.)
Still WIP, Pushed the prefetch variant |
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. |
Implements ipfs follow in a minimally non intrusive way; #4435
TODO: