Non-self IPNS names need republishing

[ghost]

Version information: 0.4.7

Type:

Priority: P2

Description:

It doesn't look like names added by ipfs key gen ever get republished. I didn't actually test it, but I'm fairly certain there's no code for it.

It should:

• at startup, call Republisher.AddName() for each key in the keystore.
• same for every run of ipfs key gen, add the name to the republisher.

cc @edsilv I think this will affect you. You'll have to run ipfs name publish --key=<name> <hash> every 12 hours, for now.

[whyrusleeping]

@lgierth correct. There is another issue open for this.

We were waiting on figuring out proper UX for this

[edsilv]

@lgierth I can confirm that republishing approx every 12 hours is necessary.

[whyrusleeping]

@edsilv I'm trying to figure out a good UX for the automatic republishing. I don't want to automatically republish values for every single key in the keystore (I have over 1000 in my local keystore, for testing) but i want it to be easy for users to opt into republishing.

Would starting republishing on a key after you've published anything with it once feel like a good solution to you?

cc @Kubuxu @lgierth

[edsilv]

@whyrusleeping I think it feels like an opt-out thing where I would have assumed that the content would be permanently available at that key. Perhaps you could pass --republish=false if you want to opt out, and it defaults to true?

[whyrusleeping]

@edsilv Hrm... maybe i should provide an example of what i mean:

$ ipfs key gen beepboop --type ed25519
QmXxaV486674p7N4TVRwwbic1rkHZVa5Jn3vmNH3BeiX9h
$ # at this point, nothing is being published or republished for this key
$ ipfs name publish --key=beepboop QmaFLV5q4Gs3x89evD4ZXZrjqUWnybU5Zb9iP5ymzfYvXE
Published to QmXxaV486674p7N4TVRwwbic1rkHZVa5Jn3vmNH3BeiX9h: /ipfs/QmaFLV5q4Gs3x89evD4ZXZrjqUWnybU5Zb9iP5ymzfYvXE
$ # Now the value is published to that key, and it will continue to be republished until you delete the key

[edsilv]

That's what I did, except I used --type rsa --size 2048

https://gist.github.com/edsilv/2c80b0d565352e5cd6b7ceba5fbc156a

[whyrusleeping]

@edsilv My above comment was proposing a UX, not describing what is now. Would what i posted feel like 'correct' behaviour? (I think so )

[edsilv]

@whyrusleeping Yep, looks right. Potentially with an extra option to opt-out of republishing. Sorry for the misunderstanding...

[whyrusleeping]

No worries, We will try and have this done for the 0.4.9 relelase :)

[matthewrobertbell]

If I am regularly resolving the value of an IPNS key, and the publishing node goes down for more than 12? hours, does that mean that resolving will fail, or are names I've looked up recently automatically republished?

If not, I think this would be great functionality, and it seems related to this issue. I feel this is especially important for being able to reliably get content on non-internet connected networks.

[ghost]

Hrm... maybe i should provide an example of what i mean:

[...]

LGTM 👍

[whyrusleeping]

This one is pretty high priority, can someone help out and work on it?

[keremgocen]

I'd like to have a go at it @whyrusleeping.

As I gather from comments, could you please review these action items?;

• you're expecting every key in the keystore to be republished at startup unless the flag --republish=false is provided for a particular key during it's creation?
  • this goal could be achieved by applying setupIpnsRepublisher's AddName to all present keys instead of the local identity only
• what would be a good option to retain a key's republish state to check when traversing through the found keys?

[whyrusleeping]

@keremgocen Great!
The first thing i'd like to see, would be on daemon startup, adding any keys to the republisher that have had a value previously published for them (as evidenced by a locally stored value in the datastore for that key). The flag to opt out of publishing a particular key can come later.

[kevina]

@keremgocen, @whyrusleeping asked me to have a look at this, have you done anything with it yet?

[keremgocen]

@kevina unfortunately I did not invest any time in implementing this yet but the action items have been laid out previously and it should be easy to take it from there, thanks!

[kevina]

@whyrusleeping are you looking for something like this in setupIpnsRepublisher:

	for _, peer := range n.Peerstore.Peers() {
		n.IpnsRepub.AddName(peer)
	}

or is there something more I need to do? Sorry I'm not very familiar with that part of the codebase.

[whyrusleeping]

@kevina you'll want to grab the keys to republish from the keystore, not the peerstore. Also make sure to only add keys to the republisher if they already have a value published for them (just check locally)

[kevina]

@whyrusleeping so how do I check locally? Where is that information stored.

[whyrusleeping]

Oh, it looks like the republisher already does all that checking: https://github.com/ipfs/go-ipfs/blob/master/namesys/republisher/repub.go#L91

I think all you need to do then is add all the names (peer.IDs) from the keystore into the republisher on startup.

[whyrusleeping]

@kevina so around here: https://github.com/ipfs/go-ipfs/blob/master/core/core.go#L376

For each item in n.Keystore.List() you'll have to call keystore.Get() and turn that private key into a peer ID and add that peer.ID to the republisher.

To test this, take a look at t0240. You'll want to have one node make a new key, and publish something with it, then run through those tests with the newly created key. This also reminds me that we probably want to add newly created keys to the republisher in ipfs key gen so they don't have to restart their daemon to get keys to auto-republish.

[kevina]

@whyrusleeping okay thanks, I think I can figure it out now, there where a few key points I wasn't getting.

[kevina]

@whyrusleeping does this look right?

	ks := n.Repo.Keystore()
	keys, err := ks.List()
	if err != nil {
		return err
	}
	for _, keyId := range keys {
		privKey, err := ks.Get(keyId)
		if err != nil {
			return err
		}
		peer, err := peer.IDFromPrivateKey(privKey)
		if err != nil {
			return err
		}
		n.IpnsRepub.AddName(peer)
	}

inserted at the place you suggested.

[whyrusleeping]

Looks about right, yeah

[kevina]

@whyrusleeping based on the current implementation it seams to me it would be better to have the re-publisher just iterator thorough the keystore directory rather than have its own republish list, #3951 does exactly that, it won't try and republish keys without any local ipns names anyway so i'm not sure what purpose having a separate republish list is serving. This way we don't need to worry about populating the list on startup or added newly generated keys when after ipfs key gen

What do you think?

Note that the p.r. contains two checkpoint commits, the first one makes progress towards doing it the way you suggestion, and the second one reverts much of the first commit and does it the way I described above.

[whyrusleeping]

@kevina that sounds good to me. As you said, it makes more sense to just have one list.

[skorokithakis]

Does anyone know which version this feature will be in? It's not visible in the CLI, so I can't tell...

[bonedaddy]

Is this still a valid issue?

[magik6k]

This should be fixed in #3951 (in v0.4.10)

[bonedaddy]

strange, I'll have to do some testing since I don't think I've ever seen one of my nodes republish a record.

does the republishing require access to the same private key used to publish the record? ie if I remove the key from the keystore it won't get republished?

[Stebalien]

Yes.