-
Notifications
You must be signed in to change notification settings - Fork 153
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
Storage Power actor #308
Storage Power actor #308
Conversation
pub static ref CONSENSUS_MINER_MIN_POWER: StoragePower = StoragePower::from(2 << 30); // placeholder | ||
|
||
} | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we need a catch-all statement here if nothing is hit?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pledgePenaltyForWindowedPoStFailure
is missing? See here in policy.go.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you do this now?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget to remove these comments?
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.
Was just to give context based on the spec impl, but I removed
} | ||
} | ||
|
||
lazy_static! { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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