IPFS rebase to v0.4.18 #1425

cpacia · 2019-01-24T19:47:23Z

This is the IPFS rebase to v0.4.18. It was much more intense than previous rebases as there were a large number of breaking API changes in the IPFS library.

In addition, this is the first attempt at trying to get off of using a fork of go-ipfs and instead use v0.4.18 without modification. Most of the changes we made to our fork of go-ipfs have been implemented as config options throughout much of IPFS allowing us to massively reduce the surface of our changes. The primary one that remains is the blocking startup. From looking at the code they may or may not have made it not block but we wont know for sure until we test it. Which will require completing the remaining TODOs below.

As of this commit the only remaining fork we maintain is go-libp2p-kad-dht however it has been refactored to make the changes very minimal and I've commented every change in the repo with a // OpenBazaar: comment and explanation of the change so it's easy to find what parts have changed.

The ob-go namesys package has also been removed as we do not currently use it and it would have required rebasing the unused blockstack client in order to keep it.

TODO:

The IPFS config file needs to be migrated to the new version. So we need to create a migration to do this and figure out what the new config should look like.
Because they spun go-ipfs-config out into a separate repo we can't put extra data inside their objects like we were doing before. Currently the IPNS config fields of QuerySize and BackUpAPI have been removed and we'll likely need to add them into a separate config object.

Future work:

The IPNS Resolve function inside of ob-go takes in a peer.ID as a parameter rather than a string. This prevents us from using the function to resolve domain names. A small refactor is needed to make it accept a string rather than a peer.ID.
Currently we have some hacky code wrapping the IPNS Resolve that tries the BackUpAPI if the main Resolve function fails to find the record. A much cleaner way to do this would be create a SuperNodeRouting (or some other name) implementation that implements the ValueStore interface. And add it to the tiered routing structure of the IPFS node's Routing field. Then it will try Pubsub, DHT, and SuperNode in that order. An alternative question is if we want to make it try the DHT and SuperNode at the same time and take whichever returns first. This would likely result in a large speedup when browsing the network but the downside is it's likely it would mostly be using the SuperNode results.

This migration migrates the config file and ipfs repo version to the new format used by ipfs. It also makes some small changes to our fields such as moving custom ipns fields to a separate ipnsExtra field and removing the resolvers field.

coveralls · 2019-02-05T17:25:10Z

Coverage increased (+0.7%) to 34.814% when pulling b527ba5 on ipfsrebase into 07b7980 on master.

cpacia · 2019-02-05T20:14:41Z

So this was the first pass at getting off of using a fork of go-ipfs. Unfortunately it didn't work out this time though we are set up well to make it work for the next rebase.

We've patched go-ipfs in several areas to either suit our needs or fix bugs. The following three issues are open in go-ipfs and we will need them to be address before our next rebase if we are to avoid needing to use a fork the next go around as well.

namesys/routing.go imports dht package preventing dht.Quorum() from being used with a custom routing option #5957

First round bootstrap #5953

Disable DNSResolver option #5945

Two further issues remain that we had to patch:

The ipfs team didn't realize anyone was using hashed ed25519 keys and changed the code to only work with inline ed25519 keys which essentially broke our nodes. They have an open issue figure out how to revert the change Peer ID Calculation History And Resolution libp2p/specs#138

For our part, we've made the necessary changes to go-ipfs to get it working with hashed keys. Going forward, if enough people upgrade to our newest version of ob-go then we can migrate everyone to inline keys in a later release. That later release should be backwards compatible with our forthcoming release without requiring patches to go-ipfs though that will depend on ultimately how the go-ipfs decides to handle inline keys.

Because we were using CIDv1 in our offline messaging system we previously had to patch the go-multiaddr /ipfs/ codec to handle CIDv1. This was really a bad decision on our part as the IPFS team never upgraded to CIDv1 and are repurposing the /ipfs/ multiaddr to /p2p/ which is only used to demarcate peer IDs.

So for this rebase we are switching back to CIDv0 but to do so we've had to implement even more patches to go-multiaddr to be both backwards compatible with CIDv1 and forwards compatible with CIDv0. Assuming enough people upgrade to this forthcoming release of ob-go we can probably stop patching go-multiaddr in future releases.

Ultimately, however, if go-ipfs ever switches to CIDv1 it will break our stuff again so it's probably be we define our out protocol and codec and work to migrate over to that.

The QA packages requires that the first node that is spun up be a seed node since it cannot immeadiately bootstrap without any other nodes on the network. The fix is to just treat the first node as a seed while actual using the subsequent nodes that are created.

hoffmabc · 2019-02-06T14:33:30Z

Discovered an issue while testing this PR.

goroutine 8945 [running]:
runtime/debug.Stack(0x49c184b, 0xc000518600, 0x1)
	/usr/local/opt/go/libexec/src/runtime/debug/stack.go:24 +0xa7
runtime/debug.PrintStack()
	/usr/local/opt/go/libexec/src/runtime/debug/stack.go:16 +0x22
github.com/OpenBazaar/openbazaar-go/api.(*jsonAPIHandler).ServeHTTP.func1()
	/Users/brianhoffman/go/src/github.com/OpenBazaar/openbazaar-go/api/jsonapi.go:165 +0x10b
panic(0x527b360, 0x6796190)
	/usr/local/opt/go/libexec/src/runtime/panic.go:513 +0x1b9
github.com/OpenBazaar/openbazaar-go/core.(*OpenBazaarNode).sendMessage(0xc001246ea0, 0xc0024eb260, 0x2e, 0x0, 0x2, 0xc002c1ec30, 0x0, 0x0, 0x0, 0x0, ...)
	/Users/brianhoffman/go/src/github.com/OpenBazaar/openbazaar-go/core/net.go:39 +0x130
github.com/OpenBazaar/openbazaar-go/core.(*OpenBazaarNode).Follow(0xc001246ea0, 0xc0024eb260, 0x2e, 0x0, 0x0)
	/Users/brianhoffman/go/src/github.com/OpenBazaar/openbazaar-go/core/net.go:192 +0x336
github.com/OpenBazaar/openbazaar-go/api.(*jsonAPIHandler).POSTFollow(0xc0004e3e00, 0x58fa420, 0xc000142380, 0xc0014d3100)
	/Users/brianhoffman/go/src/github.com/OpenBazaar/openbazaar-go/api/jsonapi.go:644 +0x10b
github.com/OpenBazaar/openbazaar-go/api.post(0xc0004e3e00, 0xc00093a330, 0xa, 0x58fa420, 0xc000142380, 0xc0014d3100)
	/Users/brianhoffman/go/src/github.com/OpenBazaar/openbazaar-go/api/endpoints.go:31 +0x12cb
github.com/OpenBazaar/openbazaar-go/api.(*jsonAPIHandler).ServeHTTP(0xc0004e3e00, 0x58fa420, 0xc000142380, 0xc0014d3100)
	/Users/brianhoffman/go/src/github.com/OpenBazaar/openbazaar-go/api/jsonapi.go:174 +0x829
net/http.(*ServeMux).ServeHTTP(0xc001237c50, 0x58fa420, 0xc000142380, 0xc0014d3100)
	/usr/local/opt/go/libexec/src/net/http/server.go:2361 +0x127
net/http.serverHandler.ServeHTTP(0xc001262750, 0x58fa420, 0xc000142380, 0xc0014d3100)
	/usr/local/opt/go/libexec/src/net/http/server.go:2741 +0xab
net/http.(*conn).serve(0xc001e103c0, 0x58fbf20, 0xc001241680)
	/usr/local/opt/go/libexec/src/net/http/server.go:1847 +0x646
created by net/http.(*Server).Serve
	/usr/local/opt/go/libexec/src/net/http/server.go:2851 +0x2f5

This was on a Follow.

cmd/restore.go

placer14

Okay, @cpacia. Here's the first pass of thoughts and recommended changes.

vendor/gx/ipfs/QmTRhk7cgjUf2gfQ3p2M9KPECNZEW9XUrmHcFCgog4cPgB/go-libp2p-peer/peer.go

repo/init.go

repo/migrations/Migration018.go

placer14 · 2019-02-06T22:17:42Z

schema/manager_test.go

