Delete legacy payload reconstruction #6213

michaelsproul · 2024-08-02T07:28:11Z

Issue Addressed

I noticed that even after recent changes (see linked PR) to remove ethers, we still have it in the Cargo.lock.

Remove use of ethers core #5301

Proposed Changes

ethers was still being used for two things:

Legacy payload reconstruction, which dates to the time before the existence of the getPayloadBodies methods.
Some extra tests in execution-integration-tests

This PR addresses just the first of these uses. We can update the execution-integration-tests in a follow-up PR.

I've opted just to delete the legacy payload reconstruction, as the only place where it was marginally useful was in bellatrix_readiness for checking genesis validity. Even with it gone, there are still some checks on Bellatrix genesis that should prevent mistakes.

IMO the amount of work required to rewrite this logic into alloy is not worth it at the moment.

Additional Info

ethers depends on ethereum-types, so this PR also helps towards:

Switch Lighthouse and all deps off ethereum-types #6022

beacon_node/beacon_chain/src/beacon_block_streamer.rs

michaelsproul · 2024-09-06T00:59:33Z

beacon_node/execution_layer/src/lib.rs

+        if capabilities.get_payload_bodies_by_range_v1
+            || capabilities.get_payload_bodies_by_range_v2


This is a pre-emptive bug fix. If ELs remove support for v1 in future we still want to use v2 by calling get_payload_bodies_by_range below.

jimmygchen

I've opted just to delete the legacy payload reconstruction, as the only place where it was marginally useful was in bellatrix_readiness for checking genesis validity. Even with it gone, there are still some checks on Bellatrix genesis that should prevent mistakes.
IMO the amount of work required to rewrite this logic into alloy is not worth it at the moment.

Agree with this and the change looks great! -600 less lines of code 🔪

jimmygchen · 2024-09-06T07:26:17Z

I was hoping we can remove ethers-core from execution_layer but looks like some tests still use it. Can address this in a seprate PR.

jimmygchen · 2024-09-06T07:26:23Z

@mergify queue

mergify · 2024-09-06T07:26:50Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at d686138

michaelsproul · 2024-09-06T11:27:50Z

Thanks Jimmy!

Yeah next PR in the line to remove ethers is this one:

Rewrite deposit_contract crate using alloy #6366

Then probably another one to do the execution integration tests

Add retropgf funding (sigp#6324) Migrate from `ethereum-types` to `alloy-primitives` (sigp#6078) * Remove use of ethers_core::RlpStream * Merge branch 'unstable' of https://github.com/sigp/lighthouse into remove_use_of_ethers_core * Remove old code * Simplify keccak call * Remove unused package * Merge branch 'unstable' of https://github.com/ethDreamer/lighthouse into remove_use_of_ethers_core * Merge branch 'unstable' into remove_use_of_ethers_core * Run clippy * Merge branch 'remove_use_of_ethers_core' of https://github.com/dospore/lighthouse into remove_use_of_ethers_core * Check all cargo fmt * migrate to alloy primitives init * fix deps * integrate alloy-primitives * resolve dep issues * more changes based on dep changes * add TODOs * Merge branch 'unstable' of https://github.com/sigp/lighthouse into remove_use_of_ethers_core * Revert lock * Add BeaconBlocksByRange v3 * continue migration * Revert "Add BeaconBlocksByRange v3" This reverts commit e3ce7fc. * impl hash256 extended trait * revert some uneeded diffs * merge conflict resolved * fix subnet id rshift calc * rename to FixedBytesExtended * debugging * Merge branch 'unstable' of https://github.com/sigp/lighthouse into migrate-to-alloy-primitives * fix failed test * fixing more tests * Merge branch 'unstable' of https://github.com/sigp/lighthouse into remove_use_of_ethers_core * introduce a shim to convert between the two u256 types * move alloy to wrokspace * align alloy versions * update * update web3signer test certs * refactor * resolve failing tests * linting * fix graffiti string test * fmt * fix ef test * resolve merge conflicts * remove udep and revert cert * cargo patch * cyclic dep * fix build error * Merge branch 'unstable' of https://github.com/sigp/lighthouse into migrate-to-alloy-primitives * resolve conflicts, update deps * merge unstable * fmt * fix deps * Merge branch 'unstable' of https://github.com/sigp/lighthouse into migrate-to-alloy-primitives * resolve merge conflicts * resolve conflicts, make necessary changes * Remove patch * fmt * remove file * merge conflicts * sneaking in a smol change * bump versions * Merge remote-tracking branch 'origin/unstable' into migrate-to-alloy-primitives * Updates for peerDAS * Update ethereum_hashing to prevent dupe * updated alloy-consensus, removed TODOs * cargo update * endianess fix * Merge branch 'unstable' of https://github.com/sigp/lighthouse into migrate-to-alloy-primitives * fmt * fix merge * fix test * fixed_bytes crate * minor fixes * convert u256 to i64 * panic free mixin to_low_u64_le * from_str_radix * computbe_subnet api and ensuring we use big-endian * Merge branch 'unstable' of https://github.com/sigp/lighthouse into migrate-to-alloy-primitives * fix test * Simplify subnet_id test * Simplify some more tests * Add tests to fixed_bytes crate * Merge branch 'unstable' into migrate-to-alloy-primitives update libp2p to version 0.54 (sigp#6249) * update libp2p to version 0.54.0 * address review * Merge branch 'unstable' of github.com:sigp/lighthouse into update-libp2p * Merge branch 'update-libp2p' of github.com:sigp/lighthouse into update-libp2p Drop block data from BlockError and BlobError (sigp#5735) * Drop block data from BlockError and BlobError * Debug release tests * Fix release tests * Revert unnecessary changes to lighthouse_metrics Update manual checkpoint sync content in Lighthouse book (sigp#6338) * Update manual checkpiont sync * Update faq * Minor revision * Minor revision Light Client Bug Fix (sigp#6299) * Light Client Bug Fix Ignore Rust 1.82 warnings about void patterns (sigp#6357) * Ignore Rust 1.82 warnings about void patterns Enable `large_stack_frames` lint (sigp#6343) * Enable `large_stack_frames` lint Add blob count label to `DATA_COLUMN_SIDECAR_COMPUTATION` metric (sigp#6340) * Add blob count label to `DATA_COLUMN_SIDECAR_COMPUTATION` metric, and move metrics into the compute function, recording only successful computation. * Move `discard_timer_on_break` usage to caller site. * Merge branch 'unstable' into compute-data-column-metric * Merge branch 'unstable' into compute-data-column-metric Metadata request ordering (sigp#6336) * Send metadata request ordering * Merge branch 'unstable' into metadata-order Clarify validator monitor block log (sigp#6342) * Clarify validator monitor block log * Merge branch 'unstable' into clarify-block-log Check known parent on rpc blob process (sigp#5893) * Check known parent on rpc blob process * fix test * Merge branch 'unstable' of https://github.com/sigp/lighthouse into blob-unknown-parent Remove beta tag from gossipsub 1.2 (sigp#6344) * Remove the beta tag from gossipsub v1.2 * fix clippy * Merge branch 'unstable' into remove-beta-tag Fix lints for Rust 1.81 (sigp#6363) * Fix lints for Rust 1.81 Use tikv-jemallocator instead of jemallocator (sigp#6354) * Use tikv-jemallocator instead of jemallocator * Merge branch 'unstable' into tikv-jemallocator * Bump tikv-jemallocator and tikv-jemalloc-ctl Improve `get_custody_columns` validation, caching and error handling (sigp#6308) * Improve `get_custody_columns` validation, caching and error handling. * Merge branch 'unstable' into get-custody-columns-error-handing * Fix failing test and add more test. * Fix failing test and add more test. * Merge branch 'unstable' into get-custody-columns-error-handing * Add unit test to make sure the default specs won't panic on the `compute_custody_requirement_subnets` function. * Add condition when calling `compute_custody_subnets_from_metadata` and update logs. * Validate `csc` when returning from enr. Remove `csc` computation on connection since we get them on metadata anyway. * Add `peers_per_custody_subnet_count` to track peer csc and supernodes. * Disconnect peers with invalid metadata and find other peers instead. * Fix sampling tests. * Merge branch 'unstable' into get-custody-columns-error-handing * Merge branch 'unstable' into get-custody-columns-error-handing Delete legacy payload reconstruction (sigp#6213) * Delete legacy payload reconstruction * Delete unneeded failing test * Merge remote-tracking branch 'origin/unstable' into remove-more-ethers * Merge remote-tracking branch 'origin/unstable' into remove-more-ethers * Cleanups simplify rpc codec logic (sigp#6304) * simplify rpc codec logic * Merge branch 'unstable' of github.com:sigp/lighthouse into simplify-rpc-codec * Merge branch 'unstable' of github.com:sigp/lighthouse into simplify-rpc-codec * Merge branch 'unstable' of github.com:sigp/lighthouse into simply-rpc-codec * Merge branch 'unstable' into simplify-rpc-codec * Merge branch 'unstable' into simplify-rpc-codec

Delete legacy payload reconstruction

b350f18

michaelsproul added work-in-progress PR is a work-in-progress code-quality labels Aug 2, 2024

michaelsproul added 4 commits August 2, 2024 20:12

Delete unneeded failing test

fff92d6

Merge remote-tracking branch 'origin/unstable' into remove-more-ethers

fc59f69

Merge remote-tracking branch 'origin/unstable' into remove-more-ethers

32c288e

Cleanups

5c49ae4

michaelsproul added ready-for-review The code is ready for review v6.0.0 New major release for hierarchical state diffs dependencies Pull requests that update a dependency file and removed work-in-progress PR is a work-in-progress labels Sep 6, 2024

michaelsproul commented Sep 6, 2024

View reviewed changes

beacon_node/beacon_chain/src/beacon_block_streamer.rs Show resolved Hide resolved

michaelsproul commented Sep 6, 2024

View reviewed changes

michaelsproul mentioned this pull request Sep 6, 2024

Rewrite deposit_contract crate using alloy #6366

Open

jimmygchen approved these changes Sep 6, 2024

View reviewed changes

jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Sep 6, 2024

mergify bot added a commit that referenced this pull request Sep 6, 2024

Merge of #6213

d703330

mergify bot mentioned this pull request Sep 6, 2024

merge queue: embarking unstable (c0b4f01) and #6213 together #6368

Closed

5 tasks

mergify bot merged commit d686138 into sigp:unstable Sep 6, 2024
28 checks passed

michaelsproul deleted the remove-more-ethers branch September 6, 2024 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete legacy payload reconstruction #6213

Delete legacy payload reconstruction #6213

michaelsproul commented Aug 2, 2024

michaelsproul Sep 6, 2024

jimmygchen left a comment

jimmygchen commented Sep 6, 2024

jimmygchen commented Sep 6, 2024

mergify bot commented Sep 6, 2024 •

edited

Loading

michaelsproul commented Sep 6, 2024

		if capabilities.get_payload_bodies_by_range_v1
		\|\| capabilities.get_payload_bodies_by_range_v2

Delete legacy payload reconstruction #6213

Delete legacy payload reconstruction #6213

Conversation

michaelsproul commented Aug 2, 2024

Issue Addressed

Proposed Changes

Additional Info

michaelsproul Sep 6, 2024

Choose a reason for hiding this comment

jimmygchen left a comment

Choose a reason for hiding this comment

jimmygchen commented Sep 6, 2024

jimmygchen commented Sep 6, 2024

mergify bot commented Sep 6, 2024 • edited Loading

✅ The pull request has been merged automatically

michaelsproul commented Sep 6, 2024

mergify bot commented Sep 6, 2024 •

edited

Loading