[protocol] DowntimeSlasherV2

[gastonponti]

Description

New contract for downtime slashing using slots [CIP-0007]
The actual implementation is running out of gas

Related issues

• Fixes Downtime slashing should be possible with RC1 gas limit #3664

Backwards compatibility

Ok, new contract

[nategraf]

One strange behavior of this implementation is that the slashable downtime is effectively rounded up to the nearest slot size (i.e. ceiling(slashableDowntime / slotSize) * slotSize. We should find a way to fix that or at least require slashableDowntime % slotSize == 0.

One solution to that problem, which I believe has some nice properties, would be to allow the user to calculate the accumulator for arbitrary slots (i.e. let them choose both start block and end block for each slot). It would also be nice because it would allow users to calculate slot incrementally in as small or large chunks as they want (within the gas limit) and would not require manual intervention if the block gas limit or costs ever did change such that the slot size should be increased or decreased.

[asaj]

I had a bit of trouble following some of the code at first. I think allowing reporters to specify the intervals (slots) and requiring that those intervals do not span epoch boundaries would help with readability.

What do you think of the following spec?

uint256 public slashableDowntime;
// Maps start block -> end block -> bitmap
// Intervals are not allowed to cross epoch boundaries.
mapping(uint256 => mapping(uint256 => uint256)) bitmaps;
// Maps validator address -> epoch number -> whether the validator has been slashed (or not) for downtime starting in that epoch.
mapping(address => mapping(uint256 => boolean)) slashed;

// Calculates and returns the accumulated bitmap for the specified interval.
function getBitmapForInterval(uint256 start, uint256 end) external view returns (uint256);

// Calculates and stores the accumulated bitmap for the specified interval.
function setBitmapForInterval(uint256 start, uint256 end) external returns (uint256);

// Reads the stored accumulated bitmap for the specified interval and returns whether the specified validator signed one or more blocks .
function wasDownForInterval(uint256 start, uint256 end, uint256 signerIndex) external view returns (bool);

// Given a start block, validator, and a list of contiguous intervals of cumulative length >= `slashableDowntime` for which bitmaps have been set, slashes the validator if they did not sign any blocks in those intervals.
function slash(uint256 startBlock, uint256 intervalEndBlocks[], uint256[] signerIndices, uint256 groupMembershipHistoryIndex...

[asaj]

Continuing a conversation from #3806 (comment)

You're right that if a validator is down for >= 2*slashableDowntime, slashing them for the second offense will prevent them from being slashed for the first. IMO this is okay, as we can expect folks to run slashing bots that will slash validators as soon as they've been down for at least slashableDowntime, and so it's unlikely that any validator would be down long enough to get slashed twice without being slashed once.

Wdyt? cc @m-chrzan to get his thoughts as well

[m-chrzan]

Continuing a conversation from #3806 (comment)

You're right that if a validator is down for >= 2*slashableDowntime, slashing them for the second offense will prevent them from being slashed for the first. IMO this is okay, as we can expect folks to run slashing bots that will slash validators as soon as they've been down for at least slashableDowntime, and so it's unlikely that any validator would be down long enough to get slashed twice without being slashed once.

Wdyt? cc @m-chrzan to get his thoughts as well

Yep, I'd agree with that. We depend on reporters for slashes to take effect anyways, so making the assumption that they're going to do it relatively ASAP (which they're incentivized to do) doesn't feel like a huge leap.

[asaj]

Yep, I'd agree with that. We depend on reporters for slashes to take effect anyways, so making the assumption that they're going to do it relatively ASAP (which they're incentivized to do) doesn't feel like a huge leap.

Nice, I think this would simplify the code quite a bit

[asaj]

Smart contract is looking great! Just some very minor nits around readability

[asaj]

Smart contract LGTM!

[yorhodes]

this is cool! great work @gastonponti

[yorhodes]

any way we can document/profile the gas differences here?

[yorhodes]

ie at what point will this fail with a sufficiently large slashableDowntime

[gastonponti]

It depends on how big were the intervals checked before.
We basically pre-compute as different actions every interval, which also have a few optimisations, so I remember that we could almost cover the 8hrs window. So, let's say that you made 6hrs intervals just to be sure.
That wasDownForIntervals basically check those precompute intervals, and that's not expensive, so this should support an order of months or even years.

The only true limitation today is that we use the parentSealBitmap to compute those intervals, and we only keep track of the last 4 epochs. So if you want to slash with a slashableDowntimes > 4 epochs, the user should pre-compute every interval without knowing if the validator is going to be down, and almost "betting" that the user will stay down.

[gastonponti]

Made some numbers.
Intervals of 5k blocks work great. And the intervals verifier for 1000 intervals, it's also works like a charm, and consumes ~6.5M, si it could be even more.
So basically could work for even more than 5M blocks for slashableDowntime

[m-chrzan]

Looks great, just a few style/naming nitpicks, other than that ready to go!

[m-chrzan]

Naming suggestion: n -> block.

[gastonponti]

change it to blockNumber to avoid confusions with block global

[nategraf]

Looks great overall ✅ 🚢 . Here are some smaller comments.

[nategraf]

Ideally this functions interface could stay the same and we would figure out the intervals internal to this function. If the given validator was down for the whole window, the interval sizes don't really matter, so this function could provide reasonable defaults (e.g. 1000 blocks per interval)

Alternatively, this function could change and a new function could be created to provide a simple interface to execute a slash. Currently, it looks like any user interested in executing a slash needs to have a pretty intimate understanding of the internal details so they can post all the transactions they need to set the bitmaps before calling this function.

Note: I would not say this blocks the PR from being merged, but we will want to provide a nicer interface when possible so that making slashing bots is easy.

[asaj]

I'm not sure I agree, this seems like the sort of off-chain logic that should be done in contractkit rather than internal to the contract

[gastonponti]

Yes, that was something that we prioritised in the changes.
Use the contract as the simpler tool possible (with some limitations), but cover those limitations in the wrappers tools that we use.

[nategraf]

We should clarify the parameters below. It could either be clarified inline or we could link somewhere.

[gastonponti]

Well, this is using the SlasherUtil.sol, but it doesn't have any documentation.
I could change the docs for the SlasherUtil, but will be different from the deployed one, that shouldn't be a problem but I don't know if we assuming a version change for comments (@asaj)
The other thing that I could do, is to add a link to the slash function in the lockedGold
contract, that's basically what the SlasherUtils end up calling for the validator, and for the group. WDYT?

[nategraf]

I suggest moving this line to the first line of this function and optionally dropping the first two requires checks, as they are redundant with the ones in this function. (I could also see an argument for keeping the requires checks for readability)

[gastonponti]

I think this comment was done in the middle of a commit or something.
Correct me if I'm wrong @nategraf but probably this was when I was checking the length of the arrays (startBlocks and endBlocks)

[m-chrzan]

Looking at the latest CircleCI run, weird that there's a failure in Attestations - don't think you've touched anything related to that test. Is that a known flake?