@@ -96,7 +96,7 @@ func TestMustDefaultConfig(t *testing.T) {
 	if config == nil {
 		t.Error("Expected config to not be empty")
 	}
-	if config.Addresses.Gateway != "/ip4/127.0.0.1/tcp/4002" {
+	if config.Addresses.Gateway[0] != "/ip4/127.0.0.1/tcp/4002" {


Where did this change from a string to a slice?

In go-ipfs.. ipfs-config I believe.

I don't see any migration handling this change. Did I miss something?

core/ipns.go

placer14 · 2019-02-06T22:31:22Z

repo/migrations/Migration012_test.go

-	"slug-4": "zb2rhYFPk5iVCTJYFoGR5gEpzKodhDWu5jESE2yzvWrCou54n",
-	"slug-5": "zb2rhbNjVXhbtKkSXbf6hpGUV2CujPTBx9jWsRJpvzKdgpwj9",
+	"slug-4": "QmXC26R4PNnArmVssrviaA4WGxP1zzmx8y2AiybF6hQpRM",
+	"slug-5": "QmaEUP6zWvZkrWAbVAvcxRiV5Fou8jQnHc4nmarAUVLoQr",


This is a red flag. Having a migration change behavior will cause our data states to diverge depending on when this migration gets run (as part of a prior release or as part of the release this changeset falls into). Is this an issue? If so, how is this divergence getting handled during the runtime? If not, why not?

I don't think there's any way to change it. These hashes are generated inside go-ipfs, so if go-ipfs changes how the hash is calculated (as is the case here) then it will break any tests that hardcode the hashes.

placer14 · 2019-02-06T22:33:31Z

repo/migrations/Migration008_test.go

-		0,                      // purchase read bool
-		0,                      // sale read bool
+		0, // purchase read bool
+		0, // sale read bool


Watching this pattern change back and force is really noisy in our git history as well as for review purposes. Can you find out what env inconsistency could be causing this or take more care to not commit these changes (and then reversals) to git?

It's because we're still using gofmt 10.3. I'd like to upgrade to go 11 if we can.

placer14 · 2019-02-06T22:36:41Z

vendor/gx/ipfs/QmPEpj17FDRpc7K1aArKZp3RsHtzRMKykeK9GVgn4WQGPR/go-ipfs-config/init.go

-				"Access-Control-Allow-Headers": []string{"X-Requested-With", "Range"},
+				"Access-Control-Allow-Origin":  {"*"},
+				"Access-Control-Allow-Methods": {"GET"},
+				"Access-Control-Allow-Headers": {"X-Requested-With", "Range"},


Are you required to change these vendor files to satisfy the linter? Would prefer that non-required edits are not included in these complicated PRs. FYI, in a future version of go, vendors will be checksum-ed and verified...

Appreciate you keeping these changes within a single commit, though. 🤝

Do you know the command to omit the vendor package from gofmt?

I'm pretty sure gofmt should be aware of vendors and should not change other package's formatting.

placer14 · 2019-02-15T04:10:42Z

core/ipns_test.go

+	for i, v := range testVectors {
+		pth := ipnsAPIPathTransform(v, peerID)
+		if pth != expected {
+			t.Errorf("IpnsAPIPathTransform test %d failed. Got %s, expected %s", i, pth, expected)


The test output already includes the testname. Just expected: %s, got: %s is sufficient.

It's printing the number of the test vector that failed.

placer14 · 2019-02-15T04:12:55Z

vendor/gx/ipfs/QmPEpj17FDRpc7K1aArKZp3RsHtzRMKykeK9GVgn4WQGPR/go-ipfs-config/init.go

-				"Access-Control-Allow-Headers": []string{"X-Requested-With", "Range"},
+				"Access-Control-Allow-Origin":  {"*"},
+				"Access-Control-Allow-Methods": {"GET"},
+				"Access-Control-Allow-Headers": {"X-Requested-With", "Range"},


I'm pretty sure gofmt should be aware of vendors and should not change other package's formatting.

placer14

I think this PR also removes a package from vendors: github.com/google/uuid.

drwasho · 2019-02-25T00:52:44Z

Nice work 👏

cpacia added 15 commits January 24, 2019 14:29

First pass IPFS rebase to v0.4.18

d7b3cb1

Remove resolve config loading from mobile package

1e6ea35

Add back libsecp256k1 lib

eb7c8be

Fix routing type assertion in message retreiver

41943e8

Remove dumplicate enum descriptors from go-libp2p-kad-dht

200e74c

Update go-onion-transport with new constructor

d41e263

Add libsecp256k1 back

b842da2

Fix indentity key configuration

3c9bc64

Refactor IPFS pointer package to use IpfsDHT rather than the IpfsNode

e1e3331

Patch go-multiaddr dependency to handle CID in /ipfs/ field

c2ad5aa

Patch secio package handle old style hashed ed25519 keys

7d17eb5

Update DHT package to signal bootstrap complete

cc5c139

Fix compatibility bugs in ipfs library

9aa8e6d

Patch go-multiaddr to handle backwards compatible cids

e72c46f

Patch multiaddr codec to handle alternative serializations

d41059e

cpacia force-pushed the ipfsrebase branch from 762e661 to d41059e Compare February 4, 2019 18:48

cpacia added 7 commits February 4, 2019 13:51

Move identity set to after fsrepo.Init

6710f07

Add migration018

ddaa673

This migration migrates the config file and ipfs repo version to the new format used by ipfs. It also makes some small changes to our fields such as moving custom ipns fields to a separate ipnsExtra field and removing the resolvers field.

Wire up dht quorum and fallback API config values

0bc8157

Fix lint

25a9591

Add github.com/google/uuid to vendor

1b5528d

Update jsonapi test data with new hash format

81d2470

Update ipfs add function to also pin

a431c4e

cpacia force-pushed the ipfsrebase branch from 3374b58 to a431c4e Compare February 5, 2019 16:51

Update hash format in migration and storage tests

409e652

placer14 mentioned this pull request Feb 5, 2019

Reapply "Minor cleanups" #1434

Merged

Gofmt project

7a83231

placer14 reviewed Feb 6, 2019

View reviewed changes

cmd/restore.go Show resolved Hide resolved

placer14 suggested changes Feb 6, 2019

View reviewed changes

cpacia added 3 commits February 8, 2019 11:01

Handle json marshalling error in Migration018

3ce3377

Add test for ipns path transform

f181a99

Move cleanIdentityFromConfig to schema and add test

4bc3728

placer14 reviewed Feb 15, 2019

View reviewed changes

placer14 suggested changes Feb 15, 2019

View reviewed changes

hoffmabc force-pushed the ipfsrebase branch from 4bc3728 to 4f3d6e2 Compare February 16, 2019 21:12

cpacia added 6 commits February 19, 2019 10:19

Fix typo in gateway url

b7b287e

Fix bugs in tor transport

07f029f

Add blockingStartupMiddleware to select endpoints

1094881

Merge branch 'master' into ipfsrebase

f8d2231

Set DHT in test node and close bootstrapchan

53396e8

Add CID compatibility test

b527ba5

cpacia force-pushed the ipfsrebase branch from 6123507 to b527ba5 Compare February 20, 2019 17:20

cpacia merged commit 7a6dc86 into master Feb 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IPFS rebase to v0.4.18 #1425

IPFS rebase to v0.4.18 #1425

cpacia commented Jan 24, 2019 •

edited

Loading

coveralls commented Feb 5, 2019 •

edited

Loading

cpacia commented Feb 5, 2019

hoffmabc commented Feb 6, 2019

placer14 left a comment

placer14 Feb 6, 2019

cpacia Feb 8, 2019

placer14 Feb 15, 2019

placer14 Feb 6, 2019

cpacia Feb 19, 2019

placer14 Feb 6, 2019

cpacia Feb 8, 2019

placer14 Feb 6, 2019

placer14 Feb 6, 2019

cpacia Feb 8, 2019

placer14 Feb 15, 2019

placer14 Feb 15, 2019

cpacia Feb 19, 2019

placer14 Feb 15, 2019

placer14 left a comment

drwasho commented Feb 25, 2019

IPFS rebase to v0.4.18 #1425

IPFS rebase to v0.4.18 #1425

Conversation

cpacia commented Jan 24, 2019 • edited Loading

coveralls commented Feb 5, 2019 • edited Loading

cpacia commented Feb 5, 2019

hoffmabc commented Feb 6, 2019

placer14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

placer14 left a comment

Choose a reason for hiding this comment

drwasho commented Feb 25, 2019

cpacia commented Jan 24, 2019 •

edited

Loading

coveralls commented Feb 5, 2019 •

edited

Loading