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

PVF: refactor errors to use thiserror #2157

Closed
mrcnski opened this issue Nov 5, 2023 · 8 comments · Fixed by #2958
Closed

PVF: refactor errors to use thiserror #2157

mrcnski opened this issue Nov 5, 2023 · 8 comments · Fixed by #2958
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. T0-node This PR/Issue is related to the topic “node”.

Comments

@mrcnski
Copy link
Contributor

mrcnski commented Nov 5, 2023

We're already using thiserror in some places, so we should consider expanding its usage to reduce boilerplate. For example, we could add #[from] attributes for the variants of ValidationError, making it easier to construct. For sure another candidate is PrepareError, we could provide an auto conversion for e.g. IoErr, reducing the boilerplate whenever it's constructed. Then instead of a bunch of map_errs on std::io::Error, we can just use ?.

See here for an example thiserror usage. cc @eagr since you were interested in some refactorings. :)

@mrcnski mrcnski added the T0-node This PR/Issue is related to the topic “node”. label Nov 5, 2023
@mrcnski mrcnski moved this to Backlog in parachains team board Nov 5, 2023
@mrcnski mrcnski added the C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. label Nov 26, 2023
@maksimryndin
Copy link
Contributor

Hello @mrcnski ! Can I take this issue as the first one to familiarize myself with the codebase?

@mrcnski
Copy link
Contributor Author

mrcnski commented Jan 15, 2024

@maksimryndin Hey 👋🏻 That would be great! Please let me know if you have any questions.

If it helps, we have a README in the pvf/ crate, some module docs (run cargo doc --open --document-private-items to make sure you see them all), and some docs in https://github.com/paritytech/polkadot-sdk/tree/master/docs/contributor as well.

@maksimryndin
Copy link
Contributor

maksimryndin commented Jan 15, 2024

Thank you @mrcnski So a roadmap for now

@maksimryndin
Copy link
Contributor

@mrcnski 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 because

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

Output of cargo expand

// before

PrepareError::JobDied { ref err, ref job_pid } => {
    __codec_dest_edqy.push_byte(10usize as ::core::primitive::u8);
    ::parity_scale_codec::Encode::encode_to(err, __codec_dest_edqy);
    ::parity_scale_codec::Encode::encode_to(
        job_pid,
        __codec_dest_edqy,
    );
}
PrepareError::Kernel(ref aa) => {
    __codec_dest_edqy.push_byte(10u8 as ::core::primitive::u8);
    ::parity_scale_codec::Encode::encode_to(aa, __codec_dest_edqy);
}

// after

PrepareError::JobDied { ref err, ref job_pid } => {
    __codec_dest_edqy.push_byte(10u8 as ::core::primitive::u8);
    ::parity_scale_codec::Encode::encode_to(err, __codec_dest_edqy);
    ::parity_scale_codec::Encode::encode_to(
        job_pid,
        __codec_dest_edqy,
    );
}
PrepareError::Kernel(ref aa) => {
    __codec_dest_edqy.push_byte(11u8 as ::core::primitive::u8);
    ::parity_scale_codec::Encode::encode_to(aa, __codec_dest_edqy);
}

@maksimryndin maksimryndin mentioned this issue Jan 17, 2024
3 tasks
@maksimryndin
Copy link
Contributor

maksimryndin commented Jan 17, 2024

@mrcnski I am not sure on labels as I think it is also worth to add R0-silent and T11-documentation (although I've checked myself the fixed links)

PR here #2958

I need some time to prepare a proper working environment as it requires Linux amd64 and more free disk space in order to run integration tests

@mrcnski
Copy link
Contributor Author

mrcnski commented Jan 17, 2024

I need some time to prepare a proper working environment as it requires Linux amd64 and more free disk space in order to run integration tests

Yes, unfortunately getting this environment set up and testing on it can be a pain. I rented a VPS, but it is really slow to compile.

I think you don't need it for this particular PR though. Most of the Linux-amd64-only tests are testing things like spawning processes, security features, etc. And if there is a failing test, CI will catch it for you.

@maksimryndin
Copy link
Contributor

Thank you for a warm welcome @mrcnski :)

@mrcnski
Copy link
Contributor Author

mrcnski commented Jan 17, 2024

No problem @maksimryndin, let me know if you have any questions. And I appreciate the thoroughness!

github-merge-queue bot pushed a commit that referenced this issue Jan 19, 2024
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>
github-merge-queue bot pushed a commit that referenced this issue Jan 19, 2024
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>
@github-project-automation github-project-automation bot moved this from Backlog to Completed in parachains team board Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

2 participants