-
Notifications
You must be signed in to change notification settings - Fork 19
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(upgrade): v1.0.0 to polkadot-sdk v1.1.0 #1730
chore(upgrade): v1.0.0 to polkadot-sdk v1.1.0 #1730
Conversation
dbb4e03
to
5945ad6
Compare
Codecov Report
@@ Coverage Diff @@
## main #1730 +/- ##
==========================================
+ Coverage 87.95% 88.07% +0.11%
==========================================
Files 50 50
Lines 4243 4243
==========================================
+ Hits 3732 3737 +5
+ Misses 511 506 -5
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
106ce11
to
3d358db
Compare
10792d4
to
4afd537
Compare
5d22eeb
to
1a60415
Compare
b63d691
to
3fbc3d5
Compare
⏳ Running benchmarks and calculating weights. DO NOT MERGE! A new commit will be added upon completion... |
⏳ Running benchmarks and calculating weights. DO NOT MERGE! A new commit will be added upon completion... |
eb316eb
to
668b348
Compare
⏳ Running benchmarks and calculating weights. DO NOT MERGE! A new commit will be added upon completion... |
✅ Finished running benchmarks. Updated weights have been committed to this PR branch in commit 7164ae0. |
⏳ Running benchmarks and calculating weights. DO NOT MERGE! A new commit will be added upon completion... |
✅ Finished running benchmarks. Updated weights have been committed to this PR branch in commit 5e2fd02. |
The goal of this PR is to upgrade from v1.0.0 to v1.1.0. Update benchmarks for custom pallets and overhead. References - [Polkadot Forum](https://forum.polkadot.network/t/polkadot-release-analysis-v1-1-0/3988) - [Release Notes](https://github.com/paritytech/polkadot-sdk/releases/tag/polkadot-v1.1.0) Co-authored-by: Enddy Dumbrique <enddy.dumbrique@unfinished.com> Co-authored-by: Matthew Orris <1466844+mattheworris@users.noreply.github.com>
5e2fd02
to
8c027b7
Compare
@@ -96,17 +96,18 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> { | |||
/// Storage: `Msa::DelegatorAndProviderToDelegation` (r:1 w:1) | |||
/// Proof: `Msa::DelegatorAndProviderToDelegation` (`max_values`: None, `max_size`: Some(217), added: 2692, mode: `MaxEncodedLen`) | |||
/// Storage: `Schemas::CurrentSchemaIdentifierMaximum` (r:1 w:0) | |||
/// Proof: `Schemas::CurrentSchemaIdentifierMaximum` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | |||
/// Proof: `Schemas::CurrentSchemaIdentifierMaximum` (`max_values`: Some(1), `max_size`: Some(2), added: 497, mode: `MaxEncodedLen`) |
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.
how did we get MaxEncodedLen here?
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.
My guess is that Parity might've fixed some issues in benchmarking. So previously sometimes when we were accessing other pallets it would count them as measured
even though the top level benchmark had MaxEncodedLen
and it looks like that issue might've fixed
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.
Looks good to me.
Might be worth cleaning up the cargo deny warnings, but not a blocker.
- Ran various local tests
- Ran several versions
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.
Overall looks good a couple of questions and concerns
- Since try-runtime is separated into a different binary I think our try-runtime script would fail but it was failing before so not a blocker
- concern: Are we sure if there is no internal storage migrations for the frame-pallets that we use?
@@ -96,17 +96,18 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> { | |||
/// Storage: `Msa::DelegatorAndProviderToDelegation` (r:1 w:1) | |||
/// Proof: `Msa::DelegatorAndProviderToDelegation` (`max_values`: None, `max_size`: Some(217), added: 2692, mode: `MaxEncodedLen`) | |||
/// Storage: `Schemas::CurrentSchemaIdentifierMaximum` (r:1 w:0) | |||
/// Proof: `Schemas::CurrentSchemaIdentifierMaximum` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | |||
/// Proof: `Schemas::CurrentSchemaIdentifierMaximum` (`max_values`: Some(1), `max_size`: Some(2), added: 497, mode: `MaxEncodedLen`) |
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.
My guess is that Parity might've fixed some issues in benchmarking. So previously sometimes when we were accessing other pallets it would count them as measured
even though the top level benchmark had MaxEncodedLen
and it looks like that issue might've fixed
I did not see any migrations but perhaps I missed one @mattheworris? |
There are no migrations called out in the release notes for v1.1.0. |
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.
Great Job! Good to go!
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.
I have reviewed the changes and looks good, however wont be able to approve since I havent run full tests
Otherwise good to go
Please take the time to run the full test that you need to give it your approval. |
/// Min, Max: 100_113, 110_777 | ||
/// Average: 101_295 | ||
/// Median: 101_040 | ||
/// Std-Dev: 1335.33 |
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.
Just noting that this is a jump of ~30% in std-dev which means the weight calculations are varying more widely in this benchmark run.
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.
Good work, ship it!
The goal of this PR is to upgrade from v1.0.0 to v1.1.0.
Benchmarks updated for custom pallets and overhead.
References
Closes Issue #1687