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

fix(codec): fix last field compilation check #3543

Merged
merged 4 commits into from
Jul 3, 2023
Merged

fix(codec): fix last field compilation check #3543

merged 4 commits into from
Jul 3, 2023

Conversation

joshieDo
Copy link
Collaborator

@joshieDo joshieDo commented Jul 3, 2023

Fixes a previously modified last field check on compact codec and adds additional context to main_codec

This was probably changed to allow CheckpointBlockRange to be compiled. The necessary changes were introduced, and it doesn't affect the actual encoding of the value (not a db breaking change).

I added a bit more context on the relevant assertion:

 ! Be careful before changing the following assert ! Especially if the type does not
 implement proptest tests.

 The limitation of the last placed field applies to fields with potentially large sizes,
 like the `Transaction` field. These fields may have inner "Bytes" fields, sometimes even
 nested further, making it impossible to check with `proc_macro`. The goal is to place
 such fields as the last ones, so we don't need to store their length separately. Instead,
 we can simply read them until the end of the buffer.

 However, certain types don't require this approach because they don't contain inner
 "Bytes" fields. For these types, we can add them to a "known_types" list so it doesn't
 trigger this error. These types can handle their own deserialization without
 relying on the length provided by the higher-level deserializer. For example, a
 type "T" with two "u64" fields doesn't need the length parameter from
 "T::from_compact(buf, len)" since the length of "u64" is known internally (bitpacked).

@joshieDo joshieDo requested a review from rakita as a code owner July 3, 2023 11:07
@joshieDo joshieDo added C-bug An unexpected or incorrect behavior A-db Related to the database labels Jul 3, 2023
Copy link
Collaborator

@shekhirin shekhirin left a comment

Choose a reason for hiding this comment

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

thank you 🙏
also love the comment explaining the behaviour

@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Merging #3543 (9effb54) into main (4f32f56) will decrease coverage by 35.26%.
The diff coverage is 87.50%.

Impacted file tree graph

Impacted Files Coverage Δ
crates/storage/codecs/derive/src/lib.rs 68.10% <ø> (ø)
...rates/storage/codecs/derive/src/compact/structs.rs 81.81% <50.00%> (+0.23%) ⬆️
...tes/storage/codecs/derive/src/compact/generator.rs 98.91% <100.00%> (+0.03%) ⬆️

... and 341 files with indirect coverage changes

Flag Coverage Δ
integration-tests ?
unit-tests 33.69% <87.50%> (-30.33%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 15.34% <ø> (-7.68%) ⬇️
blockchain tree 29.79% <ø> (-51.47%) ⬇️
pipeline 45.72% <ø> (-41.27%) ⬇️
storage (db) 36.76% <87.50%> (-37.07%) ⬇️
trie 41.92% <ø> (-53.73%) ⬇️
txpool 26.37% <ø> (-24.78%) ⬇️
networking 39.17% <ø> (-38.70%) ⬇️
rpc 15.38% <ø> (-42.50%) ⬇️
consensus 35.92% <ø> (-26.67%) ⬇️
revm 19.57% <ø> (-15.43%) ⬇️
payload builder 4.00% <ø> (-2.84%) ⬇️
primitives 50.25% <ø> (-38.26%) ⬇️

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

merging - clippy unrelated

@@ -116,17 +116,26 @@ impl<'a> StructHandler<'a> {
format_ident!("specialized_from_compact")
};

// ! Be careful before changing the following assert ! Especially if the type does not
Copy link
Member

Choose a reason for hiding this comment

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

love it - seems fine to have footgun at codec-related logic, is low level enough and i like that we're highlighting it

@gakonst gakonst merged commit 73eeca0 into main Jul 3, 2023
@gakonst gakonst deleted the derive/last-fix branch July 3, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-db Related to the database C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants