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

chore: migrate to boxo #10921

Merged
merged 7 commits into from Jun 22, 2023
Merged

chore: migrate to boxo #10921

merged 7 commits into from Jun 22, 2023

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented May 25, 2023

This migrates everything except the go-car librairy: ipfs/boxo#218 (comment)

I didn't migrated everything in the previous release because all the boxo code wasn't compatible with the go-ipld-prime one due to a an in flight (/ aftermath) revert of github.com/ipfs/go-block-format. go-block-format has been unmigrated since slight bellow absolutely everything depends on it that would have required everything to be moved on boxo or everything to optin into using boxo which were all deal breakers for different groups.

This worked fine because lotus's codebase could live hapely on the first multirepo setup however boost is now trying to use boxo's code with lotus's (still on multirepo) setup: https://filecoinproject.slack.com/archives/C03AQ3QAUG1/p1685022344779649

The alternative would be for boost to write shim types which just forward calls and return with the different interface definitions.

Btw why is that an issue in the first place is because unlike what go's duck typing model suggest interfaces are not transparent golang/go#58112, interfaces are strongly typed but they have implicit narrowing. The issue is if you return an interface from an interface Go does not have a function definition to insert the implicit conversion thus instead the type checker complains you are not returning the right type.

Stubbing types were reverted ipfs/boxo#218 (comment)

Last time I only migrated go-bitswap to boxo/bitswap because of the security issues and because we never had the interface return an interface problem (we had concrete wrappers where the implicit conversion took place).

Depends on filecoin-project/go-fil-markets#792 (need a replace rule removal as well as a bump in go.mod).

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@Jorropo Jorropo requested a review from a team as a code owner May 25, 2023 14:44
@Jorropo
Copy link
Contributor Author

Jorropo commented May 25, 2023

DO NOT MERGE UNTIL ipfs/boxo#291 is fixed (and this PR is bumped to a version of boxo or go-car where this is fixed)

