cleanup: Lotus client: remove markets and deal-making from Lotus Client #11999

aarshkshah1992 · 2024-05-16T07:23:06Z

Related Issues

Closes #12002

Proposed Changes

The use of legacy markets through Lotus was deprecated a long time ago. This PR completely remove the legacy markets code from the Lotus Client CLI & API. Users can continue using Boost for deal making/as a deal making client.

Specifically, this PR removes the following APIs from the Lotus Full Node:

        // ClientImport imports file under the specified path into filestore.
	ClientImport(ctx context.Context, ref FileRef) (*ImportRes, error) //perm:admin
	// ClientRemoveImport removes file import
	ClientRemoveImport(ctx context.Context, importID imports.ID) error //perm:admin
	// ClientStartDeal proposes a deal with a miner.
	ClientStartDeal(ctx context.Context, params *StartDealParams) (*cid.Cid, error) //perm:admin
	// ClientStatelessDeal fire-and-forget-proposes an offline deal to a miner without subsequent tracking.
	ClientStatelessDeal(ctx context.Context, params *StartDealParams) (*cid.Cid, error) //perm:write
	// ClientGetDealInfo returns the latest information about a given deal.
	ClientGetDealInfo(context.Context, cid.Cid) (*DealInfo, error) //perm:read
	// ClientListDeals returns information about the deals made by the local client.
	ClientListDeals(ctx context.Context) ([]DealInfo, error) //perm:write
	// ClientGetDealUpdates returns the status of updated deals
	ClientGetDealUpdates(ctx context.Context) (<-chan DealInfo, error) //perm:write
	// ClientGetDealStatus returns status given a code
	ClientGetDealStatus(ctx context.Context, statusCode uint64) (string, error) //perm:read
	// ClientHasLocal indicates whether a certain CID is locally stored.
	ClientHasLocal(ctx context.Context, root cid.Cid) (bool, error) //perm:write
	// ClientFindData identifies peers that have a certain file, and returns QueryOffers (one per peer).
	ClientFindData(ctx context.Context, root cid.Cid, piece *cid.Cid) ([]QueryOffer, error) //perm:read
	// ClientMinerQueryOffer returns a QueryOffer for the specific miner and file.
	ClientMinerQueryOffer(ctx context.Context, miner address.Address, root cid.Cid, piece *cid.Cid) (QueryOffer, error) //perm:read
	// ClientRetrieve initiates the retrieval of a file, as specified in the order.
	ClientRetrieve(ctx context.Context, params RetrievalOrder) (*RestrievalRes, error) //perm:admin
	// ClientRetrieveWait waits for retrieval to be complete
	ClientRetrieveWait(ctx context.Context, deal retrievalmarket.DealID) error //perm:admin
	// ClientExport exports a file stored in the local filestore to a system file
	ClientExport(ctx context.Context, exportRef ExportRef, fileRef FileRef) error //perm:admin
	// ClientListRetrievals returns information about retrievals made by the local client
	ClientListRetrievals(ctx context.Context) ([]RetrievalInfo, error) //perm:write
	// ClientGetRetrievalUpdates returns status of updated retrieval deals
	ClientGetRetrievalUpdates(ctx context.Context) (<-chan RetrievalInfo, error) //perm:write
	// ClientQueryAsk returns a signed StorageAsk from the specified miner.
	ClientQueryAsk(ctx context.Context, p peer.ID, miner address.Address) (*StorageAsk, error) //perm:read
	// ClientCalcCommP calculates the CommP and data size of the specified CID
	ClientDealPieceCID(ctx context.Context, root cid.Cid) (DataCIDSize, error) //perm:read
	// ClientCalcCommP calculates the CommP for a specified file
	ClientCalcCommP(ctx context.Context, inpath string) (*CommPRet, error) //perm:write
	// ClientGenCar generates a CAR file for the specified file.
	ClientGenCar(ctx context.Context, ref FileRef, outpath string) error //perm:write
	// ClientDealSize calculates real deal data size
	ClientDealSize(ctx context.Context, root cid.Cid) (DataSize, error) //perm:read
	// ClientListTransfers returns the status of all ongoing transfers of data
	ClientListDataTransfers(ctx context.Context) ([]DataTransferChannel, error)        //perm:write
	ClientDataTransferUpdates(ctx context.Context) (<-chan DataTransferChannel, error) //perm:write
	// ClientRestartDataTransfer attempts to restart a data transfer with the given transfer ID and other peer
	ClientRestartDataTransfer(ctx context.Context, transferID datatransfer.TransferID, otherPeer peer.ID, isInitiator bool) error //perm:write
	// ClientCancelDataTransfer cancels a data transfer with the given transfer ID and other peer
	ClientCancelDataTransfer(ctx context.Context, transferID datatransfer.TransferID, otherPeer peer.ID, isInitiator bool) error //perm:write
	// ClientRetrieveTryRestartInsufficientFunds attempts to restart stalled retrievals on a given payment channel
	// which are stuck due to insufficient funds
	ClientRetrieveTryRestartInsufficientFunds(ctx context.Context, paymentChannel address.Address) error //perm:write

	// ClientCancelRetrievalDeal cancels an ongoing retrieval deal based on DealID
	ClientCancelRetrievalDeal(ctx context.Context, dealid retrievalmarket.DealID) error //perm:write

	// ClientUnimport removes references to the specified file from filestore
	// ClientUnimport(path string)

	// ClientListImports lists imported files and their root CIDs
	ClientListImports(ctx context.Context) ([]Import, error) //perm:write

