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

change the value of duration from endEpoch-startEpoch to sectorExpiry-sectorActivation #237

Conversation

firesWu
Copy link
Contributor

@firesWu firesWu commented Jan 8, 2024

if the value endepoch - startepoch is used as the duration, the calculated initial collateral for real deal may be smaller than the actual value, leading to a failed execution of the provecommit msg execution.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

This makes sense to me. In actors we calculate duration the same way: https://github.com/filecoin-project/builtin-actors/blob/8d957d2901c0f2044417c268f0511324f591cb92/actors/miner/src/lib.rs#L5522-L5534

This code gets used in lotus for StateMinerPreCommitDepositForPower here: https://github.com/filecoin-project/lotus/blob/21abfc69fb552b92fa3ce223e863f1d582dec237/node/impl/full/state.go#L1463-L1468

But I'd like someone else who works more closely with this to verify this is correct. @magik6k @LexLuthr ?

@rvagg rvagg requested review from magik6k and LexLuthr July 15, 2024 02:22
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

Going to request a rebase on master and changes to v14 too. At this stage I think the v12 changes are unnecessary.

@rvagg
Copy link
Member

rvagg commented Jul 15, 2024

@firesWu we have a 👍 on this from the miner code experts, thanks for catching it. Would you mind rebasing and switching your v12 change to v14?

@rvagg
Copy link
Member

rvagg commented Jul 16, 2024

It was easy enough to do in a new PR @ #291

@rvagg
Copy link
Member

rvagg commented Jul 22, 2024

merged as #291, thanks @firesWu

@rvagg rvagg closed this Jul 22, 2024
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.

2 participants