@Jorropo Jorropo marked this pull request as draft May 25, 2023 15:11
@aschmahmann aschmahmann force-pushed the boxo2 branch 4 times, most recently from bb21d2e to db09914 Compare June 1, 2023 18:47
@aschmahmann aschmahmann force-pushed the boxo2 branch 3 times, most recently from c8f3198 to b824e50 Compare June 2, 2023 14:54
cids := prepared.Cids()
for i, c := range cids {
blk, err := bs.Get(ctx, c)
require.NoError(t, err)

nd, err := ipld.Decode(blk)
nd, err := ipldDecoder.DecodeNode(ctx, blk)
Copy link
Contributor

Choose a reason for hiding this comment

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

Using a global registry here seems unintentional/was easy. This mostly swaps which globals you're using (and which codec implementations), but if you knew the set you wanted to support this could also be handled explicitly (e.g. adding support for dag-pb, raw, dag-cbor).

@aschmahmann aschmahmann force-pushed the boxo2 branch 2 times, most recently from f4892d4 to f8be523 Compare June 2, 2023 16:52
Comment on lines 393 to 405
dserv := dag.NewDAGService(bserv)
nd, err := dserv.Get(ctx, ch.Roots[0])
require.NoError(dh.t, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

This works because we've moved the global decoders into the dagservice. If you actually wanted to go from a block to a go-ipld-format node though things are a bit annoying since you've got to go through go-ipld-legacy to register codecs.

It's not clear that this matters, but does this indicate we want either of:

Somewhere to easily reference the globals used by boxo/merkledag:

        d: = ipldlegacy.NewDecoder()
	d.RegisterCodec(cid.DagProtobuf, dagpb.Type.PBNode, ProtoNodeConverter)
	d.RegisterCodec(cid.Raw, basicnode.Prototype.Bytes, RawNodeConverter)

Or instead of deleting the registry code from go-ipld-format instead expose it as a non-global (e.g. build your own registry).


In the case here it happens to make more sense to load the node via the dagservice since you're passing it through to NewUnixfsFile which needs one anyway, however if you used the wrong dag-pb codec (e.g. go-codec-dapb wrapped in a LegacyNode) you'd end up with runtime breakages in NewUnixfsFile.

@aschmahmann
Copy link
Contributor

Update: Plan is to revert the last two changes that switch to the go-ipld-format tests given that it looks like test-itest-deals_anycid and test-itest-ccupgrade are both flaky, unless the maintainers would prefer the go-ipld-format codecs to the go-ipld-prime ones (note: this is just in tests and there are no traversals or anything, just block decoding).

go.mod Outdated
@@ -2,12 +2,14 @@ module github.com/filecoin-project/lotus

go 1.19

replace github.com/filecoin-project/go-fil-markets => github.com/Jorropo/go-fil-markets v1.20.3-0.20230601173540-5e8a152f9466 // https://github.com/filecoin-project/go-fil-markets/pull/792
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: just a reminder to remember to remove before merge

@BigLep BigLep marked this pull request as ready for review June 6, 2023 16:41
@aschmahmann aschmahmann force-pushed the boxo2 branch 2 times, most recently from 500ad82 to 64ea7cc Compare June 6, 2023 17:42
@jennijuju
Copy link
Member

I appreciate the effort from IP stewards do this work for lotus - however, im still missing some context. Can the authors suggest a testing plan for this PR to be accepted?

@hannahhoward
Copy link
Contributor

I have updated to latest boxo (v0.10.0) as well as resolved deps updates for go-car. This now appears to pass tests. the only block on merge -- up to lotus team -- is the repo now includes a Kubo dependency for an untagged version of Kubo.

@aschmahmann when is the next tagged Kubo version out? Is there anyway we can get the former go-ipfs-http-api functionality without a Kubo dependency? (seems like an unfortunate lockstep to introduce)

@hannahhoward
Copy link
Contributor

@jennijuju these changes are ultimately targeting Boost -- there are critical upgrades in Boxo that support features for booster-http needed for the broader ecosystem. See: filecoin-project/boost#1389

Unfortunately, we still seem to have a rats nest of dependent software which has to go through you guys.

In terms of testing, while boxo is a significant refactor, it's also not a huge amount of underlying technology change for now (again, the critical updates mostly will affect booster-http). As for the origins and rationale of the refactor, I'll leave that to the Kubo team to discuss. (cc: @BigLep )

I would say: it should fold into the normal testing process for feature releases. The change contained within this is no different than a libp2p upgrade.

@aschmahmann
Copy link
Contributor

aschmahmann commented Jun 14, 2023

@hannahhoward kubo v0.21.0-rc1 is already out so if that's an acceptable tag for lotus you can use that. If you'd prefer to wait for a final release you can follow along with progress here ipfs/kubo#9814.

@hannahhoward
Copy link
Contributor

This PR only includes tagged dependency updates now, including the kubo RC.

@hannahhoward
Copy link
Contributor

I still think we need to discuss the implications of a Kubo dependency in Lotus

@hannahhoward
Copy link
Contributor

@magik6k @jennijuju @arajasek @ZenGround0 do you all still use the Kubo blockstore bridge in Lotus (used in making storage/retrieval deals)? Does anyone? (my recollection is back in the day this was used by a few 3rd party tools)

Are you ok with a Kubo dependency? (where tagged versions of Kubo don't come out as often)

If:

  1. You still use this feature in Lotus and it can't be removed
  2. You don't want a Kubo dependency

My recommendation is you fork the repo https://github.com/ipfs/go-ipfs-http-client into Filecoin and update it as needed to be compatible with latest boxo.

@BigLep
Copy link
Member

BigLep commented Jun 15, 2023

Maybe not a full response, but wanted to ACKing that that I saw I was tagged here. A couple of things:

Motivations of Boxo and its consolidation of various repos in the ipfs github organization

See https://github.com/ipfs/boxo/wiki/Copied-or-Migrated-Repos-FAQ

Why was ipfs/go-ipfs-http-client merged into Kubo?

I believe there is an issue explaining this more, but this was effectively done to help prevent deviation between Kubo's rpc client and server implementation. Adjustments to the server side can include the client side changes (including tests) in the same PR.

Why does Lotus have a dependency on Kubo RPC API?

I don't know all the specifics here, but I believe Lotus has functionality for importing data from a Kubo node. This change is making it extra clear that Lotus depends on Kubo's RPC interface and that a change in Kubo's RPC interface can break Lotus. At least now Lotus is getting a version that will be better maintained.

Can Lotus drop its Kubo RPC dependency?

I have no insight here on usage, but I'm generally always supportive when code can be removed.

Does depending on kubo/client/rpc blow out Lotus' dependency tree?

At least currently, kubo/client/rpc isn't declaring dependencies to other packages in Kubo, and thus when package pruning is done, it won't pull in the rest of Kubo's dependencies. (I know I'm not using the specific terms here - but this is my conceptual understanding that I have had confirmed verbally at different folks by different team members.)

How do we ensure kubo/client/rpc doesn't start expanding on other Kubo packages?

We are admittedly relying on "best intentions" right now, which isn't ideal. We should have a mechanism. I think all of the monorepos that PL EngRes helps maintain (go-libp2p, Boxo, Kubo, Lotus) would benefit from some tooling with a CI check to enforce this. I created an PL EngRes IPDX item for this: pl-strflt/ipdx#80 . Given it could help multiple teams, it seems like a reasonable thing to prioritize for Q3.

@BigLep
Copy link
Member

BigLep commented Jun 15, 2023

Oh shoot, I started typing before I saw Hannah's latest message. Agreed that it's ultimately up to Lotus maintainers on what they want (add the dependency or fork).

@jennijuju
Copy link
Member

@hannahhoward @BigLep Thanks both! I dont have enough visibility into the points raised so i will wait for the engineers to chime in. 🔴 @filecoin-project/lotus-maintainers, please block merge until we are aligned on the approach & having a testing plan (practicing the new 🟢 rule).

@jennijuju
Copy link
Member

jennijuju commented Jun 15, 2023

@hannahhoward you mentioned this is required for boost - is there any timeline expectation from boost that depends on when this lands in lotus? (we have some OOOs, and a bit packed on perfs & scheduled work recently, so would love some clarity for prioritization.

@hannahhoward
Copy link
Contributor

@jennijuju

re: prioritization -- Boost will ultimately need this code in a tagged Lotus release before cutting their own release, so I would say it would be good to get this into a feature tag getting cut in the next month or so, but if there's one about to happen (i.e. in the next few days) let's skip that.

@hannahhoward
Copy link
Contributor

I am trying to demo the planned feature working end to end in the next few weeks, but I'm pretty sure I can just use the branch for that.

@snadrus
Copy link
Contributor

snadrus commented Jun 19, 2023

How is it that we use IPFS in the modern Lotus stack now that Boost has the markets role?

@Jorropo
Copy link
Contributor Author

Jorropo commented Jun 19, 2023

@snadrus for the legacy market code that hasn't been removed yet.

Also boost and lotus are IPFS implementations too, you use IPFS everywhere. (s,IPFS,Kubo,g)

@arajasek
Copy link
Contributor

@magik6k @jennijuju @arajasek @ZenGround0 do you all still use the Kubo blockstore bridge in Lotus (used in making storage/retrieval deals)? Does anyone? (my recollection is back in the day this was used by a few 3rd party tools)

Are you ok with a Kubo dependency? (where tagged versions of Kubo don't come out as often)

If:

  1. You still use this feature in Lotus and it can't be removed
  2. You don't want a Kubo dependency

My recommendation is you fork the repo https://github.com/ipfs/go-ipfs-http-client into Filecoin and update it as needed to be compatible with latest boxo.

Hey, sorry about the slow response here. I do not know of any pipeline that's still using this bridge, my strong guess is that no one actually uses this. If that is the case, I would favour dropping this.

If not, I do think we should avoid a Kubo dependency here. Your proposal to fork go-ipfs-http-client into Filecoin seems reasonable here. We risk forgetting about its existence (and so never updating Kubo in this fork), but I think that's fine.

If we need to be expedient here, I'd suggest going ahead with the fork for now, and making the "can Lotus drop this functionality?" a separate question that we can track as a Lotus work item.

@magik6k
Copy link
Contributor

magik6k commented Jun 19, 2023

Agreed, this was arguably useful in earlier days when lotus was the only way to make deals. There is a chance that this feature is still in use, but given that all the legacy markets stuff will be deprecated I’d say it’s ok to drop this now.

Jorropo and others added 6 commits June 19, 2023 14:45
This migrates everything except the `go-car` librairy: ipfs/boxo#218 (comment)

I didn't migrated everything in the previous release because all the boxo code wasn't compatible with the go-ipld-prime one due to a an in flight (/ aftermath) revert of github.com/ipfs/go-block-format. go-block-format has been unmigrated since slight bellow absolutely everything depends on it that would have required everything to be moved on boxo or everything to optin into using boxo which were all deal breakers for different groups.

This worked fine because lotus's codebase could live hapely on the first multirepo setup however boost is now trying to use boxo's code with lotus's (still on multirepo) setup: https://filecoinproject.slack.com/archives/C03AQ3QAUG1/p1685022344779649

The alternative would be for boost to write shim types which just forward calls and return with the different interface definitions.

Btw why is that an issue in the first place is because unlike what go's duck typing model suggest interfaces are not transparent golang/go#58112, interfaces are strongly typed but they have implicit narrowing. The issue is if you return an interface from an interface Go does not have a function definition to insert the implicit conversion thus instead the type checker complains you are not returning the right type.

Stubbing types were reverted ipfs/boxo#218 (comment)

Last time I only migrated `go-bitswap` to `boxo/bitswap` because of the security issues and because we never had the interface return an interface problem (we had concrete wrappers where the implicit conversion took place).
@hannahhoward
Copy link
Contributor

@magik6k @jennijuju @arajasek this PR has been updated with a forked repo for your API client. In an effort to insure compatibility, I actually used the latest kubo client rpc code. Here is the forked repo: github.com/filecoin-project/kubo-api-client

@magik6k magik6k self-requested a review June 22, 2023 08:17
@magik6k magik6k merged commit 168d022 into filecoin-project:master Jun 22, 2023
93 checks passed
@Jorropo Jorropo deleted the boxo2 branch June 22, 2023 15:51
@jennijuju
Copy link
Member

and making the "can Lotus drop this functionality?" a separate question that we can track as a Lotus work item.

Do we have an issue for this?

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.

None yet

9 participants