Return eth1_data early post transition

[pawanjay176]

Issue Addressed

N/A

Proposed Changes

Return state.eth1_data() early if we have passed the transition period post electra. Even if we don't return early, the function would still return state.eth1_data() based on the current conditions. However, doing this explicitly here to match the spec. This covers setting the right eth1_data in our block.

The other thing we need to ensure is that the deposits returned by the eth1_chain is empty post transition.

The only way we get non-empty deposits post the transition is if state.eth1_deposit_index in the below code is less than min(deposit_requests_start_index, state.eth1_data().deposit_count).

lighthouse/beacon_node/beacon_chain/src/eth1_chain.rs

Lines 543 to 579 in 0850bcf

	let deposit_count = if let Some(new_eth1_data) = get_new_eth1_data(state, eth1_data_vote)? {
	new_eth1_data.deposit_count
	} else {
	state.eth1_data().deposit_count
	};
	// [New in Electra:EIP6110]
	let deposit_index_limit =
	if let Ok(deposit_requests_start_index) = state.deposit_requests_start_index() {
	std::cmp::min(deposit_count, deposit_requests_start_index)
	} else {
	deposit_count
	};
	match deposit_index.cmp(&deposit_index_limit) {
	Ordering::Greater => Err(Error::DepositIndexTooHigh),
	Ordering::Equal => Ok(vec![]),
	Ordering::Less => {
	let next = deposit_index;
	let last = std::cmp::min(deposit_index_limit, next + E::MaxDeposits::to_u64());
	self.core
	.deposits()
	.read()
	.cache
	.get_deposits(next, last, deposit_count)
	.map_err(|e| Error::BackendError(format!("Failed to get deposits: {:?}", e)))
	.map(|(_deposit_root, deposits)| deposits)
	}
	}
	}
	fn latest_cached_block(&self) -> Option<Eth1Block> {
	self.core.latest_cached_block()
	}
	/// This only writes the eth1_data to a temporary cache so that the service

This can never happen because state.eth1_deposit_index will be equal to state.eth1_data.deposit count and cannot exceed the value.

@michaelsproul @ethDreamer please double check the logic for deposits being empty post transition. Following the logic in the spec makes my head hurt.

[Copilot]

Pull Request Overview

This PR explicitly returns state.eth1_data() early if state.eth1_deposit_index equals the deposit_requests_start_index, ensuring compliance with the Electra:EIP6110 specification.

• Updated DummyEth1ChainBackend to include the early return.
• Added similar logic in CachingEth1Backend to enforce correct behavior post transition.

[michaelsproul]

This looks ok, but I'm not sure if it's comprehensive enough to silence all deposit related warnings post Electra?

Things I've heard users complain about failing:

• Fetching deposits to build the cache (we shouldn't fetch them at all)
• Fetching eth1 data (this PR should fix this)
• Something about the deposit snapshot during finalization

I don't have any more time to look at this week, but will dive back in on Monday

[pawanjay176]

Fetching deposits to build the cache (we shouldn't fetch them at all)

I think this is kinda hard to do. In order to do this, we need to somehow tell the eth1 service to stop fetching logs after the electra_fork_epoch + the transition period in which we fill in all bridge deposits. I think this would be a decent amount of work for writing that code would literally go away the first release after electra so I'm not super keen on that.

Something about the deposit snapshot during finalization

This will be an issue for users who try to do a fresh checkpoint sync from a server that has passed the transition period and isn't able to finalize the snapshot anymore. I think the impact would be not too bad. We saw those issues on holesky because a lot of people were doing fresh syncs, but I didn't see it as much on sepolia. Correct me if i'm wrong here cc @chong-he
A potential improvement here would be to disable serving the deposit_snapshot http endpoint post electra.

I can't think of changes that are simple and high impact in terms of fewer unnessary error logs for users. I feel the best way forward would be to make a release with #7133 around the time when the ELs do their releases to drop pre-merge history post electra.

[chong-he]

We saw those issues on holesky because a lot of people were doing fresh syncs, but I didn't see it as much on sepolia. Correct me if i'm wrong here cc @chong-he A potential improvement here would be to disable serving the deposit_snapshot http endpoint post electra.

Yes we don't see the log from Sepolia. They are from Holesky testnet, probably because of fresh checkpoint sync as you mentioned

[michaelsproul]

Thanks, I'm satisfied that this matches the spec for get_eth1_vote (https://github.com/ethereum/consensus-specs/blob/dev/specs/electra/validator.md#deposits), and that we will return vec![] once all the old-style deposits are processed.

I made a commit on my fork where I renamed some variables to convince myself that we were matching the spec.

• Commit: fb8e3b9
• Spec: https://github.com/ethereum/consensus-specs/blob/dev/specs/electra/beacon-chain.md#modified-process_operations