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

Extending sectors: more practical and flexible tools #6097

Merged
merged 12 commits into from
Aug 16, 2021

Conversation

zlhwdsz
Copy link
Contributor

@zlhwdsz zlhwdsz commented Apr 23, 2021

New cli tools for managing sectors expiration:

  • check-expire, facilitates insepection of expiring sectors

  • renew, a more practical alternative to extend with the following features:

    • can automatically extend sectors of all kinds of seal proof, not just v1 ones

    • can mannully provide sector numbers in a file

    • can choose from 2 extending schemes: a relative extension or an uniform new expiration

    • will take care of SectorMaxLifetime and MaxSectorExpirationExtension

    • fast search of sector location

    • can specify max-fee to avoid message congestion

@zlhwdsz
Copy link
Contributor Author

zlhwdsz commented Apr 25, 2021

The gas-feecap seems not working as expected. Will fix it.

@arajasek arajasek self-requested a review May 3, 2021 17:09
@jennijuju jennijuju added the P2 P2: Should be resolved label May 17, 2021
}
MaxExpiration := sector.Activation + maxLifetime

MaxExtendNow := currEpoch + miner3.MaxSectorExpirationExtension
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: miner5

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. And code rebased onto master.

@zlhwdsz zlhwdsz requested review from ZenGround0 and removed request for a team August 13, 2021 05:51
Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To merge you need to get ci passing. There are some lint errors and something going on with gen-check.

I'm not going to block merging on this but the code for the renew command would benefit from more decomposition. There are lots of separate logical pieces here that you could split out to help with future maintenance. If you go down this route you could test these pieces separately which will help increase the confidence that everything works as it should.

return &res, nil
}

// Example: {1,3,4,5,8,9} -> "1,3-5,8-9"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: // Duplictes are not filtered, inputs must be sorted

},
&cli.StringFlag{
Name: "max-fee",
Usage: "use up to this amount of attoFIL for one message. pass this flag to avoid message congestion.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should now be specified in FIL (see later comment or recent change to mpool replace)

Comment on lines 618 to 623
mf, err := types.BigFromString(cctx.String("max-fee"))
if err != nil {
return err
}

spec := &api.MessageSendSpec{MaxFee: mf}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mf, err := types.BigFromString(cctx.String("max-fee"))
if err != nil {
return err
}
spec := &api.MessageSendSpec{MaxFee: mf}
mf, err := types.ParseFIL(cctx.String("max-fee"))
if err != nil {
return err
}
spec := &api.MessageSendSpec{MaxFee: abi.TokenAmount(mf)}

cmd/lotus-miner/sectors.go Show resolved Hide resolved
fmt.Println(smsg.Cid())
}

fmt.Printf("Totally %d sectors renewed\n", stotal)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "%d sectors renewed"

@zlhwdsz
Copy link
Contributor Author

zlhwdsz commented Aug 16, 2021

@ZenGround0 lint and gen-check got passed. Please also consider #6961 for a more intuitive display of expiration time (and mainnet epoch time in general).

@ZenGround0 ZenGround0 merged commit 2e5b492 into filecoin-project:master Aug 16, 2021
raulk added a commit that referenced this pull request Aug 16, 2021
raulk added a commit that referenced this pull request Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 P2: Should be resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants