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

feat(actor): update go-hamt-ipld library to latest #3411

Merged
merged 4 commits into from
Sep 16, 2019

Conversation

hannahhoward
Copy link
Contributor

@hannahhoward hannahhoward commented Sep 11, 2019

Goals

Update Actor code to the latest go-hamt-ipld library, which supports fast CBOR decode/encode and also supports alternate bitwidths on HAMT.

Implementation

  • The main change from the previous version of go-ipld-hamt is the deferral of deserializing actual values stored in the HAMT. This means that when you lookup values, you need to pass a pointer to a value where raw CBOR is decoded into. Also, when iterating the values in a HAMT, it's neccesary to use a callback and a type so that deserialization happens properly.
  • go-hamt-ipld also now includes cbor-gen, which allows you to define fast-path deserialization/serialization for specific types. For the moment, it only applies to HAMT stuff but we could generate fast-path code for actor types as well.
  • This does not actually alter the bitwidth for the HAMT fanout -- that
    will be a future PR.

For discussion

I've changed a couple methods that didn't have a lot of individual test coverage to begin with -- should that be added to get this merged?

See comment on lines below for the MakeGenisisFunc function -- I don't understand how this didn't fail the waiter_test in plumbing/msg before?

@@ -169,6 +169,9 @@ func MakeGenesisFunc(opts ...GenOption) GenesisInitFunc {
if err = s.Commit(scid, a.Head); err != nil {
return nil, err
}
if err := st.SetActor(ctx, addr, a); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of particular note: I do not understand how this was working before -- the head gets committed, which updates the actor, but it isn't rewritten to the hamt -- it makes me think this was only working cause of in some memory caching? Not sure!

Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably the problem is that the actor's Head is not getting updated in the state tree without this line. If this is the case, is line 160 still necessary? If so, we need a comment explaining why we're setting the actor twice.

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

Thanks. I don't like the deps on personal repos, but this didn't make the situation any worse, and we can address it later.

@@ -177,7 +177,7 @@ func (pb *Actor) CreateChannel(vmctx exec.VMContext, target address.Address, eol

err := withPayerChannels(ctx, storage, payerAddress, func(byChannelID exec.Lookup) error {
// check to see if payment channel is duplicate
_, err := byChannelID.Find(ctx, channelID.KeyString())
err := byChannelID.Find(ctx, channelID.KeyString(), &PaymentChannel{})
Copy link
Member

Choose a reason for hiding this comment

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

In storagemarket.go you used nil for an unwanted out parameter. Do something consistent (nil seems fine if supported).

@anorth
Copy link
Member

anorth commented Sep 13, 2019

@hannahhoward your editor seems to be inserting unnecessary package aliases. I'd appreciate you figuring out how to avoid that.

Copy link
Contributor

@acruikshank acruikshank left a comment

Choose a reason for hiding this comment

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

This looks good. I have a couple question that may prompt minor modifications.

@@ -169,6 +169,9 @@ func MakeGenesisFunc(opts ...GenOption) GenesisInitFunc {
if err = s.Commit(scid, a.Head); err != nil {
return nil, err
}
if err := st.SetActor(ctx, addr, a); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably the problem is that the actor's Head is not getting updated in the state tree without this line. If this is the case, is line 160 still necessary? If so, we need a comment explaining why we're setting the actor twice.

return &act, nil
}

func hackTransferObject(from, to interface{}) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

nd, err = cbor.Decode(bytes, types.DefaultHashFunction, -1)
} else if raw, ok := v.([]byte); ok {
nd, err = cbor.Decode(raw, types.DefaultHashFunction, -1)
} else if cm, ok := v.(cbg.CBORMarshaler); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we putting CBORMarshalers into storage? Is this new functionality? If so, and we aren't yet using it, we should wait to introduce this until it is exercised by code (and tests).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so a CBORMarshaler just means an object that conforms to that interface, meaning it has methods to fast path it's own serialization / deserialization -- this covers all hamt nodes, and in fact, they need to be fastpathed to encode properly. WrapObject won't work here because of the cbg.Deferred object. The actual solution is that go-ipld-cbor should check for fast path in its own calls to marshal/unmarshal, which there is an unmerged PR for: ipfs/go-ipld-cbor#64 . Absent that getting merged, this is neccesary for a couple direct calls to storage.Put

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will add a comment to that effect.

Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

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

@hannahhoward I will defer to whatever AC & AN suggest here. My feedback is primarily nits: combine if err :=, no unnecessary import renames, and a slight preference for select over else if in vm/storage.go.

Updates to the latest go-hamt-ipld library, which supports fast CBOR decode/encode and also supports
alternate bitwidths on HAMT. This does not actually alter the bitwidth for the HAMT fanout -- that
will be a future PR
hamt behavior was relying on actors being in memory
@hannahhoward
Copy link
Contributor Author

hannahhoward commented Sep 16, 2019

@hannahhoward your editor seems to be inserting unnecessary package aliases. I'd appreciate you figuring out how to avoid that.

@anorth Hey so I finally tracked this down to not my editor but goimports, which does autoimporting, and specifically, this issue: golang/go#28428 & https://go-review.googlesource.com/c/tools/+/152000

They actually chose to add this feature and I honestly understand their reasoning -- the thought is even if the underlying package name is correct, if there's a mismatch with the filename there's no way to determine that it is without digging into the underlying source code of the file, so having the rename allows the imports of the file to be validated just by looking at the file itself.

Though also I am curious if I am the only one who uses goimports and if others have had the same issue.

Anyway, I only say this cause goimports is a pretty standard go tool, though I guess I am the only one who uses it on this project. But I'll turn it off and just do manual imports.

- remove unneccesary imports - delete set line - add comment
@codecov-io
Copy link

Codecov Report

Merging #3411 into master will decrease coverage by 22%.
The diff coverage is 15%.

@@           Coverage Diff            @@
##           master   #3411     +/-   ##
========================================
- Coverage      48%     26%    -23%     
========================================
  Files         230     230             
  Lines       14362   14296     -66     
========================================
- Hits         6959    3765   -3194     
- Misses       6375    9900   +3525     
+ Partials     1028     631    -397

@hannahhoward hannahhoward merged commit d756bd4 into master Sep 16, 2019
@ZenGround0 ZenGround0 deleted the feat/use-updated-ipld-hamt branch October 29, 2019 14:52
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.

5 participants