Pvf thiserror #2958

maksimryndin · 2024-01-17T10:20:37Z

resolve #2157

fix broken doc links
fix codec macro typo https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/node/core/pvf/common/src/error.rs#L81 (see the comment below)
refactor ValidationError, PrepareError and related error types to use thiserror crate

`codec` issue

codec macro was mistakenly applied two times to Kernel error (so it was encoded with 10 instead of 11 and the same as JobDied). The PR changes it to 11 because

it was an initial goal of the code author
Kernel is less frequent than JobDied so in case of existing error encoding it is more probable to have 10 as JobDied than Kernel

See paritytech/parity-scale-codec#555

polkadot address: 13zCyRG2a1W2ih5SioL8byqmQ6mc8vkgFwQgVzJSdRUUmp46

…validCandidate,PossiblyInvalidError; fix codec macro for PrepareError

mrcnski · 2024-01-17T10:50:53Z

codec macro was mistakenly applied two times to Kernel error (so it was encoded with 10 instead of 11 and the same as JobDied). I changed it to 11

Thank you! You're absolutely right, and sorry if it caused any confusion. It is already being fixed in #2477, and I've opened an issue about it on parity-scale-codec. Appreciate you fixing it here as well!

mrcnski

Looks great, thank you! If you want a tip, you can include your kusama or polkadot address in the PR body:

https://github.com/paritytech/substrate-tip-bot#pull-request-body

I'm interested in whether we can now better log some of these errors that didn't implement the Display trait before. We should be able to log them with {} instead of {:?}. I'll look through the codebase later for such instances.

mrcnski · 2024-01-17T11:01:10Z

polkadot/node/core/pvf/common/src/error.rs

+	#[error("panic: {0}")]
 	JobError(String),


Suggested change

#[error("panic: {0}")]

JobError(String),

#[error("prepare job error: {0}")]

JobError(String),

(That's my bad, I forgot to update this line after renaming Panic -> JobError.

For sure I will update:) I've noticed it but from the book description it was quite logical

For consistency we could also make a prepare: prefix for other messages in PrepareError if it is allowed

I'm interested in whether we can now better log some of these errors that didn't implement the Display trait before. We should be able to log them with {} instead of {:?}. I'll look through the codebase later for such instances.

It is a good point! Thank you! I will check these instances within the PR

@mrcnski I've checked the usage of {:?} within pvf/ and changed several ones to {}. And also changed PrepareError messages a little bit for consistency.

mrcnski · 2024-01-17T11:26:48Z

polkadot/node/core/pvf/src/error.rs

+	#[error(transparent)]
 	Preparation(PrepareError),
 	/// The error was raised because the candidate is invalid. Should vote against.
-	Invalid(InvalidCandidate),
+	#[error(transparent)]


Can we have specific error messages here instead of the fall-through transparent?

Let me explain the motivation:)

InternalValidationError already has validation: prefix in its messages

ValidationError has 1-1 match between enum variants and wrapped sub errors

~~As a novice to the codebase, I am not sure how much freedom I have over the error messages~~

So I decided to stick with the safest solution in my opinion. But of course, if you think we should expand messages on wrapper, I will do :)

Good points! If you want though, you can change this:

For preparation, add a message like preparation error for validation: {0}

For internal error, maybe change the message to internal validation error: {0}

As a novice to the codebase, I am not sure how much freedom I have over the error messages

Appreciate the humility and respect for the codebase. :)

Also not sure if the change is necessary TBH so I'll leave it to your judgment. I think it's fine to err on the side of being a bit too verbose: error messages are rare and the more info they contain, the better.

Agree! I've decided to use candidate validation: prefix for ValidationError. Thus it looks consistent

s0me0ne-unkn0wn

A good cleanup! Makes things more clear.

If you ever want to continue working on cleaning up the error processing, we have a long-waiting #752 ;) (although I believe it should be updated and discussed further).

maksimryndin · 2024-01-17T14:52:40Z

A good cleanup! Makes things more clear.

If you ever want to continue working on cleaning up the error processing, we have a long-waiting #752 ;) (although I believe it should be updated and discussed further).

Thank you!:) Perhaps, I will also try that but later. The coding part is not a problem here but my lack of domain knowledge and lack of usage experience which would benefit to the mentioned task.

mrcnski · 2024-01-18T13:09:21Z

/tip small

substrate-tip-bot · 2024-01-18T13:09:53Z

