Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Log UnknownBlock errors from runtime #6288

Closed
wants to merge 2 commits into from
Closed

Conversation

tdimitrov
Copy link
Contributor

@tdimitrov tdimitrov commented Nov 15, 2022

This PR is building upon paritytech/substrate#12707 and is related to paritytech/polkadot-sdk#793

The problem

All runtime calls first check the runtime version before issuing the actual runtime api call. If a call is made for a block which is pruned (e.g. because a better fork was finalized), the runtime api version can't be checked and NotSupported error is returned. This is misleading because the problem is different. The call IS supported but it can't be executed at a pruned block.

How this is solved

This PR adds explicit logging for UnknownBlock runtime errors so that it is clear why the runtime call is failing. Furthermore UnknownBlock variant is added to RuntimeApiError enum in node/subsystem-types/src/errors.rs. This allows any subsystem to handle such errors in its own way.

What this PR doesn't do

The PR doesn't fix the root cause (prevent executing logic on pruned blocks).

Sample logs

E.g. for {node=~"versi-validator-f.*"} |~ "UnknownBlock|discarded":

image

@tdimitrov tdimitrov changed the title Test PR: Handle UnknownBlock error during runtime calls gracefully Test PR: Handle UnknownBlock error from runtime calls gracefully Nov 15, 2022
@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 15, 2022
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2315557

@tdimitrov tdimitrov changed the title Test PR: Handle UnknownBlock error from runtime calls gracefully Log UnknownBlock errors from runtime Jan 31, 2023
@tdimitrov tdimitrov marked this pull request as ready for review January 31, 2023 08:40
@tdimitrov tdimitrov added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. T5-parachains_protocol This PR/Issue is related to Parachains features and protocol changes. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 31, 2023
@ordian
Copy link
Member

ordian commented Feb 2, 2023

What is the rationale for using warns for such errors?

The PR doesn't fix the root cause (prevent executing logic on pruned blocks).

How is it possible to do that in general case? Wouldn't you have a TOCTOU problem?

Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood the point of the issue was to make the error more explicit and not silence it. However, I am still not sure what would the point of warning the user about this if this error is expected to happen as we already observe in the logs.

Copy link
Contributor

@BradleyOlson64 BradleyOlson64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks clean!

node/core/runtime-api/src/lib.rs Show resolved Hide resolved
@tdimitrov
Copy link
Contributor Author

These logs doesn't contribute much to the understandably of the logs. I'm closing the PR.

@tdimitrov tdimitrov closed this May 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. T5-parachains_protocol This PR/Issue is related to Parachains features and protocol changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants