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.

Sign up for GitHub

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

Slashing Mechanism #383

Closed
wants to merge 5 commits into from
Closed

Slashing Mechanism #383

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e9b727a
slashing mechanism first stab
zixuanzh Jul 3, 2019
15c3c92
update balance getter setter
zixuanzh Jul 8, 2019
cd25479
slashing mechanism cleanup
zixuanzh Jul 27, 2019
ac5e481
update slash amount calc input
zixuanzh Jul 28, 2019
a5f65e4
update slasher share calc
zixuanzh Sep 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions actors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,13 +324,35 @@ func SlashConsensusFault(block1, block2 BlockHeader) {
Fatal("blocks do not prove a slashable offense")
}

if !self.Miners.Has(msg.From) {
Fatal("slash collateral must only be called by a miner actor")
zixuanzh marked this conversation as resolved.
Show resolved Hide resolved
}

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

a miner is removed from the pool on slashing? this surprises me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is what it currently is in the spec but likely to be changed. cc @whyrusleeping

Copy link
Member

Choose a reason for hiding this comment

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

For consensus slashing, yes. the miner is removed from the set of valid miners.

Copy link
Member

Choose a reason for hiding this comment

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

for storage fault slashing however, this should not be the case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@decentralion since we are slashing for consensus fault, a miner is removed from the pool in this case.

Fatal("miner has been slashed")
}

// Burn all of the miners collateral
miner.BurnCollateral()

// const slashedCollateral = self.GetCollateral(miner)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented code

Copy link
Contributor Author

@zixuanzh zixuanzh Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

will do after @anorth confirms that miner.Balance is the right way to get its value and we can directly set miner.Balance.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

const slashedCollateral = miner.Balance
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anorth resolved in latest commit

// self.SetCollateral(miner, 0)
miner.Balance = 0.0
zixuanzh marked this conversation as resolved.
Show resolved Hide resolved

// Some of the slashed collateral should be paid to the slasher
// growthRate determines how fast the slasher share of slashed collateral will increase as block elapses
// TODO parameter selection on growthRate and initialShare
// Current growthRate results in slasherShare reaches 1 after 30 blocks
const blockElapsed = VM.CurrentBlockHeight() - block1.Height
const growthRate = 1.26
zixuanzh marked this conversation as resolved.
Show resolved Hide resolved
const initialShare = 0.001
const slasherShare = Min(Pow(initialShare, growthRate), 1.0)
zixuanzh marked this conversation as resolved.
Show resolved Hide resolved
// self.SetCollateral(msg.From, self.GetCollateral(msg.From) + slasherShare * slashedCollateral)
msg.From.Balance = msg.From.Balance + slasherShare * slashedCollateral
zixuanzh marked this conversation as resolved.
Show resolved Hide resolved
self.BurnCollateral((1.0 - slasherShare) * slashedCollateral)
zixuanzh marked this conversation as resolved.
Show resolved Hide resolved

// Remove the miner from the list of network miners
self.Miners.Remove(miner)
self.UpdateStorage(-1 * miner.Power)
Expand Down