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

More codec edge cases #555

Open
mrcnski opened this issue Jan 2, 2024 · 1 comment
Open

More codec edge cases #555

mrcnski opened this issue Jan 2, 2024 · 1 comment

Comments

@mrcnski
Copy link
Contributor

mrcnski commented Jan 2, 2024

#507 covers one but there are two more:

  • Two codec attributes can be applied to the same variant with no warning,
  • meanwhile a variant can have no codec attribute and there is no warning.

For example:

enum Foo {
    #[codec(index = 0)]
    #[codec(index = 1)]
    A,
    B,
}

This kind of bug can happen due to a copy/paste mistake or a mistake when resolving a merge conflict (as it did to me). When there are many attributes this kind of mistake can be hard to spot. In my case it resulted in the decoded value being different from the original value before encoding, which made for a fun debugging session.

@mrcnski
Copy link
Contributor Author

mrcnski commented Jan 17, 2024

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);
}

Thanks @maksimryndin for researching it!

github-merge-queue bot pushed a commit to paritytech/polkadot-sdk 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
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk 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 <[email protected]>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk 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 <[email protected]>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk 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 <[email protected]>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant