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

IPNS republisher should not rely on DHT to store (possibly expired) records #4749

Closed
dirkmc opened this issue Mar 1, 2018 · 18 comments · Fixed by #5007
Closed

IPNS republisher should not rely on DHT to store (possibly expired) records #4749

dirkmc opened this issue Mar 1, 2018 · 18 comments · Fixed by #5007
Assignees

Comments

@dirkmc
Copy link
Contributor

dirkmc commented Mar 1, 2018

The republisher relies on the DHT to store records, but those records may expire before republish occurs (eg if the republishing node goes offline). We should store them somewhere else in the datastore.
See #4742 (comment) for more details

@dirkmc
Copy link
Contributor Author

dirkmc commented Mar 1, 2018

@whyrusleeping do you have a suggestion for how we should store these records?

@Stebalien
Copy link
Member

Proposal: make the republisher wrap the namesys service in a republishing namesys service. When calling Publish on the RepublishingNamesys, it would store the record in a datastore and then continue to republish that record until replaced or deleted.

Thoughts?

@whyrusleeping
Copy link
Member

That sounds like a reasonable proposal to me.

@dirkmc
Copy link
Contributor Author

dirkmc commented Mar 1, 2018

Should we merge in the PR to use variadic options for the namesys interface first?
#4733

@dirkmc
Copy link
Contributor Author

dirkmc commented Mar 1, 2018

@Stebalien is this what you were imagining? #4753
Note: the relevant commit is d68b5ce

@Stebalien
Copy link
Member

So, as I noted on that PR, the underlying namesys relies on this record itself to keep track of the sequence number.

I can think of three solutions:

  1. Have the underlying namesys record the sequence number separately along with the last published value. IMO, we should do that regardless. Additionally, have the republishing nameservice record which names it should republish (and their last publish times?).
  2. Have the namesys expose known, published records. That would allow us to keep the republisher simple. However, that also complicates the namesys Publisher interface (which, arguably, shouldn't expose this information).
  3. Just combine the two.

@hsanjuan
Copy link
Contributor

hsanjuan commented Mar 2, 2018

Quick note: you should be able to update to latest-gx-tagged go-libp2p-kad-dht, go-ipfs-routing, go-libp2p-record for this. I should have left everything working on that front.

@dirkmc
Copy link
Contributor Author

dirkmc commented Mar 7, 2018

I noticed that the Pubsub publisher puts an IPNS record directly into the datastore, so that the republisher can later retrieve the record's sequence number. But it looks like namesys will always publish to both the DHT and to Pubsub anyway, so is it necessary for Pubsub publisher to do this?

@dirkmc
Copy link
Contributor Author

dirkmc commented Mar 7, 2018

And I guess a related question: Should the republisher care about records published directly to Pubsub (but not to the DHT)?

@vyzo
Copy link
Contributor

vyzo commented Mar 7, 2018

That's so that both itself and the DHT publisher can find it; the latter is important if you run your daemon having disabled pubsub.

@vyzo
Copy link
Contributor

vyzo commented Mar 7, 2018

And yes, i think the republisher should care about pubsub published records, as these will have a different seqno.
Also, it's not inconceivable that we do just pubsub publishing in the future.

@dirkmc
Copy link
Contributor Author

dirkmc commented Mar 7, 2018

It looks like the pubsub record sequence number may be different from the DHT record sequence number for the same value in the following scenario:

  • A client calls namesys Publisher to update an IPNS record
  • namesys Publisher calls the DHT publisher, which increments the sequence number
  • namesys Publisher calls the pubsub publisher, which also increments the sequence number

I'm not sure if that actually causes any problems, but it's probably worth fixing

@vyzo
Copy link
Contributor

vyzo commented Mar 7, 2018

Yes, that's an artifact of the implementation.
Ideally they would both use the same seqno, but it seemed too complicated at the time i was writing that code and opted for different seqnos.
This kept the implementation straightforward, as the two publishers could be kept completely separate and without changes to the extant publisher.

@vyzo
Copy link
Contributor

vyzo commented Mar 7, 2018

But since we are hacking the publishers, we might as well go the extra mile and have them use the same seqno for the record.
It shouldn't be causing any problems though.

@dirkmc
Copy link
Contributor Author

dirkmc commented Mar 7, 2018

Yes I agree, @Stebalien suggested simply comparing the values to make sure the sequence number is not incremented when the same value is published.

@dirkmc
Copy link
Contributor Author

dirkmc commented Mar 7, 2018

Is there a case in which we would want to publish only to pubsub (but not to the DHT)? I'm wondering if the pubsub publisher should call out to the DHT publisher itself, which would take care of all the sequence number handling that the pubsub publisher has to do at the moment

@vyzo
Copy link
Contributor

vyzo commented Mar 8, 2018

Currently no, but we might want this in the future.

@dirkmc
Copy link
Contributor Author

dirkmc commented Mar 8, 2018

I think if we don't always publish to the DHT it will cause an issue with publishing to a key shared between peers:

  • An IPNS record for key A is published on peer A to pubsub but not to the DHT
  • namesys Publish is called on peer B for key A
  • Peer B goes out to the DHT to get the latest sequence number for key A
  • The IPNS record published to pubsub (but not the DHT) by peer A will not be found, so the publisher on peer B will get the wrong sequence number

Stebalien added a commit that referenced this issue May 9, 2018
fixes #4749

License: MIT
Signed-off-by: Steven Allen <[email protected]>
Stebalien added a commit that referenced this issue May 9, 2018
fixes #4749

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien Stebalien mentioned this issue May 9, 2018
8 tasks
@ghost ghost assigned Stebalien May 9, 2018
@ghost ghost added the status/in-progress In progress label May 9, 2018
Stebalien added a commit that referenced this issue May 16, 2018
fixes #4749

License: MIT
Signed-off-by: Steven Allen <[email protected]>
Stebalien added a commit that referenced this issue May 17, 2018
fixes #4749

License: MIT
Signed-off-by: Steven Allen <[email protected]>
Stebalien added a commit that referenced this issue Jun 1, 2018
fixes #4749

License: MIT
Signed-off-by: Steven Allen <[email protected]>
Stebalien added a commit that referenced this issue Jun 1, 2018
fixes #4749

License: MIT
Signed-off-by: Steven Allen <[email protected]>
Stebalien added a commit that referenced this issue Jun 1, 2018
fixes #4749

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@ghost ghost removed the status/in-progress In progress label Jun 3, 2018
djdv pushed a commit that referenced this issue Jun 27, 2018
fixes #4749

License: MIT
Signed-off-by: Steven Allen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants