-
Notifications
You must be signed in to change notification settings - Fork 629
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
chore: Collapse stateless validation versions and params #11637
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #11637 +/- ##
==========================================
- Coverage 71.59% 71.57% -0.02%
==========================================
Files 787 787
Lines 160736 160728 -8
Branches 160736 160728 -8
==========================================
- Hits 115072 115044 -28
- Misses 40631 40644 +13
- Partials 5033 5040 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ProtocolFeature::NoChunkOnlyProducers => 88, | ||
ProtocolFeature::ChangePartialWitnessDataPartsRequired => 89, | ||
ProtocolFeature::BiggerCombinedTransactionLimit => 90, | ||
ProtocolFeature::SimpleNightshadeTestonly => 100, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not work for the test in resharding... Let me check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we are anyway ignoring all resharding tests. Might be fine to just ignore test_latest_protocol_missing_chunks_low_missing_prob etc. too in case it fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a Nayduck test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They did not fail, so I guess the new version is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking this is deemed to be buried until we start working on resharsing again. Most of the resharding tests are disabled because of lack of integration with cc and sv anyway. Feel free to add ignore
to any other tests that may fail because of this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
I'm afk so couldn't review the numbers in yamls but otherwise looks good! Thanks for taking care of this.
| ProtocolFeature::OutgoingReceiptsSizeLimit | ||
| ProtocolFeature::NoChunkOnlyProducers | ||
| ProtocolFeature::ChangePartialWitnessDataPartsRequired | ||
| ProtocolFeature::BiggerCombinedTransactionLimit => 81, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to do it now but we can take it further and replace all of those with just a single protocol feature StatelessValidation
. This may be a bit annoying so let's do it separately.
Congestion control => 80 (as before, no change) All Stateless validation features => 81 (collapse versions 81..90 into 81) SimpleNightshadeTestonly => 100 (to not interfere with stateless transition)
Congestion control => 80 (as before, no change)
All Stateless validation features => 81 (collapse versions 81..90 into 81)
SimpleNightshadeTestonly => 100 (to not interfere with stateless transition)