pallet-contracts v9 storage migration can result in huge PoV

[Dinonard]

Is there an existing issue?

• I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• This is not a support question.

Description of bug

Hello,

as part of adding indeterministic instructions support for off-chain #12469, a storage migration function was added (link).

Migration is functionally very simple but doesn't take into account the possible large PoV size.
In case we have many contracts, it will explode.

For the case of Shibuya testnet, where there are 609 contract codes deployed, the PoV of the upgrade ends up being:
PoV size { header: 0.220703125kb, extrinsics: 3.5537109375kb, storage_proof: 23371.24609375kb }

The migration should be broken into multiple steps to limit the PoV size.

---

It would also be a good practice (maybe another issue ticket?) to more correctly estimate the PoV size in try-runtime, if possible.
Right now, I've received:
TryRuntime_on_runtime_upgrade executed without errors. Consumed weight = (77100000000 ps, 0 byte), total weight = (500000000000 ps, 5242880 byte) (15.42 %, 0.00 %). Tbh, I totally missed that it was 0 before.
Maybe something like this:

weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1));
weight.saturating_accrue(Weight::from_proof_size(OldPrefabWasmModule::max_encoded_len()));
weight.saturating_accrue(Weight::from_proof_size(PrefabWasmModule::max_encoded_len()));

Steps to reproduce

Attempting try-runtime off Shibuya or perhaps even Contracts parachain (haven't tested this).

[bkchr]

CC @athei

[athei]

It would also be a good practice (maybe another issue ticket?) to more correctly estimate the PoV size in try-runtime, if possible.

Probably try-runtime is not yet adapted to WeightsV2 (drops the proof size component). But even if it didn't most migrations probably don't account for proof size either (as you pointed out for the one in question).

Migration is functionally very simple but doesn't take into account the possible large PoV size.
In case we have many contracts, it will explode.

Thank you for discovering this. Splitting this migration retroactively is quite hard since the changes are already out there. It would be some ad-hoc solution not merged to master. However, I think in this case we are lucky. We just added a field to the end of a struct. We also don't actually need to read the struct (which is huge) to write this field.

I think the migration can be changed to iterate over the keys and use append in order to just add the value for determinism to the end of the struct. Then we don't read the code and incur this huge PoV. Do you think you can take this on?

@bkchr Is my assumption current that by just appending the value is not added the the storage proof?

[bkchr]

@bkchr Is my assumption current that by just appending the value is not added the the storage proof?

No. Currently appending also only supported for container like structures that have their length prepended and then followed by all the items. But even this requires reading you need the know the old length to increment it etc.

[Dinonard]

I discovered it the hard way after upgrading our Shibuya testnet. Functionally everything went fine but PoV size ended up being around 23 MB.

Do you think you can take this on?

I worked on a solution today since we want to get Shibuya contracts deployed prior to latest upgrade working ASAP.
It's essentially a multi-stage (block) migration.

Here's the PR: AstarNetwork/Astar#803 (still under review & might need some polish)

We opted for a manual transition (dedicated ext.call) since that gives us more control than having it e.g. in the on_initialize.
But I could adapt this solution to match your requirements.

[athei]

@bkchr Is my assumption current that by just appending the value is not added the the storage proof?

No. Currently appending also only supported for container like structures that have their length prepended and then followed by all the items. But even this requires reading you need the know the old length to increment it etc.

I see. But something like a struct where the size is static would work without reading, right? It is just not supported right now? We could have a append_raw which just appends something and does nothing else?

Here's the PR: AstarNetwork/Astar#803 (still under review & might need some polish)

We opted for a manual transition (dedicated ext.call) since that gives us more control than having it e.g. in the on_initialize.
But I could adapt this solution to match your requirements.

We will probably run into this issue more often. Would be nice to have this multiblock migration part of pallet-contracts. Given how pallet-contracts is constructed and its need for evolution we will probably run into this issue more often. Would be nice to have a multi-block migration mechanism built in. So that it can be used for all migrations there. Especially since there can even be more than one migration be happening if an upgrade was skipped. If you take this on this would be much appreciated.

[xlc]

We could have a append_raw which just appends something and does nothing else?

How are you creating PoV and state root without the original data? I don't think that's possible.

The better solution is lazy migration and the industry have tons of material & experiences on how to perform live migration with minimal impact to production services with huge data. We should learn from that.

[athei]

How are you creating PoV and state root without the original data? I don't think that's possible.

Yes you are right. The validator needs to produce the state root, too.

The better solution is lazy migration and the industry have tons of material & experiences on how to perform live migration with minimal impact to production services with huge data. We should learn from that.

It is not either or. Of course we should have a good mechanism for lazy migration. But if we can get away with something even easier why not. Not saying that it is. Just trying ideas here.

[bkchr]

We will probably run into this issue more often. Would be nice to have this multiblock migration part of pallet-contracts. Given how pallet-contracts is constructed and its need for evolution we will probably run into this issue more often. Would be nice to have a multi-block migration mechanism built in. So that it can be used for all migrations there. Especially since there can even be more than one migration be happening if an upgrade was skipped. If you take this on this would be much appreciated.

Yes we really need some multi block primitives, not only in contracts :D

[Dinonard]

Agree it would be nice to have a generic lazy migration support.
That does seem like a much bigger task than what I did for this scenario though 🙂.

• How long do we keep the lazy migration running?
  • Do we intervene manually at some point?
• What if previous migration hasn't been fully completed and we need yet another migration?
  • How deep should we go?
• Keeping track of migration status on-chain?
• Metadata for storage which is mid-migration?
  🤯

If you take this on this would be much appreciated.

Sure, I mentioned before I don't mind adapting the solution I did for Shibuya (and Shiden) to work for pallet-contracts. 🙂
I'm just concerned about whether this approach is something you guys want.

Yes we really need some multi block primitives, not only in contracts :D

🙌

[bkchr]

Personally I don't think that we should "support" lazy migrations. I mean in the end every body could implement this on their own. However, as you already said, you never know when you can remove them as the migration is done.

[xlc]

However, as you already said, you never know when you can remove them as the migration is done.

We can perform a one-off migration after the majority of the data is migrated and then completely retire the old code
#7911 (comment)

But yeah is it more of a pattern, than a framework feature request

[Dinonard]

Personally I don't think that we should "support" lazy migrations. I mean in the end every body could implement this on their own.

I use the phrase "lazy migration support" very liberally. So far I've encountered this issue twice and used the same template to solve it. I was hoping though that there's a better way to do it, unclear to me due to lack of knowledge 🙂.

The issue thread Bryan posted is great (I was looking for this but didn't find it before). Good to know there's an more generic open issue for this already.

Please feel free to assign this issue ticket to me if you want me to migrate AstarNetwork/Astar#803 to pallet-contracts.