Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
IPNS-over-PubSub as an Independent Transport #218
Changes from 1 commit
b44ee6b
a1a0e48
a1d0888
faa8393
7e97f06
a370a6a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
/ipns/BINARY_ID
./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)
wherekey = multihash of IPNS public key
routing-record = what the form of the routing record is (
BINARY_ID = binary representation of key
wherekey = 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:
Renaming
BINARY_ID
toBINARY_IPNS_ID
should do the trick, it match "IPNS ID" nicely.✍️ Rename pubsub topic, make it a valid IPNS path?
My concerns with
/record/
:/record/
topic without deserializing base64ipfs pubsub ls
(compare:
/record/{base64}
vs/ipns/{cid}
)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:
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.
👍 to that whole section. Those names sound fine to me and are at least not ambiguous anymore.
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 likestate = 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:
/ipns/base64(multihash(IPNS Public Key))
(could also be base32 if we wanted)base64(multihash(IPNS Public Key)
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)))
@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.
Turns out this isn't how it works in go. Instead, we just base32 the entire key:
base32("/ipns/binary_peer_id)
.We use
/record
because thego-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.
The issue here was that record keys are arbitrary binary but pubsub requires utf-8 keys.
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.