runtime node slashing (ADR 0005) #3640

ptrus · 2021-01-19T16:39:02Z

roothash/api: add evidence types
tendermint/roothash: add tx endpoint for submitting evidence of equivocation
roothash: add tests for the new evidence submission
roothash/pool: slash non-majority for SlashRuntimeIncorrectResults
e2e tests
configurable amount of rewards should be awarded to nodes contributing evidence (or resolving discrepancies), add per runtime staking parameters
rewards to nodes should be escrowed (currently these are awarded into the general balance)

go/consensus/tendermint/apps/roothash/slashing.go

codecov · 2021-01-20T10:31:35Z

Codecov Report

Merging #3640 (253376f) into master (9cdfa91) will increase coverage by 0.10%.
The diff coverage is 75.95%.

@@            Coverage Diff             @@
##           master    #3640      +/-   ##
==========================================
+ Coverage   66.66%   66.77%   +0.10%     
==========================================
  Files         393      394       +1     
  Lines       38373    38821     +448     
==========================================
+ Hits        25581    25922     +341     
- Misses       9119     9157      +38     
- Partials     3673     3742      +69

Impacted Files	Coverage Δ
go/registry/api/runtime.go	`44.72% <0.00%> (-1.14%)`	⬇️
go/roothash/api/commitment/pool.go	`71.02% <ø> (ø)`
go/oasis-node/cmd/registry/runtime/runtime.go	`58.49% <57.14%> (-0.07%)`	⬇️
...o/consensus/tendermint/apps/staking/state/state.go	`60.98% <58.33%> (+2.57%)`	⬆️
go/consensus/tendermint/apps/roothash/slashing.go	`62.26% <62.26%> (ø)`
go/roothash/api/api.go	`76.25% <79.31%> (+8.06%)`	⬆️
...consensus/tendermint/apps/roothash/transactions.go	`69.52% <81.48%> (+14.48%)`	⬆️
go/consensus/tendermint/apps/roothash/roothash.go	`75.25% <84.00%> (+1.26%)`	⬆️
.../consensus/tendermint/apps/roothash/state/state.go	`74.48% <88.88%> (+0.73%)`	⬆️
go/roothash/tests/tester.go	`88.70% <90.00%> (+0.18%)`	⬆️
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cdfa91...253376f. Read the comment docs.

go/consensus/tendermint/apps/roothash/roothash.go

go/consensus/tendermint/apps/roothash/slashing.go

go/consensus/tendermint/apps/roothash/state/state.go

go/consensus/tendermint/apps/roothash/transactions.go

go/staking/api/slashing.go

go/consensus/tendermint/apps/roothash/roothash.go

docs/adr/0005-runtime-compute-slashing.md

go/consensus/tendermint/apps/roothash/slashing.go

pro-wh · 2021-02-08T21:42:48Z

go/consensus/tendermint/apps/roothash/transactions.go