In addition to the above, the lotus client CLI family of commands has also been completely removed.

And also removed the following CLI commands from Lotus:

Additional Info

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
If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
New features have usage guidelines and / or documentation updates in
- Lotus Documentation
- Discussion Tutorials
Tests exist for new functionality or change in behavior
CI is green

rvagg · 2024-05-16T07:55:44Z

I guess we've been warning about this for a while, I only noticed this today on some CI output:

2024-05-16T07:30:55.950Z	ERROR	alerting	alerting/alerts.go:106	alert raised	{"type": {"System":"system","Subsystem":"EOL"}, "message": {"message":"The lotus-miner legacy markets subsystem is deprecated and will be removed in a future release. Please migrate to [Boost](https://boost.filecoin.io/) or similar markets subsystems."}}

f8-ptrk · 2024-05-16T08:31:09Z

https://github.com/filecoin-project/lotus/blob/master/node/config/def.go

needs adjustments i think

ribasushi · 2024-05-16T08:46:58Z

Aside from #11999 (comment) superficially this looks 👌

Flagging to @rjan90 @jennijuju @magik6k @LexLuthr and others that this set of tests is now gone. If things like curio and/or boost are not covering parts of the funcionality things might regress silently in the future

I do not have the context to evaluate what is and isn't valuable here, just making sure someone who has the context will ✅ this.

cc @smagdali as potentially impacting

LexLuthr · 2024-05-16T08:53:09Z

From Boost point of view this looks Good. Boost switched to it's own client a while ago for testing. We do have a full suite of deal tests there. Lotus client has been unsupported by Boost for a while.
Curio will have a new tests for deals which will probably skip using any client.
Magik can confirm about lotus-miner as itest suite is very tightly coupled with client API I think.

rvagg · 2024-05-16T08:58:58Z

@aarshkshah1992 I'll have to look at this tomorrow, but further to @ribasushi's point - would you be able to make a basic (rough) inventory of which parts of builtin actors behaviours/methods we may be leaving with less coverage here in the Lotus repo. While it's good to know that Boost and Curio will have coverage, it's important that we also maintain enough coverage so we have confidence to iterate on actors while knowing they're going to be exercised properly. We don't necessarily have to keep tests around, but it might give us a good idea of what we need to focus on build separate to the deal flow (i.e. more manually than these tests do).

aarshkshah1992 · 2024-05-16T15:16:15Z

Will make the adjustments to https://github.com/filecoin-project/lotus/blob/master/node/config/def.go in a subsequent PR that I am raising for the provider side of things as it is a bit tightly coupled.

aarshkshah1992 · 2024-05-16T15:26:31Z

Follow-up PR to remove Markets from Lotus Miner and Full Node is at #12005.

aarshkshah1992 · 2024-05-16T15:26:58Z

@rvagg @ribasushi Yes will do a write-up on what test coverage we're lacking now.

ZenGround0 · 2024-05-16T14:08:08Z

.circleci/config.yml

-          suite: itest-deals_retry_deal_no_funds
-          target: "./itests/deals_retry_deal_no_funds_test.go"
-      - test:
-          name: test-itest-deals


ZenGround0 · 2024-05-16T14:11:16Z

api/api_full.go

-	// ClientCalcCommP calculates the CommP and data size of the specified CID
-	ClientDealPieceCID(ctx context.Context, root cid.Cid) (DataCIDSize, error) //perm:read
-	// ClientCalcCommP calculates the CommP for a specified file
-	ClientCalcCommP(ctx context.Context, inpath string) (*CommPRet, error) //perm:write


I end up using CalcCommP and GenCAR a lot during development and I don't think they use go-fil-markets. Could you add them to lotus-shed instead of deleting them entirely?

I don't think they need to use go-fil-markets. But if its hard to recreate without the API call I can do it later.

@ZenGround0 isn't kubo much better suited for exporting a dag as a carfile...? Why would you be using the way less maintained blockstore-in-client-lotus implementations?

idk because I'm a phillistine who just wants to play with his deal smart contracts

Me have file me want commp

Me have file me want commp

cat TheZenGround0_files.mp4 | stream-commp

https://github.com/filecoin-project/go-fil-commp-hashhash/tree/master/cmd/stream-commp#installation

If you reallt insist on going the long route through ipld-unixfs - that probably fits in lotus-shed.

yeah, original discussion on this suggested that we may want to move some things to lotus-shed, and commp seems reasonable

boostx has all this functionality if you want to install it

in terms of unixfs generation, just use go-car, car is not a bad little utility to have installed, and then one of the existing commp generation utilities like stream-commp

