Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EIP-6110 Stop Eth1 data polling #8141

Merged
merged 14 commits into from
Mar 28, 2024

Conversation

StefanBratanov
Copy link
Contributor

@StefanBratanov StefanBratanov commented Mar 25, 2024

PR Description

If we have already transitioned to the new deposit mechanism:

  • Don't log missing deposit data
  • Don't do actual Eth1Data vote, just return the one from the state
  • Don't update Eth1Data metrics
  • Don't initialize and start PowchainService services
  • Implement stop for AsyncRunLoop because TimeBasedEth1HeadTracker was running in loop and stop was not implemented

Also attached FinalizedCheckpointChannel to PowchainService to stop Eth1 polling when transitioned on the fly. I am not 100 % sure of the cleanest way to pass a state to PowchainService because RecentChainData is initialized after starting of the services, so passing it before that will be in null. That's why chose to pass a Supplier (along with some comments) but happy for suggestions.

Update:
Refactored PowchainService to initialize when starting. This has couple of benefits. No initialization needed when we have already finalized the transition to the new deposit mechanism. This allows thing like using the websockets web3j client after Electra instead of relying on http, so no more of this error:

if (executionWeb3jClientProvider.getWeb3JClient().isWebsocketsClient()) {
   throw new InvalidConfigurationException("Eth1 endpoint fallback is not compatible with Websockets execution engine endpoint");
 }

It also allows to log the eth1 polling disabling only in case it has been enabled before.

Fixed Issue(s)

will solve one of the tasks under #7898

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@StefanBratanov StefanBratanov marked this pull request as draft March 25, 2024 22:07
@StefanBratanov StefanBratanov force-pushed the stop_eth1_data_polling branch from 3dd1432 to 8cfa7b2 Compare March 26, 2024 10:45
@StefanBratanov StefanBratanov marked this pull request as ready for review March 26, 2024 10:45
@StefanBratanov StefanBratanov force-pushed the stop_eth1_data_polling branch 2 times, most recently from c88d37d to 7692c27 Compare March 27, 2024 09:28
Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand it correctly that since EIP-6110 we don't need DepositTree and snapshots any more?
Though I think we need to approve it I still have some concerns on transition moment, that we are not breaking something too early when we are in Electra but former mechanism is not yet disabled. Anyway, we will run testnets for this.

@@ -39,11 +40,13 @@
import tech.pegasys.teku.service.serviceutils.ServiceConfig;
import tech.pegasys.teku.spec.Spec;
import tech.pegasys.teku.spec.TestSpecFactory;
import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState;
import tech.pegasys.teku.storage.api.Eth1DepositStorageChannel;

public class PowchainServiceTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd test something new here. At least that we start preElectra and don't start postElectra

Copy link
Contributor Author

@StefanBratanov StefanBratanov Mar 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point actually. Added a couple of relevant tests.

@@ -305,6 +305,21 @@ void shouldLogAnEventOnSlotWhenAllDepositsRequiredForStateNotAvailable() {
verify(eventLogger).eth1DepositDataNotAvailable(UInt64.valueOf(9), UInt64.valueOf(10));
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd test also that getDeposits() is empty since former mechanism is disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was part of a previous PR, can have a look at noDepositsIncludedIfFormerDepositMechanismHasBeenDisabled and getsRemainingEth1PendingDepositsIfElectraIsEnabled

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh the one I wanted comment for
All good, all done already

@StefanBratanov
Copy link
Contributor Author

StefanBratanov commented Mar 27, 2024

Do I understand it correctly that since EIP-6110 we don't need DepositTree and snapshots any more? Though I think we need to approve it I still have some concerns on transition moment, that we are not breaking something too early when we are in Electra but former mechanism is not yet disabled. Anyway, we will run testnets for this.

Yes, once we have finalized state where eth1_deposit_index == deposit_receipts_start_index we no longer need the DepositTree and snapshots since the getDeposits method would always be empty list from that point onwards, so no merkle proofs needed. So the idea here is to stop Eth1 polling once we finalize such a state and also don't even initialize if the anchor point on startup is such a state.

Also when syncing we don't need the deposit tree, so historical sync will still work. Essentially we only need the proofs when proposing in order to include it in the block.

@StefanBratanov StefanBratanov force-pushed the stop_eth1_data_polling branch from 3649c10 to c9609ae Compare March 27, 2024 20:33
Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just final nit

new PowchainService(
serviceConfig, powConfig, Optional.of(engineWeb3jClientProvider), latestFinalizedState);

powchainService.start().join();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add assertThat(powchainService.isRunning()).isTrue(); so it's clear that it's stopped only after finalized checkpoint. I know that it's clear from Service code, just the test will be more clear I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. It's more clear I agree.

@StefanBratanov StefanBratanov enabled auto-merge (squash) March 28, 2024 15:34
@StefanBratanov StefanBratanov merged commit ebd556a into Consensys:master Mar 28, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants