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

fix: CLI: adjust TermMax for extend-claim used by a different client #11764

Merged
merged 9 commits into from
Mar 28, 2024

Conversation

LexLuthr
Copy link
Contributor

@LexLuthr LexLuthr commented Mar 21, 2024

Related Issues

Proposed Changes

This PR fixes the TermMax for claim extensions requested by a client different than the original client.

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

@LexLuthr
Copy link
Contributor Author

Test with Datacap:

Before

ID  Provider  Client  Data                                                              Size     TermMin  TermMax  TermStart  Sector  
1   1000      102     baga6ea4seaqlsmme4a6x2pfvlstsoh2eyqwfxvfhudcmnf5ndiw56leqa3fiaoa  8388608  3675537  5256000  0          0       
2   1000      1009    baga6ea4seaqpfonfro6j3gcw4uxkxbivlqn2fghx5dpulc6sbi5noz7bcvzmuiq  2048     518400   5256000  13016      1

After

ID  Provider  Client  Data                                                              Size     TermMin  TermMax  TermStart  Sector  
1   1000      102     baga6ea4seaqlsmme4a6x2pfvlstsoh2eyqwfxvfhudcmnf5ndiw56leqa3fiaoa  8388608  3675537  5288893  0          0       
2   1000      1009    baga6ea4seaqpfonfro6j3gcw4uxkxbivlqn2fghx5dpulc6sbi5noz7bcvzmuiq  2048     518400   5275877  13016      1 

New TerMax is more than 5 years

@LexLuthr LexLuthr requested review from rvagg and snadrus March 21, 2024 06:42
@LexLuthr LexLuthr mentioned this pull request Mar 21, 2024
8 tasks
@LexLuthr
Copy link
Contributor Author

CC: @beck-8 Please review the code and provide a +1 if everything looks good.

@beck-8
Copy link
Contributor

beck-8 commented Mar 21, 2024

batchSize to a default value. And it is better if the user can fill in the value by himself/herself

@LexLuthr
Copy link
Contributor Author

@beck-8 I have pushed the changes. Please test it and let me know if any issues.

@LexLuthr
Copy link
Contributor Author

@rvagg Can you please review? I would like to merge before diff starts to grow.

@beck-8
Copy link
Contributor

beck-8 commented Mar 27, 2024

@rvagg Can you please review? I would like to merge before diff starts to grow.

Have you seen my comment? I don’t know if my question is correct.

@LexLuthr LexLuthr requested a review from magik6k March 27, 2024 09:08
@LexLuthr
Copy link
Contributor Author

@beck-8 Which comment? I don't see any comments on this PR.

@beck-8
Copy link
Contributor

beck-8 commented Mar 27, 2024

@beck-8 Which comment? I don't see any comments on this PR.

image

@LexLuthr
Copy link
Contributor Author

@beck-8 You never posted that comment. I think you forgot to publish the review.
Good catch! Working on the refactor to fix this.

cli/filplus.go Outdated Show resolved Hide resolved
cli/filplus.go Outdated Show resolved Hide resolved
cli/filplus.go Outdated Show resolved Hide resolved
cli/filplus.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Mar 28, 2024

This is fine I think. But there seems to be a third option that's not covered here, you have "my claim, extend up to 5 years from start", and "not my claim, extend up to 5 years from now, pay datacap", but not "my claim, extend up to 5 years from now, pay datacap", which I believe is possible. There's no client check when doing a ClaimExtensionRequest and the ability to extend by 5 years from now by paying datacap seems to be something that a client might want to be doing.

Maybe that doesn't have to be in this PR but I can see it being a concern so perhaps think about how that might work in terms of UX and make sure that it'd be additive to the command rather than having to break it.

You have a conflict now with itest/direct_data_onboard_verified_test.go, I did so updates in #11766, they got rewritten to work on release/v1.26.0 and then pulled back in to master. It should be straightforward I think.

@beck-8
Copy link
Contributor

beck-8 commented Mar 28, 2024

i think maybe "my claim, extend up to 5 years from start" = "my claim, extend up to 5 years from now, pay datacap" ?

Normal people will not use parameters less than 5 years.

@rvagg
Copy link
Member

rvagg commented Mar 28, 2024

start != now though, https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0045.md#renewal-of-claim-term-by-spending-datacap, this is the perpetual-storage case where you can keep spending datacap to go beyond max term. I don't think this is urgent, by any means, maybe we have a few years before this will be something that makes sense doing given sector lifetimes, but it seems like a use-case that will at least eventually be desired

@LexLuthr
Copy link
Contributor Author

@rvagg Thank you for the review. I will make all the suggested changes. The third case makes sense in terms of Verifreg actor but Sector lifetime is currently limited to 5 years. I think we should defer it to future when sector lifetime has been extended or resealing is in place.

@LexLuthr LexLuthr requested a review from rvagg March 28, 2024 06:07
@LexLuthr LexLuthr merged commit 0e46776 into master Mar 28, 2024
184 checks passed
@LexLuthr LexLuthr deleted the fix/extend-claim branch March 28, 2024 11:18
ribasushi pushed a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request May 11, 2024
…ilecoin-project#11764)

* fix Datacap TermMax

* fix itest

* add batching

* make batch size variable

* docsgen-cli

* reduce batch size, fix batch dc

* apply suggestion from review

* put back TODO

---------

Co-authored-by: LexLuthr <lexluthr@protocol.ai>
ribasushi pushed a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request May 11, 2024
…ilecoin-project#11764)

* fix Datacap TermMax

* fix itest

* add batching

* make batch size variable

* docsgen-cli

* reduce batch size, fix batch dc

* apply suggestion from review

* put back TODO

---------

Co-authored-by: LexLuthr <lexluthr@protocol.ai>
ribasushi pushed a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request May 16, 2024
…ilecoin-project#11764)

* fix Datacap TermMax

* fix itest

* add batching

* make batch size variable

* docsgen-cli

* reduce batch size, fix batch dc

* apply suggestion from review

* put back TODO

---------

Co-authored-by: LexLuthr <lexluthr@protocol.ai>
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.

None yet

3 participants