paradigmxyz / reth

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

Add difficulty/pow ethhash checks in consensus pre merge #9476

Open mattsse opened 3 months ago

mattsse commented 3 months ago

Describe the feature

https://github.com/paradigmxyz/reth/blob/d943e782947014a530384c827588cb4db35e86f4/crates/ethereum/consensus/src/lib.rs#L186-L188

TODO

geth ref: https://github.com/ethereum/go-ethereum/blob/37590b2c5579c36d846c788c70861685b0ea240e/consensus/ethash/consensus.go#L308-L311

Additional context

No response

SozinM commented 3 months ago

Can i take this? 🥺

mattsse commented 3 months ago

nice, assigned, ty!

I think we need to port the various difficulty functions first and then select them based on the timestamp

SozinM commented 3 months ago

@mattsse there are also faster implementations for difficulty calculators in https://github.com/ethereum/go-ethereum/blob/37590b2c5579c36d846c788c70861685b0ea240e/consensus/ethash/difficulty.go They were written and tested but not used (https://github.com/ethereum/go-ethereum/pull/21976). Is it better to port them instead? I don't like the fact that they are not used in the code tho.

SozinM commented 3 months ago

I implemented the frontier calculator so it could be looked at to make sure it looks right. I don't see any applications for returning errors from the calculator so I used Result<U256, ()> for now.

SozinM commented 3 months ago

Wrote code part of the calculators. I will add their usage and tests next.

SozinM commented 3 months ago

I will be on leave for 2 weeks so if this functionality is not urgent I will complete it once I return, sorry for the inconveniences :)

SozinM commented 3 months ago

@mattsse I think we should put the calc checks into the validate_header_against_parent and not in the validate_header_with_total_difficulty, WDYT? Otherwise, it's quite tricky to integrate as we need a parent header to calculate difficulties

SozinM commented 3 months ago

Almost there, The last fight I have is that in e2e tests generated chain mock has blocks difficulty=0x0 and it causes problems because now we have validation in place After that I will test everything and push it onto review

SozinM commented 3 months ago

Found interesting problem While validating payloads against the generated block: https://github.com/paradigmxyz/reth/blob/603e39ab74509e0863fc023461a4c760fb2126d1/crates/payload/validator/src/lib.rs#L111 We include difficulty=0 into the payload: https://github.com/paradigmxyz/reth/blob/603e39ab74509e0863fc023461a4c760fb2126d1/crates/rpc/rpc-types-compat/src/engine/payload.rs#L16 This causes all new generated block with difficulty != 0 to have mismatched hash, because difficulty included in block hashing process https://github.com/alloy-rs/alloy/blob/56e317d9be3592363e5505dc7971053e15c45af1/crates/consensus/src/header.rs#L349

@mattsse do you have any ideas on how to fix it fundamentally? It needed to fix tests for validation, but it doesn't affect payload generation itself (because POS) I don't really see any easy ways except for crutches (like disabling difficulty validation in tests)

P.S. seems like I forgot to skip the difficulty check after POS is activated and it fixes everything

SozinM commented 2 months ago

Okay, tests passed, i will sync the new node up to POS activation, write some more tests now

SozinM commented 2 months ago

I added plenty of tests covering all transitions and the majority of branches. Having a problem syncing the reth node, for some reason it doesn't want to find peers if synced from the start, still troubleshooting it.

This PR could be reviewed in the meantime

SozinM commented 2 months ago

Node almost synced to the Merge, so far so good, in the meantime I'll fix the tests and push to PR once it's reach POS.

SozinM commented 2 months ago

Now it's ready for review :) Node with these changes has fully synced on mainnet, all good. I fixed a bunch of test problems along the way. The main change to be careful about is adding a block to the Paris TTD condition. Without a block, it's impossible to account for merge when you compare block headers (because you don't have to). This addition broke a bunch of tests because previously they were skipping Paris (as the block field was None) @mattsse