go-merkledag duplicates and global variables

[aschmahmann]

Background

1. There are currently two packages of the merkledag library. There is the one here in boxo and go-merkledag.
2. There is a library https://github.com/ipfs/go-ipld-legacy that is used to create nodes that satisfy the go-ipld-format and go-ipld-prime interfaces
3. That library uses a global variable to manage specific types of conversions, such as getting a DagPB specific type rather than a generic untyped node. https://github.com/ipfs/go-ipld-legacy/blob/219032a33b3037ac7754ea9f04c4b1ead533f364/decode.go#L32
4. There exist places in the code that do type checks against specific types such as

   boxo/ipld/unixfs/file/unixfile.go

   Line 155 in 8059f18

   case *dag.ProtoNode:

Bug

These interactions collectively mean that depending on whether go-merkledag gets initialized before or after boxo/ipld/merkledag you may/may not get an error if you call boxo/ipld/merkledag/DagService.Get and pass the result into boxo/ipld/unixfs/file/NewUnixFSFile.

Proposed fixes

There are obviously lots of problems here that could resolve this particular issue given the many unfortunate practices that have led us here (two versions of similar code, using global variables to manage state, doing explicit type checks, etc.).

Probably the easiest is to copy go-ipld-legacy into boxo. At that point the entry paths into go-ipld-legacy will be a little more obvious/contained and we can figure out the best way to go from there.

Another option that might not be too bad would be to remove the global variables from go-ipld-legacy, and force an instance to be instantiated with a converter registry, which would likely be a breaking change. That repo doesn't appear to be highly utilized explicitly (as opposed to via merkledag) so it probably wouldn't be too rough on consumers. That being said it's certainly more disruptive than a copy.

Open to better suggestions if folks have them.

Tracking steps

Stage the changes:

• rename dagpb protobuf to merkledag.v1 #323
• rename unixfs protobuf package to unixfs.v1 #318
• feat: remove ipld legacy and format globals in ipld/merkledag #322
• Feat: reduce globals #324
• feat: remove block decoding global registry go-ipld-format#80

Validate the changes with consumers::

• Kubo: chore: update boxo to version with fewer globals kubo#9907
• Lotus: chore: migrate to boxo filecoin-project/lotus#10921
• Boost: update boxo filecoin-project/boost#1492
• go-fil-markets: chore: migrate to boxo filecoin-project/go-fil-markets#792
• ipfs-cluster: feat: update go-ipld-format and boxo to remove globals ipfs-cluster/ipfs-cluster#1936 (along with related dependencies feat: update go-ipld-format and boxo to remove globals go-ds-crdt#201 and feat: update go-ipld-format and boxo to remove globals hsanjuan/ipfs-lite#296)

Disruption phase:

• (go-ipld-format) Merge feat: remove block decoding global registry go-ipld-format#80
• (boxo) Merge Feat: reduce globals #324
• (Kubo) Merge chore: update boxo to version with fewer globals kubo#9907
• Remove boxo/car (will be tracked in Handle relationship between boxo and go-car and go-merkledag #218)

[BigLep]

2023-05-18 maintainer conversation:

1. This needs to get completed before we can back out boxo/car.
2. Options:

• First one is easy to execute on
• Second one is ideal as removing the global variables

3. @aschmahmann will reach out to IPLD folks in this issue and Slack. Fall back if we hit agreement or timeline challenges is option 1.

[willscott]

The change to resolve this in ipld-prime wasn't too bad - the use of registry and a 'DefaultRegistry' meant that in the cases where there were conflicts, a specific registry could be specified to avoid the use of the implicit global.

[rvagg]

My personal preference is to not see more shared dependencies puled in to the boxo vortex, it continues to cause problems. A new example to highlight this is an inability to update a transitive dependency of the non-boxo go-merkledag that's been tagged with a security vulnerability, so now that go-merkledag gets the pleasure of being flagged by github as having security problems (even though it doesn't), because of the dependency tangle: ipfs/go-merkledag#103. I'd prefer people to not rely on go-merkledag at all, but this is likely to just push people to reach for the boxo form, which may help boxo adoption metrics but it doesn't help migrate people off legacy abstractions.

Anyway, that aside, two other thoughts:

1. IIRC go-ipld-legacy was made for the major go-merkledag refactor where it was essentially turned into an API bridge between the go-merkledag abstractions and go-ipld-prime (because entirely removing it is actually pretty traumatic, I'll acknowledge that point). I'm pretty sure that breaking changes in go-ipld-legacy aren't going to hurt many, or any other consumers so that seems like fair game.

2. How hard would it be to paper over the problems through the stack? Or are you concerned there may be problems hiding that won't be picked up by tests? e.g. in the one you linked, it looks pretty minimal, newUnixfsDir doesn't seem to actually need a ProtoNode internally so its argument type could be changed to a Node so the remaining use is dn.Data() so the switch case could maybe become something as simple as case interface{ Data() []byte }:.

[willscott]

This is also leading to issues with compatibility between caboose and bifrost-gateway somehow

[BigLep]

This is also leading to issues with compatibility between caboose and bifrost-gateway somehow

Eek - not good. This was already on the critical to solve list. Our original plan was to handle after the inflight items complete next week (per #196 (comment) ). But if it is now blocking bifrost-gateway gateways, we can context switch to handle sooner. @willscott : can this be evaluated when everyone's back on Tuesday, 2023-05-30?

[willscott]

Yep 👍

[BigLep]

Next steps per 2023-05-30 maintainer conversation (updated later that day):

• Land rename unixfs protobuf package to unixfs.v1 #318
• Land rename dagpb protobuf to merkledag.v1 #323
• Land Update to v0.2.0 go-ipld-legacy#13 and bubble to Boxo (feat: remove ipld legacy and format globals in ipld/merkledag #322)
• Land feat: remove block decoding global registry go-ipld-format#80 and bubble to Boxo (feat: remove ipld legacy and format globals in ipld/merkledag #322)

[willscott]

ipfs/go-merkledag#104 is the bubble of ipfs/go-ipld-legacy#13 on the go-merkledag side, and should be reflected in the one in boxo

[aschmahmann]

@willscott @rvagg realizing that the go-ipld-prime globals are in too many places right now and I don't immediately need go-ipld-legacy v0.2.1 so going to make dealing with that a follow-up issue. It'll likely touch the merkledag repo here and if you'd like to do the same in go-merkledag that's 👍. I'll tag you as an FYI way once there's there's anything to look at although IMO it's not that high priority at the moment so could take some time.

[BigLep]

2023-06-08 maintainer conversation:
@aschmahmann is providing an update in FIL slack to Boost/Bedrock folks.
We are going to call this issue closed once:

1. Lotus PR is merged: chore: migrate to boxo filecoin-project/lotus#10921
2. ipfs-cluster PRs are out of draft and ready for bubbling

[BigLep]

They've already been linked, but for visibility, this has been impacting Outercore Engineering:
application-research/delta-edge#45
application-research/delta#136

[aschmahmann]

@hannahhoward has said she will take care of pushing through the go-fil-markets, lotus and boost PRs. They should be good to go from the perspective of this issue (i.e. just switching from commit hashes to releases), but there may be other changes they want to make before hitting the merge button. (cc @jlogelin since you'll likely be blocked on the lotus PR).

I've updated the ipfs-cluster dependencies and ipfs-cluster PRs. They're waiting for when @hsanjuan has time to review.

[BigLep]

I'm closing this issue since all the issues/PRs were handled and merged. The one exception is ipfs-cluster, but there are open PRs for that work.

Thanks @aschmahmann and others for helping detangle here. Not fun but obviously important.