Implementation of RFC-123

[ordian]

Closes #64

This PR implements approach 4 outlined in the issue and polkadot-fellows/RFCs#123.

Unresolved questions:

• Does the CallContext::{Offchain, Onchain} determine whether it's runtime calls / (block production or import)?
• 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
• Do we want to pass IgnorePendingCode regardless in the higher level APIs? No
• Do we want to emit an event when pending code is set?
• Should the system_version check also be done on the node side (block production and import)?
• What is the best place to call maybe_apply_pending_code_upgrade?

TODO:

• Fix failing tests
• New unit tests
• e2e tests also for PVF upgrade
• make sure this After runtime upgrade, Runtime API calls use new code with unmigrated storage #64 (comment) is not a problem

[bkchr]

• dryRun RPC call will not work properly on the block setting the pending code, right? Is that a problem?

Why?

make sure this #64 (comment) is not a problem

This also happens in the block before. Not sure how this should be a problem in the runtime upgrade block. Yes, it decreases the available PoV size, but then we need to use multi block migrations.

[ordian]

dryRun RPC call will not work properly on the block setting the pending code, right? Is that a problem?

Why?

If we try to check a transaction at the block that sets the new code as pending code, it will use the old code for checking it (because RPC uses a runtime call with offchain context), but the next block will use the new code. So even though transaction might be valid with old code, it might be invalid with the new one (or vice versa). Am I missing something here?

[bkchr]

Ahh you meant validate_transaction. Transactions are getting invalidated any way by runtime upgrades because the spec version changes. Runtime upgrades are also not happen that regularly, so I think it is fine.

[ordian]

This also happens in the block before. Not sure how this should be a problem in the runtime upgrade block. Yes, it decreases the available PoV size, but then we need to use multi block migrations.

For this item, what I want to test is a runtime upgrade with code close to max size to make sure it fits into PoV. My worry is that it will be included twice (as part of block body and state proof in :pending_code or twice in state proof). Given that max code size is set as 3mb, two of them might not fit into PoV limits (unless we bump them), although compression might save us here.

[ordian]

Another question I have is that it seems to me that we only start compiling the new code with wasmtime when we call

polkadot-sdk/substrate/client/executor/src/wasm_runtime.rs

Line 218 in f7119e4

pub fn with_instance<'c, H, R, F>(

In that case, it means that currently, we will compile the new runtime right after importing the block N:

┌────────────────────────────────────────────────────────────────┐
│┌────────────────────┐┌──────────────────┐┌────────────────────┐│
││                    ││                  ││┌────┐              ││
││      block #N      ││   compile new    │││migr│ block #N+1   ││
││                    ││     runtime      │││atio│              ││
││                    ││                  ││└────┘              ││
│└────────────────────┘└──────────────────┘└────────────────────┘│
└────────────────────────────────────────────────────────────────┘

because we will call some runtime API after importing it. However, with this PR, we will only start compiling the new runtime when we start importing block N+1 (unless we are a block producer) as the runtime API calls for block N use old code:

┌─────────────────────────────────────────────────────────────────────────┐
│┌────────────────────┐┌──────────────────┐┌─────────────────────────────┐│
││                    ││                  ││┌──────┐┌────┐               ││
││      block #N      ││      :(          │││compi ││migr│  block #N+1   ││
││                    ││                  │││lation││atio│               ││
││                    ││                  ││└──────┘└────┘               ││
│└────────────────────┘└──────────────────┘└─────────────────────────────┘│
└─────────────────────────────────────────────────────────────────────────┘

Is that correct, or I am missing somethings?

[bkchr]

My worry is that it will be included twice (as part of block body and state proof in :pending_code or twice in state proof).

How? Parachains are not upgrading instantly. They first need to announce it to the relay chain and even if they would do it instantly, :pending_code is moved to :code in X + 1 while X contained the transaction that upgraded the node (but as I said before, this is not possible for parachains any way).

[bkchr]

because we will call some runtime API after importing it. However, with this PR, we will only start compiling the new runtime when we start importing block N+1 (unless we are a block producer) as the runtime API calls for block N use old code:

While that is being correct, this doesn't mean it would always need to be the case that this is happening. However, we could add this optimization to ensure that the code is compiled in between importing and before building a new block. But this is not really a requirement for this PR IMO.

[paritytech-workflow-stopper]

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12650411555
Failed job name: fmt

[ordian]

@bkchr any feedback on the questions and the implementation?

[skunert]

Logic looks good. I went ahead and paranoid verified some scenarios manually (spoiler: everything worked :D).

• Upgrade relay chain with system_version < 3
• Upgrade relay chain with system_version >= 3
• Upgrade parachain

For these I verified each time that the correct code is being used.

Do we want to pass IgnorePendingCode regardless in the higher level APIs?

Currently I don't see a use case for it. If needed, we could add it later IMO. In 99% of cased, we want to use the normal :code. Only to advance the state :pending_code is relevant, so seems niche.

Do we want to emit an event when pending code is set?

Currently we should emit Event::ValidationFunctionStored already when this happens no?

Should the system_version check also be done on the node side (block production and import)?

Not sure about this one, do you think it would be useful? It looks like :pending_code will not play a role with system_version < 3, so node will ignore it automatically.

BTW, I saw that we don't check the system_version for parachains, did we have a discussion about that?

[skunert]

Nitpick: Why do we do this map_err(drop) here? Seems overengineered 🤔

[ordian]

Not sure, but seems like to satisfy types. Either way, not introduced here :)