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.

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

do not chill indirectly-slashed nominators #4553

Merged
merged 2 commits into from Jan 7, 2020
Merged

Conversation

@rphmeier
Copy link
Member

rphmeier commented Jan 7, 2020

cc @burdges, @jacogr

When a validator is slashed, its nominators are currently kicked until explicit re-nomination. This PR causes that not to happen. The phragmén election already pre-filters for votes cast after a validator's most recent slash, so the nominators' votes on the slashed validator will be ignored until next explicit nomination.

The UX here is not ideal, since those votes on slashed validators will be silently ignored. User apps will be able to perform the same check as the staking code - I've spoken with @jacogr about this before, but pinging again here.

@rphmeier rphmeier requested a review from kianenigma as a code owner Jan 7, 2020
@gavofyork

This comment has been minimized.

Copy link
Member

gavofyork commented Jan 7, 2020

No tests to correct?

@burdges

This comment has been minimized.

Copy link

burdges commented Jan 7, 2020

We've worked over the trade offs here fairly well, so if you think this makes sense then go for it. We should ideally avoid them being able to reoffend for free in future of course, but I gather this is happening in a way that makes that not happen.

Is there some weaker state of which we should elevate the importance? We'd some scheme for chilling them only when they got slashed enough for example. Or just not worry about it for now?

@rphmeier

This comment has been minimized.

Copy link
Member Author

rphmeier commented Jan 7, 2020

We should ideally avoid them being able to reoffend for free in future

Is that a risk introduced by this PR? This approximate behavior could always be emulated in the old system by ensuring that a nomination transaction immediately following the slash report.

We'd some scheme for chilling them only when they got slashed enough for example

At some point I guess, but the complexity of tracking so much state for each nominator sounds like a lot.

@gavofyork gavofyork merged commit 9178b42 into master Jan 7, 2020
13 checks passed
13 checks passed
continuous-integration/gitlab-cargo-check-benches Build stage: test; status: success
Details
continuous-integration/gitlab-cargo-check-subkey Build stage: test; status: success
Details
continuous-integration/gitlab-check-line-width Build stage: test; status: success
Details
continuous-integration/gitlab-check-runtime Build stage: test; status: success
Details
continuous-integration/gitlab-check-web-wasm Build stage: test; status: success
Details
continuous-integration/gitlab-check_warnings Build stage: build; status: success
Details
continuous-integration/gitlab-node-exits Build stage: test; status: success
Details
continuous-integration/gitlab-test-dependency-rules Build stage: test; status: success
Details
continuous-integration/gitlab-test-frame-staking Build stage: test; status: success
Details
continuous-integration/gitlab-test-full-crypto-feature Build stage: test; status: success
Details
continuous-integration/gitlab-test-linux-stable Build stage: test; status: success
Details
continuous-integration/gitlab-test-linux-stable-int Build stage: test; status: success
Details
continuous-integration/gitlab-test-wasmtime Build stage: test; status: success
Details
@gavofyork gavofyork deleted the rh-dont-kick-nominators branch Jan 7, 2020
gavofyork added a commit that referenced this pull request Jan 7, 2020
* do not chill indirectly-slashed nominators

* test nomination non-kick and vote ignoring behavior
@burdges

This comment has been minimized.

Copy link

burdges commented Jan 8, 2020

Is that a risk introduced by this PR?

No. Indirectly is already additive and their span ends with the era making the next era additive too.

I think the primary concern is that it makes it more profitable to run modified code that delays slashing transactions, but that's a concern that will not pop up in the wild for at least a year or so.

At some point I guess, but the complexity of tracking so much state for each nominator sounds like a lot.

I've now forgotten if this required tracking more state. I'd believed it was equivalent at some point, but you maybe found something else that needed tracking if I recall? If so, I do not remember cleanly sorting out cleanly if that record sticks with other records in the tree (cheap) or floats off elsewhwere (expensive).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.