make hamt a go-ipld-prime ipld.Node

[willscott]

• The structure of the ipld.NodePrototype is a bit janky. Would be interested in suggestions on how to make that cleaner.
• This doesn't support the fully life-cycle of NodeBuilder/NodeAssembler, so won't automatically load.
• IPLD methods don't provide an explicit context, so context.Background is used. Ideally, a context on the node would allow for graceful cancellation of in-progress operations.

[anorth]

Thanks for showing what this would look like.

My first reaction here is that it's a lot of noise with little immediate value to the core use case of actors. Since the Filecoin actors code is so security-critical, and with very strict compatibility requirements, our general desire is to minimize any unnecessary code and dependencies. I didn't see a go.mod diff here, but expect you need to add go-ipld-prime which would immediately raise a flag.

This HAMT is, unfortunately, not built with go-ipld-prime to begin with. My hope for integration there is to replace it wholesale with a HAMT natively built with/for go-ipld-prime, when that framework and implementation (this one?) is ready. Along with all the other hand-rolled structures, exchanging one set of code and dependencies for another, better one (rather than including both).

I infer that you wanted this compatibility for some downstream processing. Would it work to locally extend the HAMT struct, instead of modifying it here?

[willscott]

This does not need to depend on go-ipld-prime, just implement the interface defined there for an ipld.Node. This branch is being used in statediff, as a way to create a hamt.Node, and then use it as an ipld.Node in some of the generic ipld code to traverse efficiently through hamts.

[willscott]

longer term, i'll transition that to the in-progress ipld-schema'd hamt, but this works as a branch for ipld tools to use in the short term. I don't think it needs to be merged, but it's useful for me to maintain it as a branch on this repo so that i can pull in the v3 changes as they finalize.

[willscott]

cc @warpfork / @mvdan - I recently pushed the additional code to this for allowing HAMTs to act as reifying ADLs

[BigLep]

2022-04-05 conversation: the potential areas of concern that we need in review:

1. Ensure not affecting performance for standard usecase
2. Don't somehow break compatibility.

[willscott]

This is a complementary code path and will not affect any current use cases. I think the only/primary concern I see here is if we're okay with the extra maintenance burden of the extra code, and what level of test coverage we feel is needed to be comfortable with it staying in sync with the primary implementation over time.

[BigLep]

@willscott: 2022-04-19 conversation notes:

1. @rvagg is good to approve from a correctness standpoint once the code review comments get incorporated BUT
2. ultimately need a repo owner (@Stebalien , @ZenGround0 ?) to approve this given the extra maintenance burden. Utlimately I think someone in Lotus needs to decide whether they're up for supporting this longer term.

This is a good change, but it does increase the amount that someone needs to understand to properly maintain this library.