Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

grandpa: don't send equivocation reports for local identities #7372

Merged
3 commits merged into from
Oct 26, 2020

Conversation

andresilva
Copy link
Contributor

There's no need to send equivocation reports for keys that we control:

  • If the equivocation went to the network then someone else will report it;
  • Otherwise there was no harm done so the slash is unnecessary.

This can happen (and has happened) when node operators restore the database from a backup and lose the GRANDPA voter state. They might end up equivocating on an old round for which no one in the network is listening anymore (and therefore the equivocation is harmless and would go unreported), but they end up reporting themselves and getting slashed.

@andresilva andresilva added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Oct 21, 2020
this works on rust nightly but breaks on ci which is using rust stable
@andresilva andresilva force-pushed the andre/grandpa-never-self-report branch from 24eb825 to 1a5c4c8 Compare October 22, 2020 00:02
@andresilva andresilva requested a review from octol October 22, 2020 00:33
@burdges
Copy link

burdges commented Oct 22, 2020

It's harmless I'm sure, and even helpful, but not really a fix if I understand, right? We need grandpa to recognize when its voter state looks tainted relative to the database.

We’ve a “slashing reform” design https://hackmd.io/@rgbPIkIdTwSICPuAq67Jbw/BkCOQ8CvP that should basically end slashing of honest nodes across the board. It meshes nicely with two other problems: our session keys never sign their own certificates, and some future session keys require proofs-of-possession. It'd benefit from more formalization probably, but basically workable.

I'm not arguing against this change, but I wanted anyone interested to know.

@andresilva
Copy link
Contributor Author

@burdges Yeah this is just small patchwork to avoid some class of "benign" equivocations. I've read through the document you've sent and I agree some mechanism like that is the proper way to go for preventing these kinds of equivocations caused by operational errors. I have created an issue for this (#7398) and linked to the document you posted.

@andresilva
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Oct 26, 2020

Trying merge.

@ghost ghost merged commit 891900a into master Oct 26, 2020
@ghost ghost deleted the andre/grandpa-never-self-report branch October 26, 2020 12:47
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants