discuss: no feature gating in trace_decoder

[0xaatif]

No description provided.

[Nashtare]

Overall I'm not really convinced that hiding away network-specific logic with this kind of tricks to not rely on feature-gating is the best approach.
pre-execution is 1 single example, but realistically we should apply this to all network specific pieces of logic, like post-state execution, custom list of precompile addresses, withdrawals, system contracts ... Note also that we would probably need to extend the CLI to inform the network (in the future the HF too), so that zero/ crates can also get rid of feature-gating and feed expected inputs to the decoder, which goes against your idea of simple CLI I believe.

Would love @atanmarko / @muursh, and more generally anyone's opinion on this. Maybe there's another approach getting rid of feature-gating that would be more satisfactory with respect to the points above, but given the flexibility we need the different packages to have, which will keep on increasing as we get informed of additional requirements, etc.., I really think that obfuscating everything internally behind gates so that these custom pieces of logic are abstracted away is the less harmful way.

[0xaatif]

Hi Robin,

Could you help me understand your objection better here?
You mention that there are several other places which would need to be network-aware. Are they fundamentally different in a way that would break this approach?
Perhaps we'd need the code to be aware of different structs at different times (which proposed design cannot be).

As a note, I have no problems with a --network eth-mainnet | polygon-cdk style CLI argument - I think it would be much more elegant than having different binaries that are build with different features. And the relevant option simply gets plumbed through to the entrypoint.

Representing our constraints as data, using the more powerful language of e.g enums is I think much better than the blunt object of #[cfg(feature - ...)]. We're currently hacking on mutual exclusivity to a part of features that was not designed for, nor really supports it.

To be clear, the benefits to developers and users of trace decoder of this change are:

• I don't need to build/run/test with different combinations of feature gates. You may recall fixing a bug where I failed to do this one before: fix: fix compilation with test_only feature zero-bin#84. This makes that kind of oversight impossible
• My IDE knows all my code is live. This is impossible as it stands because our cargo features are mutually exclusive (this goes against cargo's feature guidelines btw - one of a few reasons those guidelines exist). So my feedback loop is either manual, or I have to switch between feature flags.

The main point is that following an if branch is significantly easier than following different #[cfg(feature = ...)] statements, and from my understanding, most of the code outside of evm_arith could make this change with very little product-level consequence (perf), but with a big devx and cognitive complexity upside.

I also think this is a decision with a low architectural risk today. Whether @hratoanina's thinking gives us different design requirements for example, the change is trivial to make to trace decoder if we take this approach.

[Nashtare]

You mention that there are several other places which would need to be network-aware. Are they fundamentally different in a way that would break this approach?

I haven't extensively investigated the approach to be honest, but it does feel weird to me to bloat API with arguments that end up being specific to a given network, that we need to expose through some enums for all variants. FWIW, withdrawals do not exist for non eth_mainnet, ger_data as well for non cdk_erigon. There are more things coming still. We're mixing things up here while I'd ideally advocate for unified, simpler entry points, and have all these specific logic be done internally.

I think it would be much more elegant than having different binaries that are build with different features.

The thing is, we will have different binaries anyway, because evm_arithmetization heavily relies on this, and we had a consensus on relying on feature-gating for this crate at least.

I don't need to build/run/test with different combinations of feature gates. You may recall fixing a bug where I failed to do this one before: 0xPolygonZero/zero-bin#84. This makes that kind of oversight impossible

To be fair, I think this is a non-issue. Our CI is currently pretty poor, and this issue you mentioned arised because we were not testing a feature we were supporting which is a big red flag. We shouldn't have any supported feature untested.

but with a big devx and cognitive complexity upside.

I would say this is a personal take here. I personally find your approach (taken as a whole, not just this limited example but applied to all the items I mentioned on slack), more confusing than internally handling them in a "silent" way. That being said, it's true that if your IDE greys out feature-specific code, this isn't ideal (although tweaking the default feature temporarily would alleviate the issue).

[hratoanina]

I can mostly talk about the evm_arithmetization crate since that's what I've been working with most of the time.
Something without feature-gating seems achievable, but from what I'm seeing it would add a lot of circuitry (new CLI arguments, new generic traits, new fields, many if and match branches...), and would also incur run-time overhead (I can't tell if it would be significant or not, but I think it would at least not be prohibitive). Feature-gating operates with the same logic of these if/match branches, but without introducing any of these extras and without the runtime overhead.

I can understand how no feature-gating makes for a cleaner approach, and for a mature product with a stabilized API and set of features maybe it could be considered, but while we're still actively on the features I can definitely say that feature-gating makes the work easier. I can't project myself in the future to see if this would make maintaining the code base harder long-term, but it doesn't feel like it either.
I also don't think that it has to be a binary decision, at least for now, and we can still have individual discussions on individual structs or parts of the code to see if there are painless ways of reducing feature-gating.

[Nashtare]

The main point is that following an if branch is significantly easier than following different #[cfg(feature = ...)] statements

@0xaatif What do you think of a middle ground, still explicitly highlighting the need for a specific network feature without clogging the API with multiple enums that end up being specific to only a small subset. Something like this:

Original

#[allow(unused_variables)]
/// Performs all the pre-txn execution rules of the targeted network.
fn do_pre_execution<StateTrieT: StateTrie + Clone>(
    block: &BlockMetadata,
    ger_data: Option<(H256, H256)>,
    storage: &mut BTreeMap<H256, StorageTrie>,
    trim_storage: &mut BTreeMap<ethereum_types::H160, BTreeSet<TrieKey>>,
    trim_state: &mut BTreeSet<TrieKey>,
    state_trie: &mut StateTrieT,
) -> anyhow::Result<()> {
    // Ethereum mainnet: EIP-4788
    #[cfg(feature = "eth_mainnet")]
    do_beacon_hook(
        block.block_timestamp,
        storage,
        trim_storage,
        block.parent_beacon_block_root,
        trim_state,
        state_trie,
    )?;

    #[cfg(feature = "cdk_erigon")]
    do_scalable_hook(
        block,
        ger_data,
        storage,
        trim_storage,
        trim_state,
        state_trie,
    )?;

    Ok(())
}

Proposed

/// Performs all the pre-txn execution rules of the targeted network.
fn do_pre_execution<StateTrieT: StateTrie + Clone>(
    block: &BlockMetadata,
    ger_data: Option<(H256, H256)>,
    storage: &mut BTreeMap<H256, StorageTrie>,
    trim_storage: &mut BTreeMap<ethereum_types::H160, BTreeSet<TrieKey>>,
    trim_state: &mut BTreeSet<TrieKey>,
    state_trie: &mut StateTrieT,
) -> anyhow::Result<()> {
    // Ethereum mainnet: EIP-4788
    if cfg!(feature = "eth_mainnet") {
        return do_beacon_hook(
            block.block_timestamp,
            storage,
            trim_storage,
            block.parent_beacon_block_root,
            trim_state,
            state_trie,
        );
    }

    if cfg!(feature = "cdk_erigon") {
        return do_scalable_hook(
            block,
            ger_data,
            storage,
            trim_storage,
            trim_state,
            state_trie,
        );
    };

    Ok(())
}

This should also solve your IDE problem, as it keeps the blocks in the binary (even if the condition is compile-time evaluated)

[atanmarko]

Not really my area, FWIW no issues from the zero side.

[0xaatif]

Happy with the middle ground for now :) thanks robin!