Yeah I am not sure we need to keep the CommP functionality around in lotus-shed given the plethora of other tools out there that already do it if Lotus itself does not need this functionality for anything anymore.

Worth checking the docs/tutorials - pretty sure it’s referenced in a couple places

aarshkshah1992 · 2024-05-17T10:25:53Z

@rvagg I went through the itests we have deleted and it dosen't look like we're losing any meaningful Actors coverage:

The DDO itests you wrote already cover deal publishing and waiting for sectors to be sealed and they are the main code paths getting removed from the deleted tests (that concern us).
Other than that, we're losing coverage of some Miner/Miner-Worker behaviour wrt to sector upgrades and sector upgrade aborts but that should be covered by Curio.

That Market Actor AddBalance method is covered by the supply_test.go itest and so we have some coverage for the Market Actor too. I think we are fine here but let me know if you think otherwise.

I think all we need is a test for upgrading a CC sector with real data.

…-client

github-actions · 2024-05-17T10:39:41Z

All checks have passed

itests/worker_upgrade_test.go

…-client

chain/sub/incoming.go

cmd/lotus-miner/sectors.go

documentation/en/cli-lotus-miner.md

rvagg · 2024-05-23T10:13:20Z

itests/path_type_filters_test.go

-
-			asserts(t, ctx, &miner, func() {
-				dh := kit.NewDealHarness(t, &client, &miner, &miner)
-				dh.RunConcurrentDeals(kit.RunConcurrentDealsOpts{N: 1})


oh, this test seems like a bit of a loss all because it needs deals to exercise storage; it'd be good to get some curio ppl feedback on this one

I think this should get covered by Curio tests. I am not sure if it is great ROI building a lot of testing deal onboarding infra just for this.

rvagg · 2024-05-23T10:14:20Z

itests/sector_finalize_early_test.go

-	}
-
-	t.Run("single", func(t *testing.T) {
-		dh.RunConcurrentDeals(kit.RunConcurrentDealsOpts{N: 1})


another test that's in the unfortunate blast radius; I wonder if we could replace this functionality with something simpler so this doesn't all have to go away

rvagg · 2024-05-23T10:14:43Z

itests/worker_test.go

-	require.NoError(t, err)
-	require.True(t, e)
-
-	dh := kit.NewDealHarness(t, client, miner, miner)


ditto; would be good to not lose this one too

I mean this should get covered by Curio tests.

rvagg · 2024-05-23T10:18:58Z

This looks great to me, exciting to remove so much legacy.

2 things to resolve: removal of lib/unixfs package, and restoration of sectors refs CLI.

And there's the few tests in there that are not deal related but get caught up in the lack of deal infrastructure, yet they're testing important functionality that leaves a gap.

I think next step might be to get an eye from @magik6k; both in terms of the general approach here but also for some feedback about those tests that I marked as a concern—should we put some work in to replacing the deal infrastructure with some data onboarding flows to keep the tests active?

aarshkshah1992 · 2024-05-30T07:58:16Z

@rvagg I am not sure if it is high ROI to add and then maintain deal onboarding code just to make up for a couple of tests. Some of that should be covered by Curio tests.

And we're adding tests for SnapDeals and DDO to cover the Actors interaction as part of our NI-PoRep testing work.

rvagg · 2024-05-31T01:09:43Z

lgtm! if we can get sign-off from a miner expert then this should be gtg

aarshkshah1992 added 5 commits May 16, 2024 10:11

remove client CLI

96b4593

remove markets CLI from miner

03bd0a5

remove markets from all CLI

74dbabb

remove client API

2c4d1bd

update go mod

ba3e26c

aarshkshah1992 changed the title ~~[WIP] Remove Markets deal making CLI and Client from Lotus~~ Remove Markets deal making CLI and Client from Lotus May 16, 2024

aarshkshah1992 changed the title ~~Remove Markets deal making CLI and Client from Lotus~~ Remove Lotus Client deal making CLI and API from Lotus May 16, 2024

aarshkshah1992 requested a review from rvagg May 16, 2024 08:17

aarshkshah1992 changed the title ~~Remove Lotus Client deal making CLI and API from Lotus~~ cleanup: Lotus deal making client: remove client deal making API and CLI May 16, 2024

aarshkshah1992 mentioned this pull request May 16, 2024

cleanup: miner: remove markets and deal-making from Lotus Miner #12005

Open

8 tasks

ZenGround0 approved these changes May 16, 2024

View reviewed changes

rvagg mentioned this pull request May 17, 2024

[DX Streamline] Remove legacy components: client dealmaking #12008

Closed

Merge remote-tracking branch 'origin/master' into feat/remove-markets…

64133ec

…-client

aarshkshah1992 commented May 20, 2024

View reviewed changes

itests/worker_upgrade_test.go Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into feat/remove-markets…

0a44fce

…-client

aarshkshah1992 changed the title ~~cleanup: Lotus deal making client: remove client deal making API and CLI~~ cleanup: Lotus client: remove markets and deal-making from Lotus Client May 22, 2024