-
Notifications
You must be signed in to change notification settings - Fork 232
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
IPNS-over-PubSub as an Independent Transport #218
Conversation
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.
@hugomrdias is OOO this week, but I made a quick pass focused on readability:
Co-Authored-By: Marcin Rataj <lidel@lidel.org>
Co-Authored-By: Marcin Rataj <lidel@lidel.org>
Co-Authored-By: Marcin Rataj <lidel@lidel.org>
@lidel you think we're ready to merge this? |
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.
(resolved some questions to make it easier for others to review)
Small question about base64 below, apart from that LGTM.
Would like to have @Stebalien or someone familiar with historical ipfs/spec
process to eyeball this.
|
||
For a given IPNS local record key described in the IPNS Specification the PubSub topic is: | ||
|
||
**Topic format:** `/record/base64url-unpadded(key)` |
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 see ipfs/specs/naming#local-record uses /ipns/base32(<HASH>)
.
We could use the same naming convention in all places, if possible.
What would be less work, change this to /ipns/base32(key)
or the other way around?
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 about the inconsistency, there's two issues here:
- I think that area of the IPNS spec could use some clarification. Here, we're actually referring to the ipfs/specs/naming#routing-record
/ipns/BINARY_ID
. - This spec is currently using
/record
instead of/ipns
. Should we make it use/ipns
? If starting with/ipns
is not necessary for routing records across multiple routers then we should probably just remove the section on routing records.
What do you think?
Edit: Just an update/clarification that IPNS over PubSub currently uses /record
as the topic. It's possible this was an oversight, is this something we should change going forward?
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 asked @Stebalien what he thought about this and his understanding of the linked IPNS is that:
local-record = what we put in a local datastore = /ipns/base32(key)
where key = multihash of IPNS public key
routing-record = what the form of the routing record is (BINARY_ID = binary representation of key
where key = multihash of IPNS public key
This means our topic format is /record/base64url-unpadded(/ipns/BINARY_ID)
. The topic format being /record
is therefore not actually incompatible with the already described routing-record for IPNS.
However, we should give BINARY_ID a new name in the other spec that makes more sense. One set of words we tend to use is IPNS private key = signing key, IPNS public key = verification key, IPNS key = multihash(IPNS public key)
. However, we could probably use better words, any suggestions?
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.
@aschmahmann
I've been thinking about naming and got some ideas on how to simplify things a bit:
👍 unify popular terms
As you noted, public
/private
provides good mental model and we should keep it in specs.
What is ambiguous is IPNS key
which is just a shortcut/fingerprint that acts as "IPNS identifier".
I suspect something like this would have the least cognitive overhead:
- IPNS Private Key = signing key
- IPNS Public Key = verification key
- IPNS ID / Identifier = multihash(IPNS Public Key)
Renaming BINARY_ID
to BINARY_IPNS_ID
should do the trick, it match "IPNS ID" nicely.
✍️ Rename pubsub topic, make it a valid IPNS path?
My concerns with /record/
:
- ambiguity during debugging
- can't guess what is the purpose of a
/record/
topic without deserializing base64
- can't guess what is the purpose of a
- adding unnecessary noise when doing
ipfs pubsub ls
- can't just open IPNS path without doing additional conversion
(compare:/record/{base64}
vs/ipns/{cid}
)
- can't just open IPNS path without doing additional conversion
What if we simplify this and rename pubsub topic to follow text representation proposed in libp2p/specs#209? Basically /ipns/cid(IPNS ID)
For CIDv1 default text representation would be in Base32:
base32(cid(1, libp2p-key-codec, multihash(IPNS Public Key)))
Key idea here is that when you do ipfs pubsub ls
, you get topics that are valid IPNS content paths. User can just copy topic name and IPNS resource will load, without doing any additional conversion (huge UX win).
Thoughts?
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.
unify popular terms
👍 to that whole section. Those names sound fine to me and are at least not ambiguous anymore.
Rename pubsub topic, make it a valid IPNS path
This bothers me too, especially the ipfs pubsub ls
being pretty unhelpful bit.
I think the reason for /record
was to indicate that PubSub has this generalized strategy for dealing with records (e.g. the "keep latest record" strategy could be replaced with basically any function that looks like state = merge(state, new record)
as described in libp2p/go-libp2p-pubsub-router#36).
I'm a little concerned about using /ipns/base32(cid(1, libp2p-key-codec, multihash(IPNS Public Key)))
as a PubSub topic. While libp2p/specs#209 still allows PeerIDs to be searched for or referenced on the network by multihash, this change would make IPNS IDs effectively CIDs since the network level representation in PubSub is the topic name.
A couple options to do something like this might be:
- Just make IPNS IDs CIDs
- this will likely run into the same problems as PeerIDs as CIDs (e.g. bump CID version number and now the topic is different)
- Use
/ipns/base64(multihash(IPNS Public Key))
(could also be base32 if we wanted)- indicates it's an IPNS record, but not super helpful for figuring out which one (since IPNS IDs are not
base64(multihash(IPNS Public Key)
- could combine this with an aliasing function for debugging tools (e.g.
ipfs pubsub ls
or Wireshark) that processes it into a format we might prefer like/ipns/base32(cid(1, libp2p-key-codec, multihash(IPNS Public Key)))
- note that deciding what the preferred format is should probably be a PR to the IPNS spec that's similar (or points to) RFC 0001: Text Peer Ids as CIDs libp2p/specs#209.
- indicates it's an IPNS record, but not super helpful for figuring out which one (since IPNS IDs are not
@Stebalien any thoughts on /record
vs /ipns
for the PubSub topic?
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.
local-record = what we put in a local datastore = /ipns/base32(key) where key = multihash of IPNS public key
Turns out this isn't how it works in go. Instead, we just base32 the entire key: base32("/ipns/binary_peer_id)
.
This spec is currently using /record instead of /ipns. Should we make it use /ipns? If starting with /ipns is not necessary for routing records across multiple routers then we should probably just remove the section on routing records.
We use /record
because the go-libp2p-routing-helpers
is record type agnostic. It's not specific to IPNS records, it works with all records. We wanted a namespace this special router could own inside the pubsub topic namespace so it wouldn't conflict with other pubsub topics.
Really, IPNS-over-PubSub should be renamed to Records-over-PubSub.
can't guess what is the purpose of a /record/ topic without deserializing base64
The issue here was that record keys are arbitrary binary but pubsub requires utf-8 keys.
adding unnecessary noise when doing ipfs pubsub ls
I agree this kind of sucks but this is one of the reasons we have ipfs name pubsub subs
.
My thoughts: leave it as it is, maybe rename this spec to "records over pubsub".
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 could rename the spec as this is really just the spec for go-libp2p-routing-helpers
.
However, given that the record agnostic go-libp2p-routing-helpers
requires a record validator and implicitly assumes that other subscribers to the same topic have the same validators it might be reasonable for it to take both a protocol prefix and a validator.
The question is, do we get anything out of having this /record
namespace since it looks like /ipns
might be more helpful?
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.
Yes, we get to reuse the same validator logic, the same record keys, the same everything. We can add support for a new record type to the entire system just by adding the record type to the validator.
@aschmahmann I was going to rebase this PR onto master, but found that it got created in your fork. Would you prefer:
Also, this is a good opportunity to document the multiple routers of IPNS and not just make the case for PubSub. |
I can create a new branch in this repo.
Absolutely, also we should make the IPNS spec itself cleaner (e.g. #205). There's also some ideas about what the spec should be for an IPNS address (i.e. |
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.
You know, all this would be so much simpler if we CIDs as message IDs...
hey @aschmahmann , I am not sure why this is hanging here, should merge? |
@hsanjuan don't think there's any particular reason, other than I was being a perfectionist and got a bit distracted by other priorities 😬. The two things I wanted to revise/revisit were:
I'll try and fix this up this week/weekend and if it's not done by then let's merge on Monday? You're right that this has been sitting here too long. |
Thanks! /remind me to merge on Monday |
ah, reminders only work on issues, not PRs :( |
@hsanjuan let's merge this before I forget again (I don't have permissions) 😄. Updates will come in a followup PR. |
Interesting, only ipfs/docs or ipfs/admin can merge in this repo, but spec writers like you are not in those teams. |
IPNS over PubSub does not currently operate as an independent transport since it only receives updates, and has no way of getting existing published records. Additionally, even the current IPNS over PubSub does not have a spec.
This PR is meant to develop an IPNS over PubSub spec based on some of the ongoing work at ipfs/kubo#6447.
This spec is also dependent on a libp2p spec for the Fetch protocol, which will be posted here as soon as a PR is up.
Please comment, make suggestions, and help make this spec better and easier to work with 😄
^ @hugomrdias @lidel would appreciate whether this spec looks complete enough that you wouldn't have to look at the Go code to implement this in JS.