Skip to content

Downtime slashing when epoch changes #2436

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

Merged
celo-ci-bot-user merged 8 commits into from
Jan 24, 2020
Merged

Downtime slashing when epoch changes #2436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
588e8f8
fixing it?
mrsmkl Jan 14, 2020
56a116a
seems to work now
mrsmkl Jan 15, 2020
d660fb6
Merge branch 'master' into downtime-slashing-epoch-change-fix
mrsmkl Jan 15, 2020
9ec4316
Merge branch 'master' of github.com:celo-org/celo-monorepo into downt…
mrsmkl Jan 17, 2020
bb166b3
added comment
mrsmkl Jan 17, 2020
d7ff23a
lint
mrsmkl Jan 17, 2020
2b0dca8
Merge branch 'master' into downtime-slashing-epoch-change-fix
mrsmkl Jan 22, 2020
0b2b621
Merge branch 'master' into downtime-slashing-epoch-change-fix
mrsmkl Jan 24, 2020
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
4 changes: 3 additions & 1 deletion packages/protocol/contracts/governance/DowntimeSlasher.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@ contract DowntimeSlasher is SlasherUtil {
uint256 signerIndex = epochNumberOfBlock(n, sz) == startEpoch
? startSignerIndex
: endSignerIndex;
if (uint256(getParentSealBitmap(n)) & (1 << signerIndex) != 0) return false;
// We want to check signers for block n,
// so we get the parent seal bitmap for the next block
if (uint256(getParentSealBitmap(n + 1)) & (1 << signerIndex) != 0) return false;
Copy link
Copy Markdown
Contributor

@mcortesi mcortesi Jan 16, 2020

Choose a reason for hiding this comment

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

So, not sure of the semantics of getParentSealBitmap.
From your change it seems that getParentSeal(x) will get the parentSeal that's on block X header. which is a seal of all signers for block X-1.

In that case, this is correct.. and great catch by the way!
Can you confirm? The other case is that getParentSealBitmap(x) get's the parent seal that have signer for block X and that lives on block X+1 header..

In any case, a comment would be nice :D

Copy link
Copy Markdown
Contributor Author

@mrsmkl mrsmkl Jan 16, 2020

Choose a reason for hiding this comment

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

I think I'll check from geth, but I tried to check from baklava, the current version didn't seem to work.

Copy link
Copy Markdown
Contributor Author

@mrsmkl mrsmkl Jan 17, 2020

Choose a reason for hiding this comment

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

It gets the header from the block given as argument https://github.com/celo-org/celo-blockchain/blob/master/core/vm/contracts.go#L797

So it should be correct now.

Copy link
Copy Markdown
Contributor

@asaj asaj Jan 24, 2020

Choose a reason for hiding this comment

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

Do we need to change require(endBlock < block.number - 1, "end block must be smaller than current block");?

Copy link
Copy Markdown
Contributor Author

@mrsmkl mrsmkl Jan 24, 2020

Choose a reason for hiding this comment

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

No, I think it's correct now.

}
return true;
}
Expand Down
32 changes: 29 additions & 3 deletions packages/protocol/test/governance/downtime_slasher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { CeloContractName } from '@celo/protocol/lib/registry-utils'
import { assertContainSubset, assertRevert } from '@celo/protocol/lib/test-utils'
import { assertContainSubset, assertRevert, jsonRpc } from '@celo/protocol/lib/test-utils'
import BigNumber from 'bignumber.js'
import {
AccountsContract,
Expand Down Expand Up @@ -37,7 +37,7 @@ contract('DowntimeSlasher', (accounts: string[]) => {

const slashingPenalty = 10000
const slashingReward = 100
const slashableDowntime = 3
const slashableDowntime = 10

beforeEach(async () => {
accountsInstance = await Accounts.new()
Expand Down Expand Up @@ -70,7 +70,7 @@ contract('DowntimeSlasher', (accounts: string[]) => {
})
it('should have set slashable downtime', async () => {
const res = await slasher.slashableDowntime()
assert.equal(res.toNumber(), 3)
assert.equal(res.toNumber(), slashableDowntime)
})
it('can only be called once', async () => {
await assertRevert(slasher.initialize(registry.address, 10000, 100, 2))
Expand Down Expand Up @@ -132,28 +132,54 @@ contract('DowntimeSlasher', (accounts: string[]) => {
describe('#slash()', () => {
let blockNumber: number
let startBlock: number
let changeBlock: number
const validatorIndex = 0
before(async () => {
let bn: number = 0
do {
bn = await web3.eth.getBlockNumber()
await jsonRpc(web3, 'evm_mine', [])
} while (bn < 350)
})
beforeEach(async () => {
blockNumber = await web3.eth.getBlockNumber()
startBlock = blockNumber - 50
const epoch = (await slasher.getEpochNumberOfBlock(blockNumber)).toNumber()
await slasher.setEpochSigner(epoch, validatorIndex, validator)
await slasher.setEpochSigner(epoch - 1, validatorIndex, validator)
await slasher.setEpochSigner(epoch - 2, 1, validator)
await slasher.setEpochSigner(epoch + 1, validatorIndex, validator)
// Signed by validators 0 and 1
const bitmap = '0x0000000000000000000000000000000000000000000000000000000000000003'
// Signed by validator 1
const bitmap2 = '0x0000000000000000000000000000000000000000000000000000000000000002'
// Signed by validator 0
const bitmap3 = '0x0000000000000000000000000000000000000000000000000000000000000001'
await slasher.setParentSealBitmap(blockNumber, bitmap)
await slasher.setParentSealBitmap(blockNumber + 1, bitmap)
await slasher.setParentSealBitmap(blockNumber + 2, bitmap2)
await slasher.setNumberValidators(2)
// when epoch-2 changes to epoch-1, the validator to be slashed is down, but our signer number changes
// another validator is up around the epoch change
changeBlock = (epoch - 1) * 100 - 3
async function prepareBlock(bn) {
const parentEpoch = (await slasher.getEpochNumberOfBlock(bn - 1)).toNumber()
await slasher.setParentSealBitmap(bn, parentEpoch === epoch - 2 ? bitmap3 : bitmap2)
}
for (let i = 0; i < 7; i++) {
await prepareBlock(changeBlock + i)
}
})
it('fails if they were signed', async () => {
await assertRevert(
slasher.slash(blockNumber, validatorIndex, validatorIndex, 0, [], [], [], [], [], [])
)
})
it('success with validator index change', async () => {
await slasher.slash(changeBlock, 1, validatorIndex, 0, [], [], [], [], [], [])
const balance = await mockLockedGold.accountTotalLockedGold(validator)
assert.equal(balance.toNumber(), 40000)
})
it('decrements gold when success', async () => {
await slasher.slash(startBlock, validatorIndex, validatorIndex, 0, [], [], [], [], [], [])
const balance = await mockLockedGold.accountTotalLockedGold(validator)
Expand Down