Unified equivocation slashing #1737
Conversation
See https://github.com/ethereum/eth2.0-specs/issues/1387—thanks to @hwwhww for pinging me :) **TLDR**: Significant simplification, especially for phase 1. The idea is that all forms of equivocation (beacon proposing, shard proposing, shard attesting) are unified in a single equivocation slashing condition using uniqueness roots. **Substantive changes**: * Introduce `uniqueness_root` in signing root * Replace `process_proposer_slashing` by `process_equivocation` **Cosmetic changes**: * Move `BeaconBlockHeader` aside `BeaconBlockBody` and `BeaconBlock` * Rename `SigningRoot` to `SigningData` * Remove a couple of unnecessary new lines
|
Hi @JustinDrake is there a strong rationale why this absolutely needs to be included in phase 0? These changes, albeit small in text, entail a significant burden for client developers (i.e. we have to create and maintain another feature branch that breaks consensus, has thousands of lines of code / tests / generated protos). These changes end up distracting us for 1 or 2 more weeks from key improvements on what we thought was the last, consensus-breaking spec change before mainnet barring critical bugs. We are already quite busy in terms of improving our client's testnet resilience and user experience today. If there is no urgent need for this in phase 0, we kindly ask on behalf of @prylabs that we move this to phase 1. |
|
I reviewed this with other members of @prysmaticlabs and I share the opinion expressed above. However, I would like to add a bit more to our feedback. Having a generic function for equivocation slashing is a great idea. Had this been 6 months ago then I don't think anyone would be debating this. The issue now is that we are already in a code freeze. When the spec was unfrozen the first time, we agreed these significant changes were worth it. We've spent months building with an evolving spec and now we're finally just weeks away from The Multiclient Testnet followed by The Mainnet Release. Now, in the face of unfreezing another frozen spec, do you all think this is worth it? |
Agreed—I kinda dropped the ball on this one 😢
As of now we know of 3 types:
It's possible more types of equivocation will be required for phase 2. |
|
The testing infrastructure we have in clients and in the spec today should allow us to make this change relatively pain-free - we're fine either way on the Nimbus side - worth considering that it's easier to introduce this now than in the phase1 HF for example. |
We agree that this is relatively easier than the previous unfreezing of the spec, however I estimate that this change will delay a mainnet release by 2 to 4 weeks, at least. As all things in software engineering, whether or not we move forward on this is a trade off that must be carefully calculated. |
|
Closing in favour of #1758 which is easier to review |
See #1387. Special thanks to @hwwhww for pinging me :)
TLDR: Unify all forms of slashing from equivocation (in particular equivocation through beacon proposing in phase 0, or through shard proposing and shard attesting in phase 1) into a single equivocation slashing condition.
rationale: This is a significant simplification to the slashing logic for phase 1. Each slashing condition costs about 50 lines of consensus code and needs to be separately tested. All in all we save about 100 lines of consensus code for phase 1. Avoiding two slashing conditions in phase 1 correspondingly simplifies testing.
substantive changes:
equivocation_rootin signing rootprocess_proposer_slashingbyprocess_equivocationcosmetic changes:
BeaconBlockHeaderasideBeaconBlockBodyandBeaconBlockSigningRoottoSigningDatacc @vbuterin, @djrtwo