[Proposal] rework rewards/penalties to be more granular #1747

djrtwo · 2020-04-23T16:27:47Z

More granular rewards/penalty function to allow us to write and publish tests specific to each of source, target, head, inclusion delay, and inactivity leak.

Includes tests for each and a new rewards generator type

hwwhww · 2020-04-23T16:38:11Z

Is this proposal targeting at v011x or v012x?

hwwhww

First quick scan. 👀

I like going back to this more granular pattern (once upon a time, v0.5.0...). And yes, we really need more tests for these. 👍

specs/phase0/beacon-chain.md

Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>

djrtwo · 2020-04-29T15:59:29Z

@protolambda I've done some work to de-duplicate the tests across source, target, head (want to run same scenarios against each essentially).

Curious if you see any less verbose but clean way to further reduce the verbosity here.

Essentially helpers/rewards.py contains the meat of the tests and a separate test file for each source, target, and head call the helper-tests from rewards.py. Problem is that source, target ,and head test files look almost exactly the same but have a different runner being passed in.

edit: and this is all obviously in the context of having good generators

specs/phase0/beacon-chain.md

djrtwo · 2020-05-04T16:36:20Z

One problem I'm thinking about here is that ultimately, for each test case I right for a particular deltas function, it might as well have test vector outputs for each of the deltas functions against that test.

That leads me to consider default outputing rewards/penalties for each deltas function for each deltas test instead of essentially copying over some tests and having them run separate deltas.

The nice thing is that this would de-duplicate pre-states in deltas tests and would ensure that no-ops have the intended effect on tests that don't really concern a deltas function.

The bad thing is that it muddies up the output a bit and makes it less clear what is actually being tested (but still shows granularity).

not sure. To start, I'll have the proposal ready for review shortly without combining all deltas vectors into one format

tests/core/pyspec/eth2spec/test/helpers/rewards.py

djrtwo · 2020-05-05T03:12:24Z

okay @protolambda and @hwwhww, this is finally ready for review (including tests)

EDIT: the tests are much more comprehensive, but still keep an eye out for cases that are not covered. happy to add more

hwwhww

Great work! I reviewed the content correctness, but still thinking about more edge cases. 🤔

tests/core/pyspec/eth2spec/test/helpers/rewards.py

tests/core/pyspec/eth2spec/test/phase_0/rewards/test_get_inclusion_delay_deltas.py

tests/core/pyspec/eth2spec/test/helpers/rewards.py

Co-authored-by: Hsiao-Wei Wang <hwwang156@gmail.com>

protolambda

Partial review, going to refresh for latest changes.

specs/phase0/beacon-chain.md

tests/core/pyspec/eth2spec/test/helpers/rewards.py

tests/core/pyspec/eth2spec/test/phase_0/rewards/test_get_inactivity_penalty_deltas.py

tests/formats/rewards/README.md

protolambda

You can batch apply these, the comments were inconsistent after the change to deltas

tests/core/pyspec/eth2spec/test/phase_0/rewards/test_get_inactivity_penalty_deltas.py

tests/core/pyspec/eth2spec/test/phase_0/rewards/test_get_inclusion_delay_deltas.py

tests/core/pyspec/eth2spec/test/phase_0/rewards/test_get_source_deltas.py

tests/core/pyspec/eth2spec/test/phase_0/rewards/test_get_target_deltas.py

tests/core/pyspec/eth2spec/test/helpers/rewards.py

tests/core/pyspec/eth2spec/test/phase_0/rewards/test_get_head_deltas.py

… in slashed tests

"rewards/penalties" -> "deltas" in throughout test comments/descriptions Co-authored-by: Diederik Loerakker <proto@protolambda.com>

specs/phase0/beacon-chain.md

djrtwo · 2020-05-08T16:51:11Z

@hwwhww or @protolambda, I'm looking to get this merged asap for v0.12 release. Let me know if there's anything else you want to see right now

hwwhww

The PR looks good to me 👍
We can still come up with other new test cases after this PR.

rework rewards/penalties to be more granular

fe13bab

hwwhww reviewed Apr 23, 2020

View reviewed changes

hwwhww added general:RFC Request for Comments phase0 labels Apr 23, 2020

minor feedback and fixes on rewards/penalites proposal

7612667

Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>

protolambda mentioned this pull request Apr 23, 2020

Rewards/penalties approach [illustrative, do not merge] #1748

Closed

djrtwo added 2 commits April 28, 2020 17:24

Merge branch 'dev' into rewards-rework

4a086fb

add baseline get_target_deltas tests

5f18dd7

djrtwo force-pushed the rewards-rework branch from ecc8f3a to 1c12502 Compare April 29, 2020 15:57

add tests for source, target, head

cd27e5e

djrtwo force-pushed the rewards-rework branch from 1c12502 to cd27e5e Compare April 29, 2020 16:23

JustinDrake reviewed Apr 30, 2020

View reviewed changes

specs/phase0/beacon-chain.md Outdated Show resolved Hide resolved

specs/phase0/beacon-chain.md Outdated Show resolved Hide resolved

