-
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): polkadot v1.10 to v1.2.0 #1982
chore(upgrade): polkadot v1.10 to v1.2.0 #1982
Conversation
0353c31
to
3fa64ba
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
c998734
to
f4d0615
Compare
f4d0615
to
1cbb9de
Compare
1cbb9de
to
669d90b
Compare
4208fe8
to
5c119a3
Compare
c686fce
to
7de683c
Compare
7de683c
to
9663fa3
Compare
@@ -646,7 +649,7 @@ impl pallet_balances::Config for Runtime { | |||
type WeightInfo = weights::pallet_balances::SubstrateWeight<Runtime>; | |||
type MaxReserves = BalancesMaxReserves; | |||
type ReserveIdentifier = [u8; 8]; | |||
type MaxHolds = ConstU32<0>; | |||
type MaxHolds = ConstU32<1>; |
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.
Curious why this change. I thought we didn't want to allow holds as they are going away
Although perhaps it is needed to not break the preimage pallet while upgrading?
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 question. I also had to review this part. The introduction of MaxHolds is part of the process of deprecating the Currency trait. To clarify, a hold is similar to the deprecated reserve idea, reducing the free balance in your account. Previously, holds were set to zero because no pallet was utilizing them. However, with the Preimage pallet now using holds, as a result, we increased the MaxHold by one. You can see how holds are utilized in the new associated type called Consideration:
impl pallet_preimage::Config for Runtime {
...
type Consideration = HoldConsideration<
AccountId,
Balances,
PreimageHoldReason,
LinearStoragePrice<PreimageBaseDeposit, PreimageByteDeposit, Balance>,
>;
}
This type incorporates the balances pallet, which implements the MutateHold trait. Within the Preimage pallet, the method T::Consideration::new(...) is called, invoking the balances pallet to create a hold.
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.
One question about holds, but otherwise looks like a nice simple upgrade.
- Read through the changes
0a96653
to
074b894
Compare
Upgrade Polkadot v1.1.0 to v1.2.0. [Polkadot release notes](https://forum.polkadot.network/t/polkadot-release-analysis-v1-2-0/4451) Lazy Migration paritytech/polkadot-sdk#1363 Update weights. issue-1718
074b894
to
7b2f70c
Compare
// Minimum execution time: 13_175_000 picoseconds. | ||
Weight::from_parts(11_629_620, 2809) | ||
// Standard Error: 9_686 | ||
.saturating_add(Weight::from_parts(3_219_955, 0).saturating_mul(b.into())) | ||
// Minimum execution time: 13_026_000 picoseconds. | ||
Weight::from_parts(11_528_896, 2809) | ||
// Standard Error: 11_636 | ||
.saturating_add(Weight::from_parts(3_232_614, 0).saturating_mul(b.into())) |
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 do you know what numbers to change all of these values to? They all seem somewhat arbitrary.
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.
These numbers are automatically generated when we run benchmarks against the extrinsics we create. For example, this benchmark tests the performance of the messages pallet. When the benchmarks are executed, they run the specified code and provide measurements of execution time and resource usage. These metrics help us understand the computational cost of transactions and determine appropriate fees.
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 reviewed Cargo.lock to check that the update SHA matches the Polkadot release SHA
- I checked that new weights were not significantly change from before - around 2 microseconds except where noted.
- I reviewed other code changes - mostly the switch to
transferAllowDeath
and the note about MaxHolds.
.saturating_add(Weight::from_parts(2_305, 0).saturating_mul(s.into())) | ||
.saturating_add(T::DbWeight::get().reads(1_u64)) | ||
.saturating_add(T::DbWeight::get().writes(2_u64)) | ||
// Minimum execution time: 45_175_000 picoseconds. |
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.
Noting that preimage pallet weights are a lot higher (10+ microseconds some of them) due to extra reads/writes
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.
- Read through the changes
- Read through the release notes and release analysis
- Executed e2e tests (All passed)
Suggestion: Consider adding https://github.com/ggwpez/substrate-scripts/blob/master/migrate-preimage-lazy.py to our scripts
, I have found this helpful for running the scripts in the past.
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.
Ship it!
- Reviewed changes
- Ran locally
- Ran e2e tests locally
Goal
Update polkadot from v1.1.0 to v1.2.0.
Polkadot release notes
Lazy Migration
paritytech/polkadot-sdk#1363
Closes #1718