@mrcnski A small (20 DOT) tip was successfully submitted for @maksimryndin (13zCyRG2a1W2ih5SioL8byqmQ6mc8vkgFwQgVzJSdRUUmp46 on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/referenda

mrcnski

Looks awesome, just one optional comment and then we can merge!

resolve #2157 - [x] fix broken doc links - [x] fix codec macro typo https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/node/core/pvf/common/src/error.rs#L81 (see the comment below) - [x] refactor `ValidationError`, `PrepareError` and related error types to use `thiserror` crate ## `codec` issue `codec` macro was mistakenly applied two times to `Kernel` error (so it was encoded with 10 instead of 11 and the same as `JobDied`). The PR changes it to 11 because - it was an initial goal of the code author - Kernel is less frequent than JobDied so in case of existing error encoding it is more probable to have 10 as JobDied than Kernel See paritytech/parity-scale-codec#555 ---- polkadot address: 13zCyRG2a1W2ih5SioL8byqmQ6mc8vkgFwQgVzJSdRUUmp46 --------- Co-authored-by: s0me0ne-unkn0wn <48632512+s0me0ne-unkn0wn@users.noreply.github.com>

maksimryndin added 3 commits January 17, 2024 11:12

fix documentation links

7459c3c

cargo fmt docs changes

d013a67

thiserror for PrepareError,InternalValidationError,ValidationError,In…

9edbb1d

…validCandidate,PossiblyInvalidError; fix codec macro for PrepareError

maksimryndin mentioned this pull request Jan 17, 2024

PVF: refactor errors to use thiserror #2157

Closed

mrcnski requested review from s0me0ne-unkn0wn and mrcnski January 17, 2024 10:45

mrcnski assigned maksimryndin Jan 17, 2024

mrcnski added T0-node This PR/Issue is related to the topic “node”. I4-refactor Code needs refactoring. R0-silent Changes should not be mentioned in any release notes T11-documentation This PR/Issue is related to documentation. labels Jan 17, 2024

mrcnski approved these changes Jan 17, 2024

View reviewed changes

s0me0ne-unkn0wn approved these changes Jan 17, 2024

View reviewed changes

maksimryndin added 2 commits January 17, 2024 15:23

review feedback on thiserror use for errors in pvf

d75bf14

Merge branch 'master' into pvf-thiserror

9af3376

maksimryndin marked this pull request as ready for review January 17, 2024 14:36

mrcnski approved these changes Jan 18, 2024

View reviewed changes

maksimryndin added 2 commits January 18, 2024 15:30

validation error display prefix

471a768

Merge branch 'master' into pvf-thiserror

d4e7017

mrcnski added this pull request to the merge queue Jan 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 19, 2024

Merge branch 'master' into pvf-thiserror

031b2d8

mrcnski added this pull request to the merge queue Jan 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 19, 2024

mrcnski added this pull request to the merge queue Jan 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 19, 2024

mrcnski added this pull request to the merge queue Jan 19, 2024

Merged via the queue into paritytech:master with commit 47a3faa Jan 19, 2024
124 checks passed

maksimryndin mentioned this pull request Jan 19, 2024

Tips are not received for a newly created DOT account paritytech/substrate-tip-bot#133

Closed

github-actions bot mentioned this pull request Mar 13, 2024

Update polkadot-sdk from v1.3.0 to v1.7.2 moonbeam-foundation/moonbeam#2703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pvf thiserror #2958

Pvf thiserror #2958

maksimryndin commented Jan 17, 2024 •

edited

Loading

mrcnski commented Jan 17, 2024

mrcnski left a comment

mrcnski Jan 17, 2024

maksimryndin Jan 17, 2024 •

edited

Loading

maksimryndin Jan 17, 2024

maksimryndin Jan 17, 2024 •

edited

Loading

maksimryndin Jan 17, 2024

mrcnski Jan 17, 2024

maksimryndin Jan 17, 2024 •

edited

Loading

mrcnski Jan 18, 2024

mrcnski Jan 18, 2024

maksimryndin Jan 18, 2024

s0me0ne-unkn0wn left a comment

maksimryndin commented Jan 17, 2024

mrcnski commented Jan 18, 2024

substrate-tip-bot bot commented Jan 18, 2024

mrcnski left a comment

Pvf thiserror #2958

Pvf thiserror #2958

Conversation

maksimryndin commented Jan 17, 2024 • edited Loading

codec issue

mrcnski commented Jan 17, 2024

mrcnski left a comment

Choose a reason for hiding this comment

mrcnski Jan 17, 2024

Choose a reason for hiding this comment

maksimryndin Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

maksimryndin Jan 17, 2024

Choose a reason for hiding this comment

maksimryndin Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

maksimryndin Jan 17, 2024

Choose a reason for hiding this comment

mrcnski Jan 17, 2024

Choose a reason for hiding this comment

maksimryndin Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

mrcnski Jan 18, 2024

Choose a reason for hiding this comment

mrcnski Jan 18, 2024

Choose a reason for hiding this comment

maksimryndin Jan 18, 2024

Choose a reason for hiding this comment

s0me0ne-unkn0wn left a comment

Choose a reason for hiding this comment

maksimryndin commented Jan 17, 2024

mrcnski commented Jan 18, 2024

substrate-tip-bot bot commented Jan 18, 2024

mrcnski left a comment

Choose a reason for hiding this comment

maksimryndin commented Jan 17, 2024 •

edited

Loading

`codec` issue

maksimryndin Jan 17, 2024 •

edited

Loading

maksimryndin Jan 17, 2024 •

edited

Loading

maksimryndin Jan 17, 2024 •

edited

Loading