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.

Sign up for GitHub

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

Jump to bottom

update and use upstream deps #1270

Closed
wants to merge 0 commits into from
Closed

update and use upstream deps #1270

wants to merge 0 commits into from

Conversation

Stebalien
Copy link
Member

Updates:

  • go-libp2p (re-disables the relay as it's now enabled by default)
  • go-ipfs-cmds (yay)
  • go-ipld-cbor

fixes #643

Cids by value or by pointer: In the past, we always passed around *cid.Cid (a pointer to a struct). In go-ipfs, we now pass around cid.Cid (effectively a newtype around a string). This:

  1. Is significantly more efficient.
  2. Can be used as a map key.
  3. Can be compared with ==
  4. Means we don't use by-value CIDs in cbor and by-pointer CIDs everywhere else.

However, it introduces a tricky edge-case: nil CIDs. To handle this, we use cid.Undef (or cid.Cid{}) and have defined a myCid.Defined() method to tell if the CID is valid or not.

Unfortunately, this has lead to a rather annoying edge-case concerning refmt and ipld-cbor: refmt provides no way to decode null as cid.Cid{} or encode cid.Cid{} as null. The current workaround (in this patch) is to:

  1. Flag nullable CID fields with refmt:",omitempty".
  2. Never put null where a CID is.

The alternatives are:

  1. Change the refmt library. Personally, I'd love to see this happen (some form of "null = zero value" option.
  2. Use *cid.Cid in structs where the CID is nullable.

Please carefully look over this PR, it contains quite a few changes. I've broken it up into logical commits (sticking all gx updates in the first once) so it should be reviewable.

Please also tell me if there are any changes made to upstream libraries in forks that I may be unaware of. I believe ipfs/go-ipld-cbor is now on-par with filecoin-project/go-ipld-cbor but I may be wrong.

@Stebalien Stebalien requested review from whyrusleeping, phritz and dignifiedquire and removed request for whyrusleeping November 2, 2018 15:56
Stebalien
Stebalien commented Nov 2, 2018
View reviewed changes
actor/builtin/storagemarket/storagemarket.go Outdated
@@ -72,7 +72,7 @@ type Actor struct{}

// State is the storage market's storage.
type State struct {
Miners cid.Cid
Miners cid.Cid `refmt:",omitempty"`
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the real design question here. I'm really quite unhappy with this "omitempty" dance but want to get some opinions before I try pushing for a change in refmt.

Copy link
Contributor

Choose a reason for hiding this comment

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

@acruikshank can this ever be nil?

Copy link
Member

Choose a reason for hiding this comment

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

no, this should never be nil. At worst, its the cid of the empty HAMT

Copy link
Contributor

Choose a reason for hiding this comment

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

@phritz @whyrusleeping This value is nil when it's initialized. Nil signals that there's currently no block in storage for the Cid. It could be implemented differently if we really want to avoid nils.

phritz
phritz approved these changes Nov 3, 2018
View reviewed changes
Copy link
Contributor

@phritz phritz left a comment

Choose a reason for hiding this comment

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

  • thanks for doing this
  • yes we should fix refmt, current state is just going to cause bugs and confusion
  • can you please send an email to filevcoin-dev highlighting the cid changes?

api/actor.go Outdated
@@ -12,11 +12,11 @@ import (
type ActorView struct {
ActorType string `json:"actorType"`
Address string `json:"address"`
Code *cid.Cid `json:"code"`
Code cid.Cid `json:"code"`
Copy link
Contributor

Choose a reason for hiding this comment

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

pretty sure code can be nil

Copy link
Member

Choose a reason for hiding this comment

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

no. Code can't be nil

Copy link
Contributor

Choose a reason for hiding this comment

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

@whyrusleeping @Stebalien yes it can: https://github.com/filecoin-project/go-filecoin/blob/34875d0636c784fe6fa29d745f0484e228f580bd/consensus/processor.go#L340. Recall we added actor upgrades to handle sends to predictable but uninitialized addresses.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Stebalien @whyrusleeping

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm waiting on decisions in refmt, currently. For now, we could also just use *cid.Cid here and sidestep the issue (it's just a little annoying to deal with both cid.Cid and *cid.Cid).

Nit: We should add a test case that covers this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah. Actually, ActorView is just for the user. As far as I can tell, it's never turned into an IPLD object. However, I can make it omitempty anyways (can't hurt).

commands/client.go Outdated
@@ -72,9 +72,9 @@ var clientImportDataCmd = &cmds.Command{

re.Emit(out.Cid()) // nolint: errcheck
},
Type: cid.Cid{},
Type: cid.Undef,
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

(fixed)

actor/builtin/storagemarket/storagemarket.go Outdated
@@ -72,7 +72,7 @@ type Actor struct{}

// State is the storage market's storage.
type State struct {
Miners cid.Cid
Miners cid.Cid `refmt:",omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@acruikshank can this ever be nil?

@Stebalien
Copy link
Member Author

Rebased. If I don't keep this up-to-date, it'll be really painful.

@Stebalien
Copy link
Member Author

Refmt issue here: polydawn/refmt#42

@Stebalien
Copy link
Member Author

So, it turns out I was technically incorrect:

refmt provides no way to decode null as cid.Cid{}

refmt will decode null as cid.Cid{}, it just won't round-trip it back to null when encoding.

@Stebalien
Copy link
Member Author

Status:

This is blocked on polydawn/refmt#42. Instead of waiting on that, I'd like to propose two alternatives:

  1. Stick with omitempty. Really, we should be omitting fields with "null" CIDs.
  2. Use *cid.Cid for "nullable" CID fields.

Thoughts?

@Stebalien Stebalien force-pushed the feat/update-deps branch 3 times, most recently from f0c5e65 to e15bb7b Compare November 21, 2018 00:05
@lkowalick lkowalick closed this Nov 21, 2018
@Stebalien Stebalien deleted the feat/update-deps branch December 6, 2018 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acruikshank acruikshank acruikshank left review comments

@phritz phritz phritz approved these changes

@dignifiedquire dignifiedquire Awaiting requested review from dignifiedquire

@whyrusleeping whyrusleeping Awaiting requested review from whyrusleeping

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

reconciling fileccoin/ipfs go-ipld-cbor incompatibility
5 participants
@Stebalien @phritz @acruikshank @whyrusleeping @lkowalick