-
Notifications
You must be signed in to change notification settings - Fork 467
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
feat/2841 - compute collateral as a function of sector size + track collateral as actor balance + collateral check before adding new commitments #2865
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.
I have concerns about adding a constructor for miner actors that takes a balance. Other than that, this PR looks solid.
actor/builtin/miner/miner.go
Outdated
func NewActor() *actor.Actor { | ||
return actor.NewActor(types.MinerActorCodeCid, types.NewZeroAttoFIL()) | ||
// NewActor returns a new miner actor with the provided balance. | ||
func NewActor(balance *types.AttoFIL) *actor.Actor { |
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 a little sketchy. This only gets used in tests an genesis, but it suggests you can create a miner actor in a state transition with a non-zero balance. This would be really bad, since it would be generating FIL from nowhere. For clarity's sake, I'd rather you not add balance to the constructor and set it manually after the actor is constructed.
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.
For clarity's sake, I'd rather you not add balance to the constructor and set it manually after the actor is constructed.
OK. This change is easy to roll back. I introduced it to be consistent with the account actor-creation constructor. Do you think that the same logic applies to the account actor?
// given size. | ||
func CollateralForSector(sectorSize *types.BytesAmount) *types.AttoFIL { | ||
// TODO: Replace this function with the baseline pro-rata construction. | ||
// https://github.com/filecoin-project/go-filecoin/issues/2866 |
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.
🙏
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.
LGTM. It's worth checking the wiki and potentially making an update explaining that there is a minimum collateral during the miner setup workflow (though it might not affect anything if users run with the default FIL values).
3857b6d
to
147933b
Compare
Sounds good. I'm about to delete the |
Makes incremental progress towards #2530.
Fixes #2841.
Why does this PR exist?
The spec demonstrates that a miner's collateral is its balance, and the collateral which is has locked up for sectors which it is actively proving must be tracked via the
ActiveCollateral
field. This field's value must be incremented when a sector is committed, and commitments should be rejected if the miner doesn't have enough collateral to cover a new commitment.The spec also demonstrates that per-sector collateral is a function of sector size. We believe that this will change soon (see Pledge Collateral Design Doc (Baseline (Pro-rata) Construction)), but this moves the codebase in the right direction.
What's in this PR?
init
is called)MyBalance
which does what you'd expect (this is a specified function)