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/patches #1739

Merged
merged 6 commits into from
Oct 1, 2015
Merged

Ipns/patches #1739

merged 6 commits into from
Oct 1, 2015

Conversation

whyrusleeping
Copy link
Member

This PR contains three distinct commits that make ipns more reliable as a temporary fix until finalize the ipld records implementation.

Details on each commit provided in commit messages.

@whyrusleeping
Copy link
Member Author

weird failures i'm fairly certain arent related: https://travis-ci.org/ipfs/go-ipfs/jobs/81498938#L247

@@ -104,6 +105,7 @@ type IpfsNode struct {
Diagnostics *diag.Diagnostics // the diagnostics service
Ping *ping.PingService
Reprovider *rp.Reprovider // the value reprovider system
IpnsRepub *ipnsrp.Republisher
Copy link
Member

Choose a reason for hiding this comment

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

We'll eventually want to organize this stuff cleaner. maybe bundle all the IPNS parts into one struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. an IpfsNode does a lot

@jbenet
Copy link
Member

jbenet commented Sep 23, 2015

needs some more tests. can manipulate the timeouts to test the various edge cases.

@whyrusleeping
Copy link
Member Author

@jbenet what tests are you thinking of? About all I could come up with is to shorten the interval and check that the publish actually happened.

@jbenet
Copy link
Member

jbenet commented Sep 24, 2015

  • check the publish happens
  • check republish happens
  • check records disappear after interval if (re)publisher goes down.
  • check records re-appear if publisher returns.

(what was the problem with iptb on travis?)

@whyrusleeping
Copy link
Member Author

the issue with iptb was that it didnt use 'port zero' since it would have had to start up a daemon, get the address it chose from the output, and write that to the other nodes bootstrap configs. I still use iptb all the time locally

@whyrusleeping
Copy link
Member Author

hey look! our favorite type of race/panic!

https://travis-ci.org/ipfs/go-ipfs/jobs/82099648

@whyrusleeping
Copy link
Member Author

@jbenet I've added a test for the republisher

@jbenet
Copy link
Member

jbenet commented Sep 25, 2015

@whyrusleeping durability concerns o/

@whyrusleeping
Copy link
Member Author

@jbenet ping. There isnt anything to worry about here as far as durability goes. It the node goes down, we restart it and things continue where they left off.

@@ -226,6 +228,12 @@ func (n *IpfsNode) startOnlineServicesWithHost(ctx context.Context, host p2phost
// setup name system
n.Namesys = namesys.NewNameSystem(n.Routing)

// setup ipns republishing
n.IpnsRepub = ipnsrp.NewRepublisher(n.Routing, n.Repo.Datastore(), n.Peerstore)
n.IpnsRepub.AddName(n.Identity)
Copy link
Member

Choose a reason for hiding this comment

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

ok so this is why we don't yet need durability o/ -- we provide durability elsewhere, and always load up the state ourselves.

@jbenet
Copy link
Member

jbenet commented Sep 25, 2015

Feeling good about this. i think we should have some sharness and/or iptb tests around this. it's a large complicated system with lots of expectations.

@whyrusleeping
Copy link
Member Author

@jbenet So, i fixed iptb so that it should work with port zero stuff. and then i realized that in order to be able to tweak the timeouts and intervals of the rebroadcaster through the cli, it would take a lot of reworking things... do you think thats worth the time investment?

@jbenet
Copy link
Member

jbenet commented Sep 25, 2015

Could be a config option. Probably should be so we can play with it. 

May also want to make it default to every 4hrs for now actually, because network is still small and churn. Prob confirm that it isn't less than every 5min

@whyrusleeping
Copy link
Member Author

@jbenet what would we call this config option? I have the change to four hours in, and it shouldnt be difficult at all to grab a value from the config if it exists, just gotta decide on where it lives.

@whyrusleeping
Copy link
Member Author

Things are looking as good as i think theyre gonna. The sharness test seems to fail occasionally. I'm guessing something takes a long time for no reason at all and the republisher fires. ping @jbenet

@jbenet
Copy link
Member

jbenet commented Sep 29, 2015

@whyrusleeping feedback above o/

@whyrusleeping
Copy link
Member Author

I think it hangs because of a potential bug in the dht query code. On a localhost connection like this, it should quickly exhaust all nodes in the network in a query, and stop the query when it gets there...

@whyrusleeping
Copy link
Member Author

@jbenet alright, lookin good!

This commit adds a sequence number to the IpnsEntry protobuf
that is used to determine which among a set of entries for the same key
is the 'most correct'.

GetValues has been added to the routing interface to retrieve a set of
records from the dht, for the caller to select from.

GetValue (singular) will call GetValues, select the 'best' record, and
then update that record to peers we received outdated records from.
This will help keep the dht consistent.

License: MIT
Signed-off-by: Jeromy <[email protected]>
Queries previously would sometimes only query three (alpha value) peers
before halting the operation. This PR changes the number of peers
grabbed from the routing table to start a query to K.

Dht nodes would also not respond with enough peers, as per the kademlia
paper, this has been changed to from 4 to 'K'.

The query mechanism itself also was flawed in that it would pull all the
peers it had yet to query out of the queue and 'start' the query for
them. The concurrency rate limiting was done inside the 'queryPeer'
method after the goroutine was spawned. This did not allow for peers
receiver from query replies to be properly queried in order of distance.

License: MIT
Signed-off-by: Jeromy <[email protected]>
This commit adds a very basic process that will periodically go through
a list of given ids and republish the values for their ipns entries.

License: MIT
Signed-off-by: Jeromy <[email protected]>
@whyrusleeping
Copy link
Member Author

aaaand rebased.

@whyrusleeping
Copy link
Member Author

@jbenet RFM

@jbenet
Copy link
Member

jbenet commented Sep 30, 2015

The diff is too big to comment inline :/

(what is kingpin used for?)

@jbenet
Copy link
Member

jbenet commented Sep 30, 2015

@whyrusleeping

@whyrusleeping
Copy link
Member Author

kingpin is the flags library that iptb uses.

@whyrusleeping
Copy link
Member Author

@jbenet addressed comments, and split up the vendoring commit.

@jbenet
Copy link
Member

jbenet commented Sep 30, 2015

@ghost ghost mentioned this pull request Sep 30, 2015
88 tasks
License: MIT
Signed-off-by: Jeromy <[email protected]>
@whyrusleeping
Copy link
Member Author

did those things, @jbenet the things were done did.

@jbenet
Copy link
Member

jbenet commented Oct 1, 2015

@whyrusleeping great! thanks! 👏 👏

I'm not 100% this will work perfectly, but let's merge it and see how it does?

jbenet added a commit that referenced this pull request Oct 1, 2015
@jbenet jbenet merged commit 5b2d2eb into master Oct 1, 2015
@jbenet jbenet removed the status/in-progress In progress label Oct 1, 2015
@jbenet jbenet deleted the ipns/patches branch October 1, 2015 19:55
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
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 this pull request may close these issues.

2 participants