Storage Power actor #308
Storage Power actor #308
Conversation
austinabell
requested review from
ansermino,
dutterbutter and
ec2
as code owners
| pub static ref CONSENSUS_MINER_MIN_POWER: StoragePower = StoragePower::from(2 << 30); // placeholder | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Add comment: "Penalty to pledge collateral for the termination of an individual sector."
| fault_type: ConsensusFaultType, | ||
| ) -> TokenAmount { | ||
| // PARAM_FINISH: always penalise the entire pledge. | ||
| match fault_type { |
There was a problem hiding this comment.
Would we need a catch-all statement here if nothing is hit?
There was a problem hiding this comment.
no, because that would fail in serialization. All patterns are matched (rust doesn't let otherwise)
| // PARAM_FINISH | ||
| TokenAmount::new(0) | ||
| } | ||
|
|
There was a problem hiding this comment.
Seems pledgePenaltyForWindowedPoStFailureis missing? See here in policy.go.
There was a problem hiding this comment.
good catch ! I mistakenly referenced the other temporary function
| pub const SECTOR_TERMINATION_MANUAL: SectorTermination = 1; | ||
|
|
||
| #[derive(Clone)] | ||
| pub struct SectorStorageWeightDesc { |
There was a problem hiding this comment.
It might be worth adding comments to each of these types, I'll leave that to your judgment.
There was a problem hiding this comment.
I just don't know enough about them yet to add comments on all of them, I will make a pass through later when more specifics are ironed out
| }) | ||
| .map_err(|e| ActorError::new(ExitCode::ErrIllegalState, e))?; | ||
|
|
||
| // TODO switch 6 to OnDeleteMiner on miner actor impl |
There was a problem hiding this comment.
Can't you do this now?
There was a problem hiding this comment.
I want to keep changes contained, those method definitions don't exist in the miner actor (it's empty rn)
vm/actor/src/builtin/power/policy.rs
Outdated
| // PARAM_FINISH | ||
| // var growthRate = SLASHER_SHARE_GROWTH_RATE_NUM / SLASHER_SHARE_GROWTH_RATE_DENOM | ||
| // var multiplier = growthRate^elapsed_epoch | ||
| // var slasherProportion = min(INITIAL_SLASHER_SHARE * multiplier, 1.0) |
There was a problem hiding this comment.
Did you forget to remove these comments?
There was a problem hiding this comment.
Was just to give context based on the spec impl, but I removed
| } | ||
| } | ||
|
|
||
| lazy_static! { |
There was a problem hiding this comment.
Is there a reason why these arent just declared inside the function? Doesnt look like youre using them anywhere else. Do you think they will in the future?
There was a problem hiding this comment.
Better to have them in a static context and as variables that can more easily be modified in future, are you saying function referring to reward_for_consensus_slash_report ?
There was a problem hiding this comment.
Yeah, thats the function I'm referring to. I'm ok with them being static, though. Doesn't have much impact, and you might be right about future modifications.
Summary of changes
Changes introduced in this pull request:
Sorry for big PR, not much I could do to break up lol
Some logic is changed a bit with this due to the reliance on negative token amounts for logic within the go/spec impl. I also had to clean up some other small auxiliary things, I tried to keep very contained and they should be very unopinionated.
If you notice the deref and ref of the lazy static items, that is because the intended way to access the underlying value is through the deref (cleaner to denote specifically what is happening instead of an implicit deref through the let binding, I should have read the docs before using lol)
Reference issue to close (if applicable)
Closes #161
Other information and links