feat(spcli): correctly handle the batch logic of `lotus-miner actor settle-deal` #13189

beck-8 · 2025-06-30T15:28:06Z

Related Issues

https://filecoinproject.slack.com/archives/C03CKDLEWG1/p1751257247291959?thread_ts=1745286086.505189&cid=C03CKDLEWG1

Proposed Changes

Additional Info

Checklist

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

Commits have a clear commit message.
PR title conforms with contribution conventions
Update CHANGELOG.md or signal that this change does not need it per contribution conventions
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

BigLep · 2025-07-01T06:19:06Z

@beck-8 : looks like there's a small lint error to fix.

rvagg

there's too much duplicated code in here from cli/spcli/actor.go, we need to reduce the overlap because they're doing almost identical things so we can share the code even if we have it in two difference places.

I'm assuming that the intention here is that you'd like to have it available as a lotus-shed and not need to have the full lotus-miner setup available and you shouldn't be tied to a specific miner address? That's fine, but we need to dedupe.

beck-8 · 2025-07-01T06:44:00Z

I originally wanted to change it on the original foundation (lotus-miner). I don't think anyone will need to check the details of the message. Now everyone who uses this tool urgently wants to take out the locked amount.

However, considering that someone may really ask to check the payment, I changed the logic to lotus-shed. This is my idea. Do you have any suggestions?

beck-8 · 2025-07-01T08:14:28Z

Or should I ask if we really need this part of logic? Or put this small part elsewhere in the future?

lotus/cli/spcli/actor.go

Lines 141 to 171 in cfb4c8e

    
           // wait for it to get mined into a block 
        
           wait, err := api.StateWaitMsg(ctx, res, uint64(cctx.Int("confidence")), lapi.LookbackNoLimit, true) 
        
           if err != nil { 
        
           	return xerrors.Errorf("Timeout waiting for deal settlement message %s", res) 
        
           } 
        
           if wait.Receipt.ExitCode.IsError() { 
        
           	return xerrors.Errorf("Failed to execute withdrawal message %s: %w", wait.Message, wait.Receipt.ExitCode.Error()) 
        
           } 
        
           var settlementReturn markettypes14.SettleDealPaymentsReturn 
        
           if err = settlementReturn.UnmarshalCBOR(bytes.NewReader(wait.Receipt.Return)); err != nil { 
        
           	return err 
        
           } 
        
           fmt.Printf("Settled %d out of %d deals\n", settlementReturn.Results.SuccessCount, len(dealIDs)) 
        
           var ( 
        
           	totalPayment        = big.Zero() 
        
           	totalCompletedDeals = 0 
        
           ) 
        
           for _, s := range settlementReturn.Settlements { 
        
           	totalPayment = big.Add(totalPayment, s.Payment) 
        
           	if s.Completed { 
        
           		totalCompletedDeals++ 
        
           	} 
        
           } 
        
           fmt.Printf("Total payment: %s\n", types.FIL(totalPayment)) 
        
           fmt.Printf("Total number of deals finished their lifetime: %d\n", totalCompletedDeals)

rvagg · 2025-07-02T04:48:18Z

Actually @beck-8, hold off from putting in all that work. I think maybe you should just explain, both briefly in the CHANGELOG and a little more verbose in the Description of the new command what it does and why it's useful instead of the lotus-miner variant. I think maybe you've got a very narrow use-case here.

The alternative is to just go ahead and adapt your use-case to the lotus-miner one, make one command but make it do what you want to do on top of its existing flexibility -- or make a judgement about whether its existing flexibility is even very useful. I see complaints about it, if you just want to fix it then we can get additional SP feedback before doing so but we shouldn't feel like these things are not open to changing.

beck-8 · 2025-07-02T04:56:05Z

In fact, no matter which one, SP will still complain, because StateMarketDeals is as high as 29G, and the premise of executing settlement is to wait for a few minutes to pull this data.

beck-8 · 2025-07-02T04:59:31Z

I will give more explanations later. I know that the use case is relatively narrow, but I have no other good way to help SP. Some experienced people will use lotus-miner sectors status to extract deal, but it only applies to certain environments of some people.

As we discussed at that time, I put forward my ideas and cases. But you don't have to merge this PR. You can close it and restructure it, but you should know my idea.

beck-8 · 2025-07-10T02:15:01Z

Quickly obtain the relationship between sectors and deals
Reasonably handle batches and send messages.