+		return err
+	}
+
+	if len(rtState.Runtime.Staking.Slashing) == 0 {


Go doesn't require this extra check, right? we already rely on the Slashing[xxx] to return a zeroed Slash struct if it's not found, and then we return the same error

But it could be nil and derefing a nil map is an error. In case of Slashing from the (global) staking consensus parameters, the Slashing() state accessor method returns a non-nil map in case the parameters are unset/nil but here this is not the case.

https://golang.org/ref/spec#Index_expressions language spec says reading from a nil map gives the zero value

go/roothash/api/api.go

pro-wh · 2021-02-08T22:44:25Z

go/consensus/tendermint/apps/roothash/slashing.go

-	// TODO: part of slashed amount (configurable) should be transferred to the transaction submitter.
+	// If the caller is a node, distribute slashed funds to the controlling entity instead of the
+	// caller directly.
+	rewardAddr := ctx.CallerAddress()


I wonder if people frontrun transactions like these, to take the reward for themselves

Yeah that's a valid concern. Incentivizing evidence submission seems like it introduces more problems than it solves :-)

go/oasis-node/cmd/registry/runtime/runtime.go

go/roothash/api/commitment/pool.go

go/registry/api/runtime.go

ptrus · 2021-02-09T11:22:55Z

go/consensus/tendermint/apps/roothash/slashing.go

+		return fmt.Errorf("tendermint/roothash: runtimeAccReward.Quo(100): %w", err)
+	}
+	runtimeAddr := staking.NewRuntimeAddress(runtimeID)
+	if _, err := stakeState.TransferFromCommon(ctx, runtimeAddr, runtimeAccReward, false); err != nil {


Currently these rewards ignore the commission schedule and the whole reward is split between all delegators of the entity, is this fine or should the owning entity be able to take commission?

(note: this comment should be in line 193, here we transfer to the runtime account anyway)

Hm, actually currently the delegators get nothing, it is treated the same as if the entity just increased its self-delegation. Do we want the delegators to get a cut as well?

Oh right yeah i miss-checked that. I think that probably yes? (since if the node is slashed, delegators do lose proportionately). But i'm not sure if it's currently worth the additional complexity. Maybe a good future improvement. I would at least add some notes that runtime slashing rewards are not rewarded to delegators.

Makes sense, I changed the code to take all delegators and commission into account.

ptrus

✔️ Thanks for finishing the PR (can't actually approve as i'm the author).

ptrus · 2021-02-09T16:39:27Z

go/consensus/tendermint/apps/staking/state/state.go

+	}
+
+	// If escrow is requested, escrow the transferred stake immediately.
+	if escrow {


Hm I think (not 100% sure, didn't check in detail) we can now also simplify the AddRewards method by calling TransferFromCommon from the accounts loop.

go/consensus/tendermint/apps/staking/state/state.go

ptrus force-pushed the ptrus/feature/runtime-slashing branch from f2d964e to 0ab8e88 Compare January 19, 2021 16:39

kostko reviewed Jan 19, 2021

View reviewed changes

go/consensus/tendermint/apps/roothash/slashing.go Outdated Show resolved Hide resolved

ptrus force-pushed the ptrus/feature/runtime-slashing branch 3 times, most recently from 71c815f to a37dc25 Compare January 20, 2021 08:37

ptrus force-pushed the ptrus/feature/runtime-slashing branch 4 times, most recently from 8f89667 to 030b662 Compare January 21, 2021 10:10

ptrus mentioned this pull request Jan 21, 2021

slashing: rename DoubleSigning to ConsensusEquivocation #3646

Merged

ptrus force-pushed the ptrus/feature/runtime-slashing branch 10 times, most recently from 849b34f to 4fde045 Compare January 22, 2021 14:29

kostko reviewed Jan 22, 2021

View reviewed changes

ptrus force-pushed the ptrus/feature/runtime-slashing branch 2 times, most recently from dd78a6f to 879b43e Compare January 25, 2021 22:17

kostko reviewed Jan 28, 2021

View reviewed changes

go/consensus/tendermint/apps/roothash/roothash.go Show resolved Hide resolved

kostko force-pushed the ptrus/feature/runtime-slashing branch 5 times, most recently from c75ee0f to 6e4096e Compare February 5, 2021 14:04

kostko force-pushed the ptrus/feature/runtime-slashing branch 5 times, most recently from d7f6eec to 5d1c759 Compare February 8, 2021 11:55

kostko marked this pull request as ready for review February 8, 2021 12:00

kostko requested review from peterjgilbert, pro-wh and Yawning as code owners February 8, 2021 12:00

pro-wh approved these changes Feb 8, 2021

View reviewed changes

kostko mentioned this pull request Feb 9, 2021

Implement runtime compute slashing (ADR 0005) #3608

Closed

4 tasks

kostko force-pushed the ptrus/feature/runtime-slashing branch from 5d1c759 to 1f87036 Compare February 9, 2021 10:48

ptrus commented Feb 9, 2021

View reviewed changes

go/registry/api/runtime.go Show resolved Hide resolved

ptrus commented Feb 9, 2021

View reviewed changes

ptrus added 2 commits February 9, 2021 12:47

go/roothash: Runtime compute node equivocation slashing

c22fef1

go/roothash/tester: add submit equivocation evidence test

e0fc01b

kostko force-pushed the ptrus/feature/runtime-slashing branch 4 times, most recently from 361e4c2 to 97a1aa3 Compare February 9, 2021 13:29

ptrus commented Feb 9, 2021

View reviewed changes

kostko force-pushed the ptrus/feature/runtime-slashing branch 3 times, most recently from 432d32e to 9dd2bbd Compare February 10, 2021 09:28

go/roothash: Slash runtime compute nodes for incorrect results

253376f

kostko force-pushed the ptrus/feature/runtime-slashing branch from 9dd2bbd to 253376f Compare February 10, 2021 09:57

kostko merged commit 6e8ec70 into master Feb 10, 2021

kostko deleted the ptrus/feature/runtime-slashing branch February 10, 2021 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime node slashing (ADR 0005) #3640

runtime node slashing (ADR 0005) #3640

ptrus commented Jan 19, 2021 •

edited by kostko

codecov bot commented Jan 20, 2021 •

edited

pro-wh Feb 8, 2021

kostko Feb 9, 2021

pro-wh Feb 10, 2021

pro-wh Feb 8, 2021

kostko Feb 9, 2021

ptrus Feb 9, 2021

ptrus Feb 9, 2021 •

edited

kostko Feb 9, 2021

ptrus Feb 9, 2021 •

edited

kostko Feb 9, 2021

ptrus left a comment

ptrus Feb 9, 2021

runtime node slashing (ADR 0005) #3640

runtime node slashing (ADR 0005) #3640

Conversation

ptrus commented Jan 19, 2021 • edited by kostko

codecov bot commented Jan 20, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptrus Feb 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptrus Feb 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptrus commented Jan 19, 2021 •

edited by kostko

codecov bot commented Jan 20, 2021 •

edited

ptrus Feb 9, 2021 •

edited

ptrus Feb 9, 2021 •

edited