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

refactored ipns records to point to paths #1093

Merged
merged 5 commits into from
Apr 27, 2015
Merged

refactored ipns records to point to paths #1093

merged 5 commits into from
Apr 27, 2015

Conversation

whyrusleeping
Copy link
Member

Also changed the ipns dns resolution to use the "dnslink" format

parts := strings.Split(txt, "/")
if len(parts) < 3 {
return "", ErrBadPath
}
Copy link
Member

Choose a reason for hiding this comment

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

may want to do the parts := strings.Split(txt, "/") part first, as it's faster than b58 decoding. maybe:

parts := strings.Split(txt, "/")
if len(parts) == 1 {
    kp, err := ParseKeyToPath(txt)
    if err == nil {
        return kp, nil
    }
}
if len(parts) < 3 {
    return "", ErrBadPath
}

@jbenet
Copy link
Member

jbenet commented Apr 18, 2015

comments above o/

@wking
Copy link
Contributor

wking commented Apr 18, 2015

On Fri, Apr 17, 2015 at 11:48:58PM -0700, Jeromy Johnson wrote:

Also changed the ipns dns resolution to use the "dnslink" format

I don't know if there's a more current spec out there, or if there's
an RFC or some such for “dnslink”, but the cap2pfs paper was
suggesting 1:

ipfs=XLF2ipQ4jD3U …

It looks like DNS service discovery (RFC 6763) prefers equals to
separate keys from values 2, so I think we should stick with that
here. I'm also not sure how the unqualified example:

QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD

is supposed to be interpreted, so I'd suggest we drop that value in
favor of the more explicit forms. That means I like:

ipfs=QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD
ipns=QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD
ipfs=QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD/foo
ipns=QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD/bar
ipfs=QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD/foo/bar/baz

for the DNS TXT record format (but maybe this PR is about the symlink
format?).

It's also a bit odd to use the same hash for both the IPFS and IPNS.
A valid IPNS hash will be a public key, not a directory, so that's not
going to work for:

ipfs=QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD/foo

(which asks for children of public key?). Basically, /ipns/ should
mean “please dereference this to the signed reference and then process
again” So if you had (in IPNS):

ipns=QmbqDJaoZYoGNw4dz7FqFDCf6q9EcHoMBQtoGViVFw2qv7
→ ipfs=QmbqDJaoZYoGNw4dz7FqFDCf6q9EcHoMBQtoGViVFw2qv7

Then:

ipns=QmbqDJaoZYoGNw4dz7FqFDCf6q9EcHoMBQtoGViVFw2qv7

would be equivalent to:

ipfs=ipfs=QmbqDJaoZYoGNw4dz7FqFDCf6q9EcHoMBQtoGViVFw2qv7

as it would be if there were multiple layers of mutable IPNS
references in between:

ipns=QmbqDJaoZYoGNw4dz7FqFDCf6q9EcHoMBQtoGViVFw2qv7
→ ipns=QmaV5w1y1GNdU8ZMdjJwqpCgKeNzR4fX5TAZg5Dg9TZSiT
→ ipfs=QmbqDJaoZYoGNw4dz7FqFDCf6q9EcHoMBQtoGViVFw2qv7

So I'd use different hashes for IPFS and IPNS:

ipfs=QmVyS3iAy7mvDA2HqQWm2aqZDcGDH3bCRLFkEutfBWNBqN
ipns=QmbqDJaoZYoGNw4dz7FqFDCf6q9EcHoMBQtoGViVFw2qv7
ipfs=QmVyS3iAy7mvDA2HqQWm2aqZDcGDH3bCRLFkEutfBWNBqN/foo
ipns=QmbqDJaoZYoGNw4dz7FqFDCf6q9EcHoMBQtoGViVFw2qv7/bar
ipfs=QmVyS3iAy7mvDA2HqQWm2aqZDcGDH3bCRLFkEutfBWNBqN/foo/bar/baz

For symlinks stored in an IPFS filesystem or as IPNS values, it makes
more sense to me to store them as link objects, like 3, but with an
optional child-path array:

type IPFSSymbolicLink struct {
hash Multihash // IPFS hash, or an IPNS reference
children []string; // names of children for drilling into the
// directory that the Multihash eventually resolves to.
}

and no name (that would be a key pointing to the symlink, not part of
the symlink value) or aggregate ancestor size (which is poorly defined
once you allow mutable IPNS references).

@jbenet
Copy link
Member

jbenet commented Apr 19, 2015

@wking the format of the DNS TXT record is now (after ipfs paper DRAFT 3 was written):

dnslink=<path>

For example:

dnslink=/ipfs/QmWbKNc2sutt3Mboemu6Wsr6qU4hhCwekjg4WMK2dsfJvE/foo/bar/baz
dnslink=/ipns/QmdsnptcQjjAGzE5WiDJSupA5UmGJtVS4hzsrVdMkBjAah/foo/bar/baz

and it's general for non-ipfs use cases:

dnslink=/dns/foo.com/foo/bar/baz
dnslink=/bittorrent/<infohash>
...

Of course, this needs both to catch cycles and a recursive lookup limit.

Why dnslink ? i don't believe it's in use. perhaps it should be just link, but i worry that may clash with something.

relevant: jbenet/random-ideas#28

@whyrusleeping
Copy link
Member Author

I addressed the comments above, but just realized that path.Resolver doesnt resolve ipns paths yet

@jbenet
Copy link
Member

jbenet commented Apr 19, 2015

but just realized that path.Resolver doesnt resolve ipns paths yet

maybe it should be a test case

@whyrusleeping
Copy link
Member Author

I mean, the logic for even thinking about ipns isnt there yet. hasnt been implemented yet

@jbenet
Copy link
Member

jbenet commented Apr 19, 2015

@whyrusleeping do you mean: handling a DNS -> IPNS -> IPFS record? (i.e. right now it just handles DNS -> IPFS?

@whyrusleeping
Copy link
Member Author

@jbenet no i mean, if i do:

resolver := path.NewResolver(...)
mypath := path.Path("/ipns/QmBlahBlah/something/else")
resolver.ResolvePath(mypath)

It wont work. the resolver doesnt understand ipns

@jbenet
Copy link
Member

jbenet commented Apr 20, 2015

@whyrusleeping ah right, not the basic resolver.

@whyrusleeping
Copy link
Member Author

not sure what the best way forward here is. because the ipns resolution stuff uses the straight ipfs resolver... so adding ipns into the path.Resolver directly may make things funky

@jbenet
Copy link
Member

jbenet commented Apr 20, 2015

so adding ipns into the path.Resolver directly may make things funky

let's not do that.

namesys package should probably have its own resolver? or how low-level do you need it?

@whyrusleeping whyrusleeping added sprint and removed status/in-progress In progress labels Apr 20, 2015
@jbenet jbenet mentioned this pull request Apr 20, 2015
34 tasks
@wking wking mentioned this pull request Apr 21, 2015
@whyrusleeping
Copy link
Member Author

travis is awesome: https://travis-ci.org/ipfs/go-ipfs/jobs/59675730

but otherwise i think this is good to go


p = path.FromSegments(append(pathHead, extensions...)...)
//p = path.RebasePath(path.FromSegments(extensions...), basePath)
return resolveRecurse(n, path.FromSegments(append(respath.Segments(), extensions...)...), depth+1)
Copy link
Member

Choose a reason for hiding this comment

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

this is hard to read, maybe use vars?

newpathSeg := append(respath.Segments(), extensions...)
newpath := path.FromSegments(newpathSeg...)
return resolveRecurse(n, newpath, depth+1)

@jbenet
Copy link
Member

jbenet commented Apr 23, 2015

@whyrusleeping

  • shut down iptb in test
  • path validity + tests
  • hard to read
  • tests dont pass at all

@whyrusleeping
Copy link
Member Author

@chriscool will do!

@whyrusleeping
Copy link
Member Author

i love these random little failures: https://travis-ci.org/ipfs/go-ipfs/jobs/59829161#L2957

reliable testing is hard to get right

@chriscool
Copy link
Contributor

The failure is:

expecting success:
test_kill_repeat_10_sec $IPFS_PID

not ok 19 - 'ipfs daemon' can be killed

#
# test_kill_repeat_10_sec $IPFS_PID
# 

Maybe 10 seconds are not enough on Travis, or maybe something can be done so that ipfs daemon exits faster.

@whyrusleeping
Copy link
Member Author

We should probably take some time to investigate ipfs shutdown a little better, ten seconds should be enough

@whyrusleeping
Copy link
Member Author

@jbenet I beleive that i have addressed your concerns

@jbenet
Copy link
Member

jbenet commented Apr 27, 2015

iptb is so useful. ❤️

@jbenet
Copy link
Member

jbenet commented Apr 27, 2015

LGTM, RFM. i think this needs to be rebased.

jbenet added a commit that referenced this pull request Apr 27, 2015
refactored ipns records to point to paths
@jbenet jbenet merged commit 54cb80e into master Apr 27, 2015
@jbenet jbenet removed the sprint label Apr 27, 2015
@jbenet jbenet deleted the feat/ipns-paths branch April 27, 2015 05:51
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
refactored ipns records to point to paths

This commit was moved from ipfs/kubo@54cb80e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/ipns Topic ipns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants