Update to latest go-datastore. Remove thirdparty/datastore2

[hsanjuan]

There are 3 commits here:

• Bubbling up new go-datastore. It includes some datastores that were extracted from @whyrusleeping repos which means it reduces the dependency tree a bit. The main change here, other than the updated deps, is the ¨go-libp2p-record not signing records anymore.¨ change, which requires some fixes in some tests in namesys.
• Use delayed datastore in exchange, rather than thirdparty/datastore2. This datastore was added to go-datastore too.
• Replace ds2.ThreadSafeCloserMapDatastore() with syncds.MutexWrap(datastore.NewMapDatastore()). I am not sure what was the point of the Closer part, but syncds already implements the ThreadSafe part. If I screwed something, it will hopefully show up in the tests (this only affects test code).

[hsanjuan]

I think I'm too tired to figure out why this test fails:

--- FAIL: TestRepublish (5.07s)
	repub_test.go:93: Could not resolve name.

I will try again tomorrow but if anyone has a tip in the meantime, it's welcome.

[hsanjuan]

I have spent considerable time and haven't found why TestRepublish has started failing. However, in the last commit I have fixed it by re-publishing the already-expired record after creating the republisher (with very short EOL though). It seems then it is able to re-publish.

It would seem that now the expired record goes away from the datastore and cannot be republished once expired. This did not happen before. After looking a lot around I'm not sure where this behavior change comes from, or if it's a feature or a bugfix. @Stebalien any idea? Reverting the datastore2 removal does not help though, so this must probably come from changes in one of the updated deps?

This makes sharness tests fail too.

[whyrusleeping]

@magik6k could you take a look at this one for @hsanjuan ?

[magik6k]

So the record is being removed by kad-dht when a get message arrives to one of the peers (I assume to the peer who published, it's not easy to check with debugger), it then gets to checking if the record is still valid:

Which is this bit of code:

	var recordIsBad bool
	recvtime, err := u.ParseRFC3339(rec.GetTimeReceived())
	if err != nil {
		log.Info("either no receive time set on record, or it was invalid: ", err)
		recordIsBad = true
	}

	if time.Now().Sub(recvtime) > MaxRecordAge {
		log.Debug("old record found, tossing.")
		recordIsBad = true
	}

	// NOTE: We do not verify the record here beyond checking these timestamps.
	// we put the burden of checking the records on the requester as checking a record
	// may be computationally expensive

	if recordIsBad {
		err := dht.datastore.Delete(dskey)
		if err != nil {
			log.Error("Failed to delete bad record from datastore: ", err)
		}

		return nil, nil // can treat this as not having the record at all
	}

The interesting thing is that looking at git blame, it has been there since 2 years. I need to dig a bit deeper to find why/how it did work before.

[magik6k]

It's caused by the removal of a check in this commit

The check was in func (dht *IpfsDHT) checkLocalDatastore in go-libp2p-kad-dht

	// if its our record, dont bother checking the times on it
	if peer.ID(rec.GetAuthor()) == dht.self {
		return rec, nil
	}

Without it we will remove our own records if they are expired, which wasn't the case before. (btw, did this check work with keystore keys?)

I think it has to do with the IPRS refactoring, it's not merged yet though, so when the breaking changes got pulled in here, things obviously broke.

cc @dirkmc

[dirkmc]

That change was made because we removed the signature and author fields from the record, so it's no longer possible to check who the creator of the record was: #4613

[hsanjuan]

Thanks @magik6k !

My question here: is losing our own expired records an unintended behavior or not ? @dirkmc

If unintended: how is this going to be addressed (given that Author field and signature is gone)
If not: I can try to fix/remove sharness tests affected by this.

btw, did this check work with keystore keys?

I don't fully understand (maybe I'm missing context), but the republisher in this check does not carry a keystore (it's nil). Is there anything I can do there?

[dirkmc]

It was not my intention when I made this change to lose our own expired records. I'm not really sure what the correct answer is here. @whyrusleeping @vyzo @Stebalien do you have any thoughts?

[vyzo]

that was certainly not anticipated... we debated deleterious effect of the removal of signatures, but didn't see that one coming!

[whyrusleeping]

Hrm... yeah. We need to retain expired records for records that belong to keys we own. This is so that the republisher can continue functioning even in the absence of a still valid record (what if your node goes offline for a few days). Maybe we should store them in some other way? like a separate place in the datastore for "things we publish". The code that @dirkmc removed was a pretty gnarly hack in the first place.

[hsanjuan]

This seems to overpass the purpose of this PR (and blocks me). Is it ok if I:

a) Open issues for this problem to make sure that it's not forgotten
b) Gx "special" libp2p-kad-dht before the problematic changes (in the 3.x.x line instead of the 4.x.x) but with the new go-datastore dep (which is what interests me).
c) Bubble that up in here so that we can merge this one safely.

?

[whyrusleeping]

@hsanjuan okay, go for it. Lets work on getting the records issue fixed separately.

[dirkmc]

I'm happy to work on fixing the republisher. Let's discuss in the separate issue

[dirkmc]

Issue created here: #4749

[hsanjuan]

This is ready on my side.

[hsanjuan]

Who would be so kind to review this? @whyrusleeping @Stebalien @magik6k 🥇

I guess it will need to sit until stable 0.4.14 is out? Thanks in advance anyway

[Kubuxu]

@hsanjuan it will for sure wait until 0.4.15 dev cycle starts. Merge window for 0.4.14 is closed AFAIK.

[magik6k]

go-ipfs-blockstore appears to have wrong hash somewhere - see the CI fail - https://ci.ipfs.team/blue/organizations/jenkins/go-ipfs/detail/PR-4742/13/pipeline

[hsanjuan]

I need to rebase

[hsanjuan]

Rebased. I think things should be good now.