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

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

Merged
merged 9 commits into from
Jun 5, 2024

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented May 16, 2024

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
  • Tests exist for new functionality or change in behavior
  • CI is green

@rvagg
Copy link
Member

rvagg commented May 16, 2024

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."}}

@aarshkshah1992 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 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 aarshkshah1992 requested a review from rvagg May 16, 2024 08:17
@aarshkshah1992 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
@f8-ptrk
Copy link
Contributor

f8-ptrk commented May 16, 2024

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

needs adjustments i think

@ribasushi
Copy link
Collaborator

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
image

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
Copy link
Contributor

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
Copy link
Member

rvagg commented May 16, 2024

@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
Copy link
Contributor Author

aarshkshah1992 commented May 16, 2024

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
Copy link
Contributor Author

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

@aarshkshah1992
Copy link
Contributor Author

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

.circleci/config.yml Outdated Show resolved Hide resolved
api/api_full.go Show resolved Hide resolved
@aarshkshah1992
Copy link
Contributor Author

aarshkshah1992 commented May 17, 2024

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

  1. 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).

  2. 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.

Copy link

github-actions bot commented May 17, 2024

All checks have passed

@aarshkshah1992 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
@aarshkshah1992 aarshkshah1992 merged commit 469960c into master Jun 5, 2024
75 checks passed
@aarshkshah1992 aarshkshah1992 deleted the feat/remove-markets-client branch June 5, 2024 05:56
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.

[DX Streamline] Remove Lotus Client deal making CLI and API Remove Lotus market from code base
7 participants