Skip to content
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

Reconsider the way runtime version checks are performed on node side #3756

Open
tdimitrov opened this issue Mar 20, 2024 · 4 comments
Open
Labels
I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. T0-node This PR/Issue is related to the topic “node”. T8-polkadot This PR/Issue is related to/affects the Polkadot network.

Comments

@tdimitrov
Copy link
Contributor

Follow up from a discussion with @alindima in #3580 (comment)

At the moment new runtime apis are handled on the node side by calling has_required_runtime which queries the runtime version. Usually this call is followed by another runtime call (being either a 'new runtime call' or a 'fallback runtime call').

This leads to duplicated version checks - once in has_required_runtime and once in the runtime call itself (check the discussion in the linked PR for details and example).

There are two possible solutions:

  1. Optimise for 'legacy call' case or 'new call' case. Perform one of the calls, if it fails due to not supported error - execute the fallback. This will save one call to runtime api subsystem on the node in the happy case. Won't save runtime calls because version check results should be cached.
  2. Add fallback support to runtime-api subsystem. It will execute either 'legacy call' or 'new call' based on the runtime version and return one of the two results via an enum. This way all the communication will happen within one message to the runtime api subsystem and max two runtime calls.
@tdimitrov tdimitrov added T0-node This PR/Issue is related to the topic “node”. I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. T8-polkadot This PR/Issue is related to/affects the Polkadot network. labels Mar 20, 2024
@sandreim
Copy link
Contributor

sandreim commented Mar 21, 2024

I'd like to propose option #3 which aims at moving all of the logic related to runtime api versioning in runtime-api subsystem.

Implement the logic of detecting runtime versions changes in the runtime-api subsystem:

  • process new blocks coming in by ActiveLeaves updates
  • scan each leaf for CodeUpdated system event
  • if CodeUpdated event present, query the runtime API version and cache the value
  • add RuntimeApiMessage::VersionAt(relay_block_hash, sender) which returns the runtime api version from the local cache
  • modify the implementation of has_required_runtime to make use of RuntimeApiMessage::CurrentVersion

This has the downside of always spawning the runtime to get events at each block, not sure there is a way to avoid that.

@sandreim
Copy link
Contributor

sandreim commented Mar 21, 2024

I'd like to propose option #3 which aims at moving all of the logic related to runtime api versioning in runtime-api subsystem.

Implement the logic of detecting runtime versions changes in the runtime-api subsystem:

  • process new blocks coming in by ActiveLeaves updates
  • scan each leaf for CodeUpdated system event
  • if CodeUpdated event present, query the runtime API version and cache the value
  • add RuntimeApiMessage::VersionAt(relay_block_hash, sender) which returns the runtime api version from the local cache
  • modify the implementation of has_required_runtime to make use of RuntimeApiMessage::CurrentVersion

This has the downside of always spawning the runtime to get events at each block, not sure there is a way to avoid that.

Maybe there is no need to look for the CodeUpdated event, and just ask the runtime for the version.

@tdimitrov
Copy link
Contributor Author

Looking for CoreUpdated probably will be harder than just querying the runtime version.

The rest is more or less the same at the moment - there is a runtime call Version and each runtime call is cached. So we call Version no more than once per block. But we do can query the runtime version on each new leaf so that we have got the value cached for the actual runtime calls.

modify the implementation of has_required_runtime to make use of RuntimeApiMessage::CurrentVersion

has_required_runtime already queries the runtime version but this is one extra subsystem call which @alindima correctly suggests we can remove.

Another thing we can do is to generate event with the runtime version - once on startup and once on each new version. This way the subsystem won't need to query the runtime and will know which version is available. There is a BUT though - this is true for the latest block. The runtime might need to make a query at an older block where the runtime might be different. Then each subsystem will need to track versions by itself and we'll add unnecessary complexity.

@sandreim
Copy link
Contributor

Indeed, my 3 doesn't really solve much. I'd lean more towards 1 since 2 might be more complicated to implement and maintain over time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. T0-node This PR/Issue is related to the topic “node”. T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
Status: Backlog
Development

No branches or pull requests

2 participants