Slashing Mechanism #383
Slashing Mechanism #383
Conversation
| miner := AuthorOf(block1) | ||
|
|
||
| // TODO: Some of the slashed collateral should be paid to the slasher | ||
| // Check if miner has been slashed | ||
| if !self.Miners.Has(miner) { |
There was a problem hiding this comment.
a miner is removed from the pool on slashing? this surprises me.
There was a problem hiding this comment.
Yes, this is what it currently is in the spec but likely to be changed. cc @whyrusleeping
There was a problem hiding this comment.
For consensus slashing, yes. the miner is removed from the set of valid miners.
There was a problem hiding this comment.
for storage fault slashing however, this should not be the case
There was a problem hiding this comment.
@decentralion since we are slashing for consensus fault, a miner is removed from the pool in this case.
actors.md
Outdated
|
|
||
| // Burn all of the miners collateral | ||
| miner.BurnCollateral() | ||
|
|
||
| // const slashedCollateral = self.GetCollateral(miner) |
There was a problem hiding this comment.
remove commented code
There was a problem hiding this comment.
will do after @anorth confirms that miner.Balance is the right way to get its value and we can directly set miner.Balance.
There was a problem hiding this comment.
self.Balance should be fine for accessing it, but in order to remove funds, they need to be moved somewhere. If burning funds, use BurnFunds
There was a problem hiding this comment.
The balance is not the same as the collateral requirement. If a miner has more balance than required for collateral, only the collateral amount should be burnt. This should use CollateralForPower(miner.power)
actors.md
Outdated
| miner.BurnCollateral() | ||
|
|
||
| // const slashedCollateral = self.GetCollateral(miner) | ||
| const slashedCollateral = miner.Balance |
There was a problem hiding this comment.
I thought we weren't slashing the entire balance on a fault? Can we leave a placeholder here for the amount slashed, e.g. something like:
miner.Balance -= slashingAmountForFault(fault)
There was a problem hiding this comment.
@decentralion I think we might be slashing the entire amount here if it is consensus fault. Otherwise, we have to decide how much to slash. cc @whyrusleeping
There was a problem hiding this comment.
A miner's balance probably be greater than their collateral requirements, but could be less. We should slash the min of those two. IMO we should not burn more than the miner's collateral requirement just because they happen to have that on balance.
There was a problem hiding this comment.
@anorth resolved in latest commit
actors.md
Outdated
|
|
||
| // Burn all of the miners collateral | ||
| miner.BurnCollateral() | ||
|
|
||
| // const slashedCollateral = self.GetCollateral(miner) |
There was a problem hiding this comment.
The balance is not the same as the collateral requirement. If a miner has more balance than required for collateral, only the collateral amount should be burnt. This should use CollateralForPower(miner.power)
actors.md
Outdated
| const blockElapsed = VM.CurrentBlockHeight() - block1.Height | ||
| const GROWTH_RATE = 1.26 | ||
| const INITIAL_SHARE = 0.001 | ||
| const SLASHER_SHARE = Min(Pow(INITIAL_SHARE, GROWTH_RATE), 1.0) |
There was a problem hiding this comment.
I think this should be:
slasherSlash = min( INITIAL_SHARE * Pow(growthRate, blockElapsed), 1.0)
right?
There was a problem hiding this comment.
(blockElapsed should definitely be involved somehow)
There was a problem hiding this comment.
We're also going to have to be careful here, floating point precision and rounding needs to be very explicitly defined.
There was a problem hiding this comment.
Not exactly sure on the right floating point precision and rounding here though
|
@zixuanzh The last updates were in Sept, around the time of the big spec overhaul. Is this PR stale or still active? |
|
I was under the impression that this has been added to the current spec but looks like it was left as a TODO. I will author the change here into the current spec. cc @sternhenri @jzimmerman |
|
Added in #789 |
Not exactly sure for the following,