paradigmxyz / reth

Modular, contributor-friendly and blazing-fast implementation of the Ethereum protocol, in Rust
https://reth.rs/
Apache License 2.0
3.59k stars 945 forks source link

Switch feature logic from `zstd-codec` to `no-zstd-codec` #8889

Closed joshieDo closed 2 weeks ago

joshieDo commented 1 month ago

i think it would be safer to flip the feature logic by instead of requiring zstd-codec feature to enable compression, require no-zstd-codec feature to disable compression. It would unblock this PR without any breaking change. cc @JackG-eth

Originally posted by @joshieDo in https://github.com/paradigmxyz/reth/issues/8867#issuecomment-2173326475

JackG-eth commented 1 month ago

@joshieDo want me to pick this up then?

joshieDo commented 1 month ago

if you want, sure!

JackG-eth commented 1 month ago

@mattsse I think we need a little more discussion on this given some areas have features that rely on the logic already being flipped?

JackG-eth commented 1 month ago

@mattsse Should this be blocked right now given our previous discussions?

joshieDo commented 1 month ago

I think we need a little more discussion on this given some areas have features that rely on the logic already being flipped?

For example?

JackG-eth commented 1 month ago

I think we need a little more discussion on this given some areas have features that rely on the logic already being flipped?

For example?

We have a scenario where its flipped here:

https://github.com/paradigmxyz/reth/blob/c23fe39dd3488294afe206e53b6d8ff7702dd2be/crates/primitives/src/transaction/mod.rs#L909

https://github.com/paradigmxyz/reth/blob/c23fe39dd3488294afe206e53b6d8ff7702dd2be/crates/primitives/src/transaction/mod.rs#L845

Maybe i got confused but would the logic be to revert it like so?:

zstd-codec -> no-zstd-codec

#[cfg(feature = "zstd-codec")]   -> #[cfg(not(feature = "no-zstd-codec"))] 

#[cfg(not(feature = "zstd-codec"))]   -> #[cfg(feature = "no-zstd-codec")] 

When i was speaking to Matt he wasn't sure we could make this opt-out. What do you think?

mattsse commented 2 weeks ago

closing in favor of #9486