specs/phase0/beacon-chain.md Show resolved Hide resolved

Merge branch 'dev' into rewards-rework

c2c2779

hwwhww reviewed May 4, 2020

View reviewed changes

specs/phase0/beacon-chain.md Outdated Show resolved Hide resolved

djrtwo added 3 commits May 4, 2020 13:20

basic generators work

eda2499

Merge branch 'dev' into rewards-rework

a7354bd

add test_get_inclusion_delay_deltas

5194c1f

djrtwo force-pushed the rewards-rework branch from a9129f2 to 5194c1f Compare May 4, 2020 23:01

djrtwo added 2 commits May 4, 2020 20:58

add inactivity penalty deltas tests

8f569a8

PR feedback

3f250f7

djrtwo commented May 5, 2020

View reviewed changes

tests/core/pyspec/eth2spec/test/helpers/rewards.py Outdated Show resolved Hide resolved

add comment for helper -- has_enouh_for_reward

f35106d

hwwhww reviewed May 5, 2020

View reviewed changes

djrtwo and others added 2 commits May 5, 2020 13:08

Apply suggestions from code review from @hwwhww

d26cfd2

Co-authored-by: Hsiao-Wei Wang <hwwang156@gmail.com>

PR feedback from @hwwhww

b2dfb6c

protolambda reviewed May 5, 2020

View reviewed changes

specs/phase0/beacon-chain.md Show resolved Hide resolved

tests/core/pyspec/eth2spec/test/helpers/rewards.py Outdated Show resolved Hide resolved

protolambda reviewed May 5, 2020

View reviewed changes

tests/core/pyspec/eth2spec/test/helpers/rewards.py Show resolved Hide resolved

tests/core/pyspec/eth2spec/test/phase_0/rewards/test_get_inactivity_penalty_deltas.py Outdated Show resolved Hide resolved

tests/formats/rewards/README.md Outdated Show resolved Hide resolved

address PR feedback from @protolambda

4f40113

protolambda reviewed May 5, 2020

View reviewed changes

tests/formats/rewards/README.md Outdated Show resolved Hide resolved

protolambda reviewed May 5, 2020

View reviewed changes

djrtwo and others added 2 commits May 5, 2020 16:28

add exited tests for rewards. make some valiators exited/withdrawable…

f0742b2

… in slashed tests

Apply suggestions from code review

4ffa0db

"rewards/penalties" -> "deltas" in throughout test comments/descriptions Co-authored-by: Diederik Loerakker <proto@protolambda.com>

hwwhww reviewed May 6, 2020

View reviewed changes

specs/phase0/beacon-chain.md Show resolved Hide resolved

specs/phase0/beacon-chain.md Show resolved Hide resolved

hww feedback

7a13060

djrtwo force-pushed the rewards-rework branch from 5626f1a to 7a13060 Compare May 8, 2020 16:51

hwwhww approved these changes May 8, 2020

View reviewed changes

djrtwo merged commit 6474218 into dev May 11, 2020

djrtwo deleted the rewards-rework branch May 11, 2020 14:16

terencechain mentioned this pull request May 20, 2020

Align to spec v0.12 prysmaticlabs/prysm#5935

Closed

25 tasks

paulhauner mentioned this pull request May 21, 2020

Tracking: spec v0.12.0 sigp/lighthouse#1182

Closed

22 tasks

ColinSchwarz mentioned this pull request May 25, 2020

Structure rewards/penalties to allow for more granular testing ChainSafe/lodestar#952

Closed

mratsim mentioned this pull request Aug 16, 2020

Rewards & Penalties tests missing status-im/nimbus-eth2#1513

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Proposal] rework rewards/penalties to be more granular #1747

[Proposal] rework rewards/penalties to be more granular #1747

djrtwo commented Apr 23, 2020 •

edited

hwwhww commented Apr 23, 2020

hwwhww left a comment

djrtwo commented Apr 29, 2020 •

edited

djrtwo commented May 4, 2020

djrtwo commented May 5, 2020 •

edited

hwwhww left a comment •

edited

protolambda left a comment

protolambda left a comment

djrtwo commented May 8, 2020

hwwhww left a comment

[Proposal] rework rewards/penalties to be more granular #1747

[Proposal] rework rewards/penalties to be more granular #1747

Conversation

djrtwo commented Apr 23, 2020 • edited

hwwhww commented Apr 23, 2020

hwwhww left a comment

Choose a reason for hiding this comment

djrtwo commented Apr 29, 2020 • edited

djrtwo commented May 4, 2020

djrtwo commented May 5, 2020 • edited

hwwhww left a comment • edited

Choose a reason for hiding this comment

protolambda left a comment

Choose a reason for hiding this comment

protolambda left a comment

Choose a reason for hiding this comment

djrtwo commented May 8, 2020

hwwhww left a comment

Choose a reason for hiding this comment

djrtwo commented Apr 23, 2020 •

edited

djrtwo commented Apr 29, 2020 •

edited

djrtwo commented May 5, 2020 •

edited

hwwhww left a comment •

edited