-
Notifications
You must be signed in to change notification settings - Fork 283
Conversation
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.
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. Disable DNSResolver option #5945 Two further issues remain that we had to patch:
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.
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.
Discovered an issue while testing this PR.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, @cpacia. Here's the first pass of thoughts and recommended changes.
vendor/gx/ipfs/QmTRhk7cgjUf2gfQ3p2M9KPECNZEW9XUrmHcFCgog4cPgB/go-libp2p-peer/peer.go
Outdated
Show resolved
Hide resolved
@@ -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" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did this change from a string to a slice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In go-ipfs.. ipfs-config I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any migration handling this change. Did I miss something?
"slug-4": "zb2rhYFPk5iVCTJYFoGR5gEpzKodhDWu5jESE2yzvWrCou54n", | ||
"slug-5": "zb2rhbNjVXhbtKkSXbf6hpGUV2CujPTBx9jWsRJpvzKdgpwj9", | ||
"slug-4": "QmXC26R4PNnArmVssrviaA4WGxP1zzmx8y2AiybF6hQpRM", | ||
"slug-5": "QmaEUP6zWvZkrWAbVAvcxRiV5Fou8jQnHc4nmarAUVLoQr", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
repo/migrations/Migration008_test.go
Outdated
0, // purchase read bool | ||
0, // sale read bool | ||
0, // purchase read bool | ||
0, // sale read bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because we're still using gofmt 10.3. I'd like to upgrade to go 11 if we can.
"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"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate you keeping these changes within a single commit, though. 🤝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know the command to omit the vendor package from gofmt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure gofmt
should be aware of vendors and should not change other package's formatting.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test output already includes the testname. Just expected: %s, got: %s
is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's printing the number of the test vector that failed.
"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"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure gofmt
should be aware of vendors and should not change other package's formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR also removes a package from vendors: github.com/google/uuid
.
Nice work 👏 |
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:
IPNS
config fields ofQuerySize
andBackUpAPI
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 apeer.ID
.Currently we have some hacky code wrapping the IPNS Resolve that tries the
BackUpAPI
if the mainResolve
function fails to find the record. A much cleaner way to do this would be create aSuperNodeRouting
(or some other name) implementation that implements theValueStore
interface. And add it to the tiered routing structure of the IPFS node'sRouting
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.