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

updating the goethereum dependency #14351

Open
wants to merge 69 commits into
base: develop
Choose a base branch
from
Open

Conversation

james-prysm
Copy link
Contributor

@james-prysm james-prysm commented Aug 15, 2024

What type of PR is this?

Other

What does this PR do? Why is it needed?

  • upgrades geth to 1.14.x ~
  • exchanges the usb third party dependency for hid
  • starts e2e from Capella instead of phase 0 ( we need to use some other way of testing phase 0 to capella)

related PRs that depend on geth changes for e2e

Which issues(s) does this PR fix?

Fixes #

Other notes for review

WORKSPACE Outdated Show resolved Hide resolved
@james-prysm james-prysm added Blocked Blocked by research or external factors Breaking Changes Breaking changes for a major release labels Sep 11, 2024
@@ -62,6 +63,9 @@ func (s *Service) beaconBlockSubscriber(ctx context.Context, msg proto.Message)
// This function reconstructs the blob sidecars from the EL using the block's KZG commitments,
// broadcasts the reconstructed blobs over P2P, and saves them into the blob storage.
func (s *Service) reconstructAndBroadcastBlobs(ctx context.Context, block interfaces.ReadOnlySignedBeaconBlock) {
if block.Version() < version.Deneb {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

temporary, it's addressed in terence's PR #14605

@@ -601,7 +601,7 @@ func (s *PremineGenesisConfig) setExecutionPayload(g state.BeaconState) error {
GasLimit: gb.GasLimit(),
GasUsed: gb.GasUsed(),
Timestamp: gb.Time(),
ExtraData: gb.Extra()[:32],
ExtraData: gb.Extra(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra data is not passed anymore due to removal of clique

@@ -169,6 +169,9 @@ func (c *componentHandler) setup() {
c.eth1Proxy = proxies
}

// timeout while we wait for the eth1 client to sync
time.Sleep(10 * time.Second)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wonder if I should add a constant for this

@@ -9,11 +9,11 @@ import (
)

func TestEndToEnd_MinimalConfig_WithBuilder(t *testing.T) {
r := e2eMinimal(t, types.InitForkCfg(version.Phase0, version.Deneb, params.E2ETestConfig()), types.WithCheckpointSync(), types.WithBuilder())
r := e2eMinimal(t, types.InitForkCfg(version.Capella, version.Deneb, params.E2ETestConfig()), types.WithCheckpointSync(), types.WithBuilder())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

easier to start at capella than at the merge

shanghaiTime := GethShanghaiTime(genesisTime, cfg)
if cfg.CapellaForkEpoch == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wondering if this is the best way to handle this instead of updating the function above. it's used elsewhere so perhaps it's not good as it changes the behavior

@@ -44,11 +46,17 @@ func TestStartDiscv5_DifferentForkDigests(t *testing.T) {
require.NoError(t, err)
defer bootListener.Close()

// Allow bootnode's table to have its initial refresh. This allows
// inbound nodes to be added in.
time.Sleep(5 * time.Second)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these 5 second timeouts on tests suck

@@ -143,11 +143,9 @@ func (m *Miner) initAttempt(ctx context.Context, attempt int) (*os.File, error)
"--ws.origins=\"*\"",
"--ipcdisable",
"--verbosity=4",
"--mine",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not supported anymore

Data: code,
})

// TODO: replace call with al, err := txfuzz.CreateAccessList(rpc, tx, sender) when txfuzz is fixed in new release
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tx-fuzz doesn't work correctly in 1.4.0 so I just manually do the ethcall below

@@ -798,7 +798,13 @@ func executableDataToBlock(params engine.ExecutableData, prevBeaconRoot []byte)
pRoot := common.Hash(prevBeaconRoot)
header.ParentBeaconRoot = &pRoot
}
block := gethTypes.NewBlockWithHeader(header).WithBody(txs, nil /* uncles */).WithWithdrawals(params.Withdrawals)
// TODO: update requests with requests for electra
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we support electra E2E we need to update this

@@ -265,7 +265,7 @@ func SyncCommitteePeriodStartEpoch(e primitives.Epoch) (primitives.Epoch, error)
// given slot start time
func SecondsSinceSlotStart(s primitives.Slot, genesisTime, timeStamp uint64) (uint64, error) {
if timeStamp < genesisTime+uint64(s)*params.BeaconConfig().SecondsPerSlot {
return 0, errors.New("could not compute seconds since slot start: invalid timestamp")
return 0, fmt.Errorf("could not compute seconds since slot %d start: invalid timestamp, got %d < want %d", s, timeStamp, genesisTime+uint64(s)*params.BeaconConfig().SecondsPerSlot)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hope it's ok to update the error in this way.

@@ -64,7 +66,7 @@ func builderActive(_ *e2etypes.EvaluationContext, conns ...*grpc.ClientConn) err
if err != nil {
return err
}
if forkStartSlot == b.Block().Slot() || forkStartSlot+1 == b.Block().Slot() {
if forkStartSlot == b.Block().Slot() || forkStartSlot+1 == b.Block().Slot() || lowestBound <= 1 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add this due to this error

assertions.go:145: endtoend_test.go:607 Evaluation failed for epoch 1: capella block with slot 6 was not built by the builder. It has an extra data of  and txRoot of 0xd13b70ff02dac0503e4103e33b9b4030d4d2409d71579efb4793434fddbe58d2: capella block with slot 6 was not built by the builder. It has an extra data of  and txRoot of 0xd13b70ff02dac0503e4103e33b9b4030d4d2409d71579efb4793434fddbe58d2

@@ -265,7 +265,7 @@ func SyncCommitteePeriodStartEpoch(e primitives.Epoch) (primitives.Epoch, error)
// given slot start time
func SecondsSinceSlotStart(s primitives.Slot, genesisTime, timeStamp uint64) (uint64, error) {
if timeStamp < genesisTime+uint64(s)*params.BeaconConfig().SecondsPerSlot {
return 0, errors.New("could not compute seconds since slot start: invalid timestamp")
return 0, fmt.Errorf("could not compute seconds since slot %d start: invalid timestamp, got %d < want %d", s, timeStamp, genesisTime+uint64(s)*params.BeaconConfig().SecondsPerSlot)
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 error still happens Occasionally on e2e startup

 helpers.go:242: time="2024-11-05 21:18:14" level=error msg="could not check if block arrived early" error="could not compute seconds since slot 0 start: invalid timestamp, got 1730841480 < want 1730841484" prefix=forkchoice-doublylinkedtree
    helpers.go:242: time="2024-11-05 21:18:14" level=error msg="Could not get builder payload" error="builder returned header with 0 bid amount" prefix="rpc/validator"
    ```

@james-prysm james-prysm marked this pull request as ready for review November 5, 2024 22:55
@james-prysm james-prysm requested a review from a team as a code owner November 5, 2024 22:55
@james-prysm james-prysm added the Ready For Review A pull request ready for code review label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Blocked by research or external factors Breaking Changes Breaking changes for a major release Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants