revive: Include immutable storage deposit into the contracts storage_base_deposit
#7230
+1,996
−1,796
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
athei
commented
Jan 18, 2025
…tend --pallet pallet_migrations --fail-fast --clean'
…evive --fail-fast --clean'
xermicus
approved these changes
Jan 24, 2025
pgherveou
reviewed
Jan 24, 2025
pgherveou
approved these changes
Jan 24, 2025
|
/cmd bench-omni --runtime="dev asset-hub-westend people-rococo people-westend rococo westend" --pallet pallet_migrations --fail-fast --clean |
|
/cmd bench-omni --runtime="asset-hub-westend" --pallet pallet_migrations --fail-fast |
|
/cmd bench-omni --runtime dev asset-hub-westend people-rococo people-westend rococo westend --pallet pallet_migrations --fail-fast --clean |
|
Command "bench-omni --runtime dev asset-hub-westend people-rococo people-westend rococo westend --pallet pallet_migrations --fail-fast --clean" has started 🚀 See logs here |
…hub-westend people-rococo people-westend rococo westend --pallet pallet_migrations --fail-fast --clean'
|
Command "bench-omni --runtime dev asset-hub-westend people-rococo people-westend rococo westend --pallet pallet_migrations --fail-fast --clean" has finished ✅ See logs here Subweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
|
/cmd fmt |
bkchr
reviewed
Jan 28, 2025
| if meter.remaining().any_lt(required) { | ||
| return Err(SteppedMigrationError::InsufficientWeight { required }) | ||
| } | ||
| P::on_genesis(); |
There was a problem hiding this comment.
on_genesis has some narrow meaning, I would not use it here.
We should use GetStorageVersion and implement & seal a common trait for NoVersionSet and StorageVersion that gives you the function write_to_storage or something like that. While NoVersionSet would write 0 as StorageVersion.
There was a problem hiding this comment.
I agree that we probably should not use OnGenesis here. But isn't the point of this NoVersionSet that no version should be set in storage? Meaning when NoVersionSet is the associated type we simply should not write any version?
There was a problem hiding this comment.
But isn't the point of this
NoVersionSetthat no version should be set in storage?
No. 0 is always the default. https://docs.rs/frame-support/latest/frame_support/traits/struct.NoStorageVersionSet.html the docs are not ultra clear 🙈 But basically this is just a hint for a user that tries to implement a migration and forgets to set a storage_version.
Co-authored-by: Bastian Köcher <[email protected]>
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
ggwpez
reviewed
Jan 30, 2025
| return Err(SteppedMigrationError::InsufficientWeight { required }) | ||
| } | ||
| P::on_genesis(); | ||
| meter.consume(required); |
There was a problem hiding this comment.
Nit: There is a try_consume to do the check and consumption in the if.
ggwpez
reviewed
Jan 30, 2025
| } | ||
|
|
||
| let base_weight = T::WeightInfo::reset_pallet_migration(0); | ||
| let weight_per_key = T::WeightInfo::reset_pallet_migration(1) - base_weight; |
There was a problem hiding this comment.
Suggested change
| let weight_per_key = T::WeightInfo::reset_pallet_migration(1) - base_weight; | |
| let weight_per_key = T::WeightInfo::reset_pallet_migration(1).saturating_sub(base_weight); |
Am always a bit paranoid with this in the runtime 😅
|
|
||
| if key_budget == 0 { | ||
| return Err(SteppedMigrationError::InsufficientWeight { | ||
| required: T::WeightInfo::reset_pallet_migration(1), |
There was a problem hiding this comment.
Suggested change
| required: T::WeightInfo::reset_pallet_migration(1), | |
| required: T::WeightInfo::reset_pallet_migration(0).saturating_add(T::WeightInfo::reset_pallet_migration(1)), |
Otherwise it cannot make progress?
|
|
||
| if keys_before <= keys_now { | ||
| log::error!("ResetPallet<{}>: Did not remove any keys.", P::name()); | ||
| Err("ResetPallet failed")?; |
There was a problem hiding this comment.
This and the as _ is some big brain code 😲
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.
This PR is centered around a main fix regarding the base deposit and a bunch of drive by or related fixtures that make sense to resolve in one go. It could be broken down more but I am constantly rebasing this PR and would appreciate getting those fixes in as-one.
This adds a multi block migration to Westend AssetHub that wipes the pallet state clean. This is necessary because of the changes to the
ContractInfostorage item. It will not delete the child storage though. This will leave a tiny bit of garbage behind but won't cause any problems. They will just be orphaned.Record the deposit for immutable data into the
storage_base_depositThe
storage_base_depositare all the deposit a contract has to pay for existing. It included the deposit for its own metadata and a deposit proportional (< 1.0x) to the size of its code. However, the immutable code size was not recorded there. This would lead to the situation where on terminate this portion wouldn't be refunded staying locked into the contract. It would also make the calculation of the deposit changes onset_code_hashmore complicated when it updates the immutable data (to be done in #6985). Reason is because it didn't know how much was payed before since the storage prices could have changed in the mean time.In order for this solution to work I needed to delay the deposit calculation for a new contract for after the contract is done executing is constructor as only then we know the immutable data size. Before, we just charged this eagerly in
charge_instantiatebefore we execute the constructor. Now, we merely send the ED as free balance before the constructor in order to create the account. After the constructor is done we calculate the contract base deposit and charge it. This will makeset_code_hashmuch easier to implement.As a side effect it is now legal to call
set_immutable_datamultiple times per constructor (even though I see no reason to do so). It simply overrides the immutable data with the new value. The deposit accounting will be done after the constructor returns (as mentioned above) instead of when setting the immutable data.Don't pre-charge for reading immutable data
I noticed that we were pre-charging weight for the max allowable immutable data when reading those values and then refunding after read. This is not necessary as we know its length without reading the storage as we store it out of band in contract metadata. This makes reading it free. Less pre-charging less problems.
Remove delegate locking
Fixes #7092
This is also in the spirit of making #6985 easier to implement. The locking complicates
set_code_hashas we might need to block settings the code hash when locks exist. Check #7092 for further rationale.Enforce "no terminate in constructor" eagerly
We used to enforce this rule after the contract execution returned. Now we error out early in the host call. This makes it easier to be sure to argue that a contract info still exists (wasn't terminated) when a constructor successfully returns. All around this his just much simpler than dealing this check.
Moved refcount functions to
CodeInfoThey never really made sense to exist on
Stack. But now with the locking gone this makes even less sense. The refcount is stored insideCodeInfoto lets just move them there.Set
CodeHashLockupDepositPercentfor test runtimeThe test runtime was setting
CodeHashLockupDepositPercentto zero. This was trivializing many code paths and excluded them from testing. I set it to30%which is our default value and fixed up all the tests that broke. This should give us confidence that the lockup doeposit collections properly works.Reworked the
MockExecutableto have both adeployand acallentry pointThis type used for testing could only have either entry points but not both. In order to fix the
immutable_data_set_overridesI needed to a new functionadd_bothtoMockExecutablethat allows to have both entry points. Make sure to make use of it in the future :)