search

paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.com/
1.89k stars 695 forks source link

Improper error type is returned when a runtime API method is not available #217

Open s0me0ne-unkn0wn opened 1 year ago

s0me0ne-unkn0wn commented 1 year ago

Is there an existing issue?

Experiencing problems? Have you tried our Stack Exchange first?

Description of bug

When a non-existing runtime API method call is requested, the error returned is of type Other(String).

At that point, the information about why the error happened is lost, and that doesn't leave the caller any other chance besides treating it as a RuntimeApiError::Execution. That is, the caller knows that an error has happened when trying to execute the runtime call. Still, it doesn't know if that's a problem with the runtime call logic itself or if the current runtime does not support the runtime call requested.

That may cause problems with the current code, as well as with the future one. For example, the newer disputes logic may rely upon the fact the executor should return NotSupported error instead of the Execution one. The NotSupported error variant is quite intuitive and incentives the caller to use it to check if the runtime call is supported at all; nevertheless, the only case when it's really used is checking if the runtime supports runtime API versioning.

The proposal is to return the MethodNotFound variant and to further map it to the RuntimeApiError::NotSupported to make things more consistent.

See also: https://github.com/paritytech/polkadot/pull/6161#discussion_r1087983694

bkchr commented 1 year ago

Yes the error variant should be improved. However, the caller already can use https://docs.rs/sp-api/latest/sp_api/trait.ApiExt.html#tymethod.api_version to check if the runtime supports the expected version.

s0me0ne-unkn0wn commented 1 year ago

Yes, that's totally legit. But I mean, why do two runtime calls instead of one? Also, it's pretty counter-intuitive to get the Execution error instead of the NotSupported one when the runtime call is not supported.

bkchr commented 1 year ago

But I mean, why do two runtime calls instead of one?

api_version etc are all no runtime calls. These checks are done based on the runtime version and this is cached internally. So, these checks are really cheap.

s0me0ne-unkn0wn commented 1 year ago

Okay, I get it. But here is some further investigation:

The network was running rococo-9360 runtime, which declares its support for parachain host API version 3. But 9360 is still missing the session_executor_params call, which will (hopefully) be introduced in 9380 without the API version bump.

So version checking is a valuable mechanism (and it is used and is working), but it is unreliable for staging changes unless they involve API version bump.

ordian commented 1 year ago

Once we add some method as stable (e.g. disputes as api_version(3)), there's a risk someone will cut a release with that version (this is what happened with Rococo/Versi). Which is why I suggested to add additional methods as version 4.

We've discussed in the past using a very high number for staging methods, e.g. 1000. But that doesn't help in this case for Rococo. A proper mitigation would be having two separate versions: api_version and api_staging_version which can be bumped independently. api_staging_version on mainnet runtimes would be at 0.

s0me0ne-unkn0wn commented 1 year ago

Once we add some method as stable (e.g. disputes as api_version(3))

But I believe api_version(3) is not stable yet, it's only implemented by Rococo and Westend.

We wanted to stabilize it because of SessionInfo changes but reverted that later.