fix: chunk validator rewards #11498
Merged
fix: chunk validator rewards #11498
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Longarithm
changed the title
Oh, okay. We really should do #11265 to make epoch configs less confusing. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11498 +/- ##
==========================================
- Coverage 71.35% 71.32% -0.04%
==========================================
Files 787 787
Lines 159385 159417 +32
Branches 159385 159417 +32
==========================================
- Hits 113734 113702 -32
- Misses 40734 40768 +34
- Partials 4917 4947 +30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
tayfunelmas
reviewed
Jun 6, 2024
core/chain-configs/src/lib.rs
Outdated
| @@ -52,6 +52,9 @@ pub const BLOCK_PRODUCER_KICKOUT_THRESHOLD: u8 = 90; | |||
| /// Criterion for kicking out chunk producers. | |||
| pub const CHUNK_PRODUCER_KICKOUT_THRESHOLD: u8 = 90; | |||
There was a problem hiding this comment.
should we also make them 80 given that the default is 80 in mainnet?
There was a problem hiding this comment.
I don't want to break existing logic. Moreover, there may be tests checking that at genesis threshold was at 90 and then it was lowered to 80...
tayfunelmas
reviewed
Jun 6, 2024
| entry.endorsement_stats_mut().expected += | ||
| stat.endorsement_stats().expected; | ||
| entry.endorsement_stats_mut().produced += | ||
| stat.endorsement_stats().produced; |
There was a problem hiding this comment.
oh one more place to fix. thanks for catching this!
birchmd
approved these changes
Jun 6, 2024
| @@ -291,6 +288,10 @@ impl EpochInfoAggregator { | |||
| .and_modify(|entry| { | |||
| *entry.expected_mut() += stat.expected(); | |||
| *entry.produced_mut() += stat.produced(); | |||
| entry.endorsement_stats_mut().expected += | |||
Longarithm
changed the title
tayfunelmas
approved these changes
Jun 6, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 7, 2024
Separating chunk validator kickouts out of #11498 so we can decide on that separately. [More Zulip discussion](https://near.zulipchat.com/#narrow/stream/295558-core/topic/chunk.20validator.20rewards.26kickouts/near/443126357) Adds `chunk_validator_only_kickout_threshold = 80` which kicks out chunk validator-only nodes for which < 80% chunks were not included. Testing: * `test_validator_kickout_sanity` is extended to check that chunk producer is not kicked out for low endorsement stats, but chunk validator is. * `test_chunk_validator_kickout` checks that if too many chunks are missing but chunk producer threshold is low, only chunk validator is kicked out.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
UPDATE: this only fixes
EpochInfoAggregator::merge_common. Chunk validator threshold is a TODO.Old motivation
Here we just introduce
chunk_validator_only_kickout_threshold= 90 for mainnet. If node only validates chunks and number of endorsed chunks is less than threshold, it should be kicked out. It doesn't seem reasonable to apply this limit to block&chunk producers, see small discussion.Actually, testing this revealed a serious oversight in stats computation:
EpochInfoAggregator::merge_commondidn't sum up endorsement stats which led to them always being low, which could lead to unpredictable kickouts. This is fixed as well.Testing:
test_validator_kickout_sanityis extended to check that chunk producer is not kicked out for low endorsement stats, but chunk validator is.test_chunk_validator_kickoutchecks that if too many chunks are missing but chunk producer threshold is low, only chunk validator is kicked out.This required adding new variables to
epoch_config(...)configuration and removing old ones, which leads to big diff, but reviewing only tests should be enough.I also want to add a TestLoop test but this change is already big enough.