RFC-0123: Introduce :pending_code as intermediate storage key for the runtime code#123
Conversation
| ### Compatibility | ||
|
|
||
| The change will require that the nodes are upgraded before the runtime starts using this feature. Otherwise they will fail to import the block build by `:pending_code`. | ||
| For Polkadot/Kusama this means that also the parachain nodes need to be running with a relay chain node version that supports this new feature. Otherwise the parachains will stop producing/finalizing nodes as they can not sync the relay chain any more. |
There was a problem hiding this comment.
Should the RFC also describe how the feature will be enabled ? Is it by simply bumping system_version to 3 in the runtime once we know all the nodes have upgraded ?
There was a problem hiding this comment.
Is it by simply bumping system_version to 3 in the runtime once we know all the nodes have upgraded ?
Yes exactly. This was my idea.
There was a problem hiding this comment.
Waiting for relay chain validators to upgrade seems like an acceptable way to introduce a breaking change, but parachain nodes are known to lag behind and stay on an older version. Would it be possible to roll out this change on the relay chain side in a backwards compatible manner for parachain nodes/collators? They might even prefer to disable runtime APIs on runtime upgrades to this change.
There was a problem hiding this comment.
I don't see any way in doing this in a backwards compatible way. Whatever we do here to fix it, we will always need to touch the node AFAIK.
There was a problem hiding this comment.
I don't think it is practical or desired to require parachain teams to upgrade their collators. This will delay the deployment of this fix for a long time.
I think the most practical solution is #3 from paritytech/polkadot-sdk#64 . My argument here is that currently we are doing it wrong, we are bulding block X (which contains set_code(B)) with runtime A, but then we call runtime APIs on it using runtime B. We should always call runtime APIs at a block using the same runtime version we used to build it.
There was a problem hiding this comment.
paritytech/polkadot-sdk#64 (comment) also see the comments here around pruning.
There was a problem hiding this comment.
paritytech/polkadot-sdk#64 (comment) also see the comments here around pruning.
Pruning should not be a problem IMO, on validators/full nodes we don't actually need to call runtime APIs on blocks that old.
This would require that all the external tooling is doing the same.
Can you explain a bit how is this affected? (I don't see any details in that comment)
Doesn't the tooling rely on a node to get the data, so if the node is upgraded then it would work fine ?
There was a problem hiding this comment.
Pruning should not be a problem IMO, on validators/full nodes we don't actually need to call runtime APIs on blocks that old
So you know all the access patterns of every single app interfacing with the node? I don't think so
There was a problem hiding this comment.
I don't but it's very easy to fix - not prune the parent of the blocks containing the code upgrade.
There was a problem hiding this comment.
I still want to implement the proper solution. For now we can just implement a workaround in the node that calls initialize_block and trades extra costs for running a migration multiple times for having the correct result.
|
|
||
| The code of a runtime is stored in its own state, and when performing a runtime upgrade, this code is replaced. The new runtime can contain runtime migrations that adapt the state to the state layout as defined by the runtime code. This runtime migration is executed when building the first block with the new runtime code. Anything that interacts with the runtime state uses the state layout as defined by the runtime code. So, when trying to load something from the state in the block that applied the runtime upgrade, it will use the new state layout but will decode the data from the non-migrated state. In the worst case, the data is incorrectly decoded, which may lead to crashes or halting of the chain. | ||
|
|
||
| This RFC proposes to store the new runtime code under a different storage key when applying a runtime upgrade. This way, all the off-chain logic can still load the old runtime code under the default storage key and decode the state correctly. The block producer is then required to use this new runtime code to build the next block. While building the next block, the runtime is executing the migrations and moves the new runtime code to the default runtime code location. So, the runtime code found under the default location is always the correct one to decode the state from which the runtime code was loaded. |
There was a problem hiding this comment.
What if we need multi block migrations?
There was a problem hiding this comment.
|
|
||
| ### Performance | ||
|
|
||
| The performance should not be impacted besides requiring loading the runtime code in the first block being built with the new runtime code. |
There was a problem hiding this comment.
So we end up having the code in one more PoV with this scheme. I am not sure this is entirely clear from the wording.
|
|
||
| ## Future Directions and Related Material | ||
|
|
||
| - Solve the issue of requiring loading the entire runtime code to move it into a different location by introducing a low-level `move` function. When using the `V1` trie layout every value bigger than 32 bytes is put into the db separately. This means a low level `move` function would only need to move the hash of the runtime code from `:code` to `:pending_code`. |
There was a problem hiding this comment.
Without this, don't we have an even more severe risk of needing a multi block migration?
There was a problem hiding this comment.
If you want to migrate 5MIB at once, yes. However, even now I would not recommend you to do this in one migration. Also it is not a "severe risk", you just need to use a multi block migration ;) These already exist.
|
I finally want to get this merged. While the solution requires some time to be applied, we can make an intermediate fix that takes the code from the parent block when there was the |
anaelleltd
added
Reviewed
|
/rfc propose |
|
Hey @bkchr, here is a link you can use to create the referendum aiming to approve this RFC number 0123. Instructions
It is based on commit hash b58d5aa293f478e6604c5160d21871dc6f78a2fd. The proposed remark text is: |
|
Voting for this referenda is ongoing. Vote for it here |
|
PR can be merged. Write the following command to trigger the bot
|
|
/rfc process 0xf5971ef9eccd5d161dd9848765a5abc8cf0cf3df8986f583ee200dc0bbebf10a |
|
The on-chain referendum has approved the RFC. |
anaelleltd
added
Approved
Closes #64 This PR implements approach 4 outlined in the issue and polkadot-fellows/RFCs#123. Unresolved questions: - [x] Does the `CallContext::{Offchain, Onchain}` determine whether it's runtime calls / (block production or import)? - [x] dryRun RPC call will not work properly on the block setting the pending code, right? Is that a problem? A: No, transactions are getting invalidated any way by runtime upgrades because the spec version changes - [X] Do we want to pass `IgnorePendingCode` regardless in the higher level APIs? No - [x] Do we want to emit an event when pending code is set? - [x] Should the system_version check also be done on the node side (block production and import)? - [x] What is the best place to call `maybe_apply_pending_code_upgrade`? TODO: - [x] Fix failing tests - [x] New unit tests - [ ] e2e tests also for PVF upgrade - [x] make sure this #64 (comment) is not a problem --------- Co-authored-by: Dmitry Sinyavin <dmitry.sinyavin@parity.io> Co-authored-by: s0me0ne-unkn0wn <48632512+s0me0ne-unkn0wn@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
7 participants
No description provided.