-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: cli: lotus-miner sectors extend command #11928
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still incorrect.
3c171e7
to
12e188c
Compare
Thank you so much for the PR. I think this PR just needs a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good with some questions
|
||
sectorsInDecl := int(sectorsWithoutClaimsCount) + len(sectorsWithClaims) | ||
scount += sectorsInDecl | ||
if scount > addrSectors || len(p.Extensions) >= declMax { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a point for this check. If we are already batching and checking that batch size is smaller than policy max then what's the point of this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what you mean, this part is the original logic, so I didn’t change it.
The first half of this judgment is redundant, and the second half of the inspection may still be useful. I had no one to communicate with at the time, so I chose not to adjust it in a conservative strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I delete the redundant first half?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I realized, there is nothing wrong here, it is completely wrong to remove the first half.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function here is to determine whether a new parameter should be opened. The function of the previous batching is to reduce the input batch size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's raining, 2300ml at a time
My barrel only has 500ml, and every time it rains 2300ml. My barrel is full, and there is still 1800ml (2300-500) left, and my other barrel cannot hold it.
So I divided the 2300ml each time into 500ml each time, so that my barrel can work normally (at this time, I also need to check whether the barrel is full)
932765e
to
7245483
Compare
cli/spcli/sectors.go
Outdated
if len(p.Extensions) >= declMax { | ||
params = append(params, p) | ||
p = miner.ExtendSectorExpiration2Params{} | ||
scount = sectorsInDecl | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest a complete refactor to get rid of this. If we are batching then we simply should determine the batch size correctly and apply the policies beforehand.
if len(p.Extensions) >= declMax { | |
params = append(params, p) | |
p = miner.ExtendSectorExpiration2Params{} | |
scount = sectorsInDecl | |
} | |
if len(p.Extensions) >= declMax { | |
params = append(params, p) | |
p = miner.ExtendSectorExpiration2Params{} | |
scount = sectorsInDecl | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can only help him get back on the right path, but I can't reconstruct it.
7245483
to
5349448
Compare
Related Issues
#11927
Proposed Changes
Additional Info
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>
fix: mempool: Introduce a cache for valid signatures
PR type
: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea
, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps