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

Remove TTD Bellatrix merge configuration option #8951

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

zilm13
Copy link
Contributor

@zilm13 zilm13 commented Dec 20, 2024

PR Description

WIP

Fixed Issue(s)

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.

@zilm13 zilm13 mentioned this pull request Dec 20, 2024
3 tasks
@zilm13
Copy link
Contributor Author

zilm13 commented Jan 8, 2025

It's built on top of #8943 which should be merged before

Some notes:

  • ValidatorIndexProviderTest.java order is not guaranted at least on my java version and architecture, should we respect some order? From my opinion no, so I've changed it to compare ignoring order
  • TekuConfigurationTest.java temporarily disabled upon investigation. 1. Test forces teku:test and output misses cause. 2. It's not clear how to fix it.
  • WebsocketsMergeTransitionAcceptanceTest.java - test purpose recovered
  • OptimisticSyncSafeSlotsAcceptanceTest.java and BellatrixMergeTransitionAcceptanceTest.java - not possible to test anything without TTD, test removed
  • other acceptance tests modified to merge with execution side genesis

@zilm13 zilm13 marked this pull request as ready for review January 8, 2025 21:53
@zilm13 zilm13 marked this pull request as draft January 8, 2025 22:00
}
STATUS_LOG.fatalError(errorDescription, rootCause);
System.exit(FATAL_EXIT_CODE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we always want to do System.exit()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for information: here is the original PR where exit() appeared: #2626

@zilm13 zilm13 marked this pull request as ready for review January 9, 2025 20:48
Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

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

LGTM with some minor questions

createGenesisGenerator()
.network(NETWORK_NAME)
.withAltairEpoch(UInt64.ZERO)
.withBellatrixEpoch(UInt64.ONE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why Bellatrix at epoch 1? So essentially the EL remains waiting at genesis block (which is known and is DEFAULT_EL_GENESIS_HASH until CL reaches epoch 1 and then it merges using that genesis block?

@@ -13,6 +13,7 @@
"londonBlock": 0,
"parisBlock": 0,
"terminalTotalDifficulty": 0,
"terminalTotalDifficultyPassed": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed to deprecate TTD I guess

terminalBlock =
new PowBlock(
terminalBlockHash, TERMINAL_BLOCK_PARENT_HASH, terminalTotalDifficulty, transitionTime);
terminalBlockParent = new PowBlock(TERMINAL_BLOCK_PARENT_HASH, Bytes32.ZERO, UInt64.ZERO);
Copy link
Contributor

Choose a reason for hiding this comment

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

we can do some additional cleanup here in the stub, see terminalBlockHashInTTDMode

final boolean isMergeTransitionComplete =
isMergeTransitionComplete(recentChainData.getChainHead());
if (!isMergeTransitionComplete
&& spec.isMilestoneSupported(SpecMilestone.BELLATRIX)
Copy link
Contributor

Choose a reason for hiding this comment

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

i think spec.isMilestoneSupported(SpecMilestone.BELLATRIX) is redundant since we already check bellatrix spec config at line 75

Comment on lines +64 to +65
final IntCollection actual = provider.getValidatorIndices().getImmediately();
assertThat(actual).containsExactlyInAnyOrderElementsOf(IntArrayList.of(1, 20, 300));
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change? (the same below)

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.

3 participants