I suggest exporting this method or transferring it to a reasonable place, and then GetMinerAllDeals -> GetMarketDealIDs
https://github.com/sumitvekariya/lotus/blob/7f2a7b7179dd75b5e379ac78e2af1f9caca04d58/cli/state.go#L1752

BigLep · 2025-07-10T21:45:22Z

My 2025-07-10 understanding of where this PR is at:

Related conversation: https://filecoinproject.slack.com/archives/C03CKDLEWG1/p1745286086505189
@beck-8's core goal is to make this API faster. It's currently too slow because it has to load and transfer all the deals.
Maintainers want to avoid a duplicated lotus-shed settle-deal when there’s a lotus-miner settle-deal
@beck-8 and @tediou5 will work to land this so it has a faster API and that it's leveraged in lotus-miner settle-deal
For now while it's still being worked on, it's in draft.

beck-8 · 2025-07-29T09:53:32Z

I know this may not meet some norms. I just want to express the effect I want.

I may merge the command into lotus-miner later (still some logic will be removed)

beck-8 · 2025-07-29T11:45:58Z

Test completed, currently meets my expectations.

./lotus-miner --actor f01703655 actor settle-deal --really-do-it --max-deals 50 --from f3tdlvj4lzhdnes5qzg2w6cghsotka7kjeaiho3qpffctyqlncgls5oxlnclgk4mevivj3acvx46sumyg5ng5a
There are a total of 3335 deals, and about 66 messages will be sent out quickly.
Requested deal settlement in message: bafy2bzacea2j2oscgzjkcrc4pcyh4whja346tf2fkqmp7eqcajvpvegciauqg
Requested deal settlement in message: bafy2bzaceausxqeos6e5qglqqwdfn7yq3pu74opupun4muy6msovmjutdcyw2
Requested deal settlement in message: bafy2bzacecro7en6ij2zik6uvmih6kkukdfbi2p6rkz3stt3yok5xcmuhuyz4
Requested deal settlement in message: bafy2bzaceddzbutjwhg53n3ard7ktomuts4fwr5jevv5slhnasxnev3grikwy
Requested deal settlement in message: bafy2bzaced7by4ol2lnwqs3aabgkcuitad22xpxd46xttxn6dyqnjrdjly4ck
Requested deal settlement in message: bafy2bzaceb7p4gug4vcw23g4i7iogykr3qe4biibjfu37zur7in25teiymppa
Requested deal settlement in message: bafy2bzacebpec5kfuk2ofnq66qlbl4jd6pmtbfip4xuzk5g64ekxg3ivkcjgk
Requested deal settlement in message: bafy2bzacede35a4owjk7eark63vvucibklz2yfurs6p4aeclkqquujfl4cy3a
Requested deal settlement in message: bafy2bzacebafpufpphriwrh7zrxzqcliwcsefbdw5bfzbhhfivvw5kubafnfo
Requested deal settlement in message: bafy2bzaced7ttpkttehptrylyrwgemj7qeuam2iucfs5l2tldgzkordzu7tye
Requested deal settlement in message: bafy2bzacea5puvzm6ncj6gvzq4nblpsgtbbp2nwdbm5ug66j4sxjxktbqhlq4
Requested deal settlement in message: bafy2bzacedxpsykecnfhibggweaa5tphb2pn7oxcv3dg63jzbipvk42bmw5x6
Requested deal settlement in message: bafy2bzaceat3z7hrkocyverij2sre2trp3l7ti7cueaps4mnr5py5hmu666yg
ERROR: mpool push: failed to push message: failed to check balance: not enough funds including pending messages (required: 0.422663959546500762 FIL, balance: 0.3917174615362471 FIL): validation failure

			&cli.IntFlag{
				Name:  "max-deals",
				Usage: "the maximum number of deals contained in each message",
				Value: 50,
			},

--max-deals 500 --max-deals 100 out of gas, so set 50

…ettle-deal`

beck-8 · 2025-07-29T12:16:06Z

I don't know how to deal with the conflict in UNRELEASED.md. If I'm wrong, please help me correct it directly.

rjan90 · 2025-07-29T12:42:08Z

@beck-8 I resolved the conflict in the CHANGELOG.md for you

tediou5

Thank you for your pr.

From my perspective, this PR contains two parts:

An optimization for the GetDeals method.
Batch submission for settle-deal.

Regarding the first part, I'm in full agreement. This will greatly improve the quality of life for SPs.

However, for the second part, I have some concerns:

I might be lacking some background knowledge on this, but is it necessary to batch settle-deal transactions? This shouldn't be a particularly heavy transaction.
The logic after the transaction is modified here to exit immediately without waiting. While I can understand this behavior, it's a breaking change.

I think, would it be better to preserve the original behavior of waiting for the packaging to complete, but provide a flag to skip it directly?

Additionally, I suggest splitting this PR into two parts. This PR should only contain the GetDeals optimization, which we can merge directly. For the batching part, please submit it as a new, separate PR where we can have a more detailed discussion and work on the implementation.

cli/spcli/actor.go

cli/state.go

cli/spcli/actor.go

tediou5

Great, this is very close to completion. The rest of it looks good to me. I think we can get this merged once the batch push message is optimized.

cli/spcli/actor.go

tediou5 · 2025-07-30T09:34:03Z

I've submitted the change to switch to using MpoolBatchPushMessage, also combine logs to prevent interleaving caused by concurrency.

beck-8 · 2025-07-30T10:35:06Z

It looks fine, very good, take off 🛫

Dismissing the request for change, since that request was for the lotus-shed cmd variant, but this has now changed fix/correctly handle the batch logic of lotus-miner actor settle-deal

tediou5

Looks good to me, thanks!

tediou5 · 2025-07-31T01:34:09Z

lets go 🛫!

…ettle-deal` (#13189) * feat(spcli): correctly handle the batch logic of `lotus-miner actor settle-deal` * retain StateWaitMsg logic * use v0api.WrapperV1Full * opt(spcli): push settle-deals Chucks with MpoolBatchPushMessage * chore(spcli): combine logs to prevent interleaving caused by concurrency * fix typo * chore: remove unnecessary var --------- Co-authored-by: Phi-rjan <orjan.roren@gmail.com> Co-authored-by: qians <qiangezaici@outlook.com>

* feat(spcli): correctly handle the batch logic of `lotus-miner actor settle-deal` (#13189) * feat(spcli): correctly handle the batch logic of `lotus-miner actor settle-deal` * retain StateWaitMsg logic * use v0api.WrapperV1Full * opt(spcli): push settle-deals Chucks with MpoolBatchPushMessage * chore(spcli): combine logs to prevent interleaving caused by concurrency * fix typo * chore: remove unnecessary var --------- Co-authored-by: Phi-rjan <orjan.roren@gmail.com> Co-authored-by: qians <qiangezaici@outlook.com> * Empty commit to trigger CI --------- Co-authored-by: beck <34204218+beck-8@users.noreply.github.com> Co-authored-by: qians <qiangezaici@outlook.com>

beck-8 · 2025-08-01T20:22:59Z

I want to add a flag to indicate whether the deal is expired. By default, only expired deals will be processed.

tediou5 · 2025-08-04T03:22:06Z

I want to add a flag to indicate whether the deal is expired. By default, only expired deals will be processed.

Makes sense to me, let's open a new PR to add this.

github-project-automation bot added this to FilOz Jun 30, 2025

github-project-automation bot moved this to 📌 Triage in FilOz Jun 30, 2025

BigLep requested a review from rvagg July 1, 2025 06:19

BigLep moved this from 📌 Triage to 🔎 Awaiting Review in FilOz Jul 1, 2025

rvagg previously requested changes Jul 1, 2025

View reviewed changes

github-project-automation bot moved this from 🔎 Awaiting Review to ⌨️ In Progress in FilOz Jul 1, 2025

beck-8 force-pushed the op/settle-deal branch from 7ea9f4f to e8923f6 Compare July 1, 2025 11:54

beck-8 force-pushed the op/settle-deal branch from e8923f6 to a883d77 Compare July 9, 2025 02:07

beck-8 marked this pull request as draft July 10, 2025 02:07

beck-8 force-pushed the op/settle-deal branch from a883d77 to cc2f7af Compare July 29, 2025 09:51

beck-8 force-pushed the op/settle-deal branch 3 times, most recently from 7aaa5b4 to 1880e60 Compare July 29, 2025 11:42

beck-8 changed the title ~~feat: add settle-deal to lotus-shed~~ feat(spcli): correctly handle the batch logic of lotus-miner actor settle-deal Jul 29, 2025

beck-8 marked this pull request as ready for review July 29, 2025 11:44

rjan90 requested a review from tediou5 July 29, 2025 11:46

beck-8 force-pushed the op/settle-deal branch from 04f99bb to 518d3de Compare July 29, 2025 12:09

feat(spcli): correctly handle the batch logic of `lotus-miner actor s…

784eadc

…ettle-deal`

beck-8 force-pushed the op/settle-deal branch from 518d3de to 784eadc Compare July 29, 2025 12:13

Merge branch 'master' into op/settle-deal

eb2a44c

tediou5 requested changes Jul 30, 2025

View reviewed changes

cli/spcli/actor.go Outdated Show resolved Hide resolved

retain StateWaitMsg logic

504dbee

tediou5 reviewed Jul 30, 2025

View reviewed changes

cli/state.go Outdated Show resolved Hide resolved

tediou5 reviewed Jul 30, 2025

View reviewed changes

cli/spcli/actor.go Outdated Show resolved Hide resolved

cli/spcli/actor.go Show resolved Hide resolved

use v0api.WrapperV1Full

d02aaf0

tediou5 reviewed Jul 30, 2025

View reviewed changes

cli/spcli/actor.go Outdated Show resolved Hide resolved

tediou5 added 2 commits July 30, 2025 16:45

opt(spcli): push settle-deals Chucks with MpoolBatchPushMessage

b06f674

chore(spcli): combine logs to prevent interleaving caused by concurrency

eb41c14

tediou5 force-pushed the op/settle-deal branch from 0325486 to eb41c14 Compare July 30, 2025 09:30

tediou5 and others added 3 commits July 30, 2025 17:44

fix typo

85797cb

chore: remove unnecessary var

56355cc

chore: remove fmt

fa6e3f8

tediou5 approved these changes Jul 31, 2025

View reviewed changes

github-project-automation bot moved this from ⌨️ In Progress to ✔️ Approved by reviewer in FilOz Jul 31, 2025

tediou5 merged commit 0edb14c into filecoin-project:master Jul 31, 2025
95 checks passed

github-project-automation bot moved this from ✔️ Approved by reviewer to 🎉 Done in FilOz Jul 31, 2025

rjan90 mentioned this pull request Jul 31, 2025

chore: backport #13189 to the release/v1.33.1 branch #13238

Merged

8 tasks

rjan90 mentioned this pull request Jul 31, 2025

Lotus Node v1.33.1 Release #13132

Closed

43 tasks

BigLep mentioned this pull request Sep 4, 2025

build: release Lotus Node and Miner v1.34.0-rc1 #13314

Merged

8 tasks

feat(spcli): correctly handle the batch logic of lotus-miner actor settle-deal #13189

feat(spcli): correctly handle the batch logic of lotus-miner actor settle-deal #13189

Uh oh!

Conversation

beck-8 commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issues

Proposed Changes

Additional Info

Checklist

Uh oh!

BigLep commented Jul 1, 2025

Uh oh!

rvagg left a comment

Choose a reason for hiding this comment

Uh oh!

beck-8 commented Jul 1, 2025

Uh oh!

beck-8 commented Jul 1, 2025

Uh oh!

rvagg commented Jul 2, 2025

Uh oh!

beck-8 commented Jul 2, 2025

Uh oh!

beck-8 commented Jul 2, 2025

Uh oh!

beck-8 commented Jul 10, 2025

Uh oh!

BigLep commented Jul 10, 2025

Uh oh!

beck-8 commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beck-8 commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beck-8 commented Jul 29, 2025

Uh oh!

rjan90 commented Jul 29, 2025

Uh oh!

tediou5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tediou5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tediou5 commented Jul 30, 2025

Uh oh!

beck-8 commented Jul 30, 2025

Uh oh!

tediou5 left a comment

Choose a reason for hiding this comment

Uh oh!

tediou5 commented Jul 31, 2025

Uh oh!

Uh oh!

beck-8 commented Aug 1, 2025

Uh oh!

tediou5 commented Aug 4, 2025

Uh oh!

Uh oh!

feat(spcli): correctly handle the batch logic of `lotus-miner actor settle-deal` #13189

feat(spcli): correctly handle the batch logic of `lotus-miner actor settle-deal` #13189

beck-8 commented Jun 30, 2025 •

edited

Loading

beck-8 commented Jul 29, 2025 •

edited

Loading

beck-8 commented Jul 29, 2025 •

edited

Loading