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

Add Safety Features to Renew Sectors Command #8398

Closed
5 of 15 tasks
TippyFlitsUK opened this issue Mar 29, 2022 · 7 comments
Closed
5 of 15 tasks

Add Safety Features to Renew Sectors Command #8398

TippyFlitsUK opened this issue Mar 29, 2022 · 7 comments
Assignees
Milestone

Comments

@TippyFlitsUK
Copy link
Contributor

Checklist

  • This is not a new feature or an enhancement to the Filecoin protocol. If it is, please open an FIP issue.
  • This is not a new feature request. If it is, please file a feature request instead.
  • This is not brainstorming ideas. If you have an idea you'd like to discuss, please open a new discussion on the lotus forum and select the category as Ideas.
  • I have a specific, actionable, and well motivated improvement to propose.

Lotus component

  • lotus daemon - chain sync
  • lotus miner - mining and block production
  • lotus miner/worker - sealing
  • lotus miner - proving(WindowPoSt)
  • lotus miner/market - storage deal
  • lotus miner/market - retrieval deal
  • lotus miner/market - data transfer
  • lotus client
  • lotus JSON-RPC API
  • lotus message management (mpool)
  • Other

Improvement Suggestion

Since the arrival of Snap Deals, the lotus-miner renew command has become an essential tool for SP's looking to make full use of existing sectors.

In its current form I believe it is far too easy to renew unintended sectors in error. We are already seeing examples of this in support channels which have resulted in accidental storage power loss.

I would like to initially propose a similar approach to the one that is already used when using the lotus-miner sectors expired --remove-expired function. When using this command a list of affected sectors is displayed prior to removal by default and a command is then suggested which includes --confirm-remove-count and --expired-epoch as required fields.

A similar enhancement to the lotus-miner sectors renew command would initially display a list of affected sectors in the same way as above and include the --from and --to flags as required fields. This would also remove the necessity for --really-do-it flag.

I would sincerely welcome comments or alternative suggestions.

Many thanks

@TippyFlitsUK TippyFlitsUK changed the title Add Safety Check to Renew Sectors Command Add Safety Features to Renew Sectors Command Mar 29, 2022
@magik6k
Copy link
Contributor

magik6k commented Mar 29, 2022

I like the two-step idea. It should also warn about non-cc sectors, because, at least in case of fil+ sectors, renewing them may not be the intended action.

@TippyFlitsUK
Copy link
Contributor Author

Great idea @magik6k! That would be a very a useful addition.

@Reiers
Copy link

Reiers commented Mar 29, 2022

I would sincerely welcome comments or alternative suggestions

If it can differentiate between CC and deal/FIL+ sectors then adding a flag like this:
lotus-miner sectors renew --only-cc --from=x --to=x

Could also be an option.

@TippyFlitsUK TippyFlitsUK self-assigned this Mar 30, 2022
@Angelo-gh3990
Copy link

@Reiers I completely agree, that should be an option and would make it super clear. I would even go as far that you always have to give an option --only-cc=true or to renew any others --with-deals=true, so no matter if you execute it like lotus-miner sectors renew --really-do-it you never change the deal duration or anything else.

Thanks @TippyFlitsUK to put this together

@William8Work
Copy link

I did run into power lost after the renew command. what happened to the fil+ sectors? @TippyFlitsUK

@rjan90
Copy link
Contributor

rjan90 commented Aug 19, 2022

So this feature lotus-miner sectors renew --only-cc --from=x --to=x have been implemented with #9184.

Personally, I think it solves a lot of the above mentioned issues. But there still might be some desire to have a simulation check/confirm for this command?

@rjan90 rjan90 added this to the LM-Tech-Debt milestone Mar 31, 2023
@rjan90
Copy link
Contributor

rjan90 commented Apr 4, 2023

Closing this now as the lotus-miner sectors renew cmd, refactored in v1.21.0 to the lotus-miner sectors extend cmd is now running a simulation/check, and the user can check which sectors will be extended before pushing the message with --really-do-it

Example:

lotus-miner sectors extend 
Extending 72 sectors: 
 {
  "Extensions": [
    {
      "Deadline": 2,
      "Partition": 0,
      "Sectors": "3183-3186,3188-3189,3191-3195,3200-3204,3206-3211,3228-3229,3233-3234,3236-3245,3247-3251,3261,3263-3276,3280-3283,3285-3288",
      "NewExpiration": 4298104
    },
    {
      "Deadline": 3,
      "Partition": 0,
      "Sectors": "3197,3199,3235,3253",
      "NewExpiration": 4298104
    },
    {
      "Deadline": 1,
      "Partition": 0,
      "Sectors": "3205,3246,3252,3289",
      "NewExpiration": 4298104
    }
  ]
}
72 sectors extended

@rjan90 rjan90 closed this as completed Apr 4, 2023
@github-project-automation github-project-automation bot moved this to 👀 In Review in Lotus-Miner-V2 Apr 4, 2023
@rjan90 rjan90 moved this from 👀 In Review to 🛑 Removed/Closed/Invalid/Outdated in Lotus-Miner-V2 Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🛑 Removed/Closed/Invalid/Outdated
Development

No branches or pull requests

6 participants