Skip to content
This repository was archived by the owner on Jul 5, 2024. It is now read-only.

Update geth dependency from v1.10.18 to v1.11.5 #1363

Merged
merged 3 commits into from
Apr 18, 2023
Merged

Update geth dependency from v1.10.18 to v1.11.5 #1363

merged 3 commits into from
Apr 18, 2023

Conversation

axic
Copy link
Contributor

@axic axic commented Apr 16, 2023

Description

Updates the geth dependency in geth-utils to latest release.

Pulled this out from #1361.

Issue Link

Related to #1362.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Contents

  • Update geth dependency (and ran go mod tidy to update sub-dependencies)
  • Update code bindings to struct changes

Rationale

N/A

How Has This Been Tested?

Using the testing suite in the repo.

@github-actions github-actions bot added the crate-geth-utils Issues related to the geth-utils workspace member label Apr 16, 2023
axic
axic commented Apr 16, 2023
View reviewed changes
geth-utils/gethutil/trace.go

result, err := core.ApplyMessage(evm, message, new(core.GasPool).AddGas(message.Gas()))
result, err := core.ApplyMessage(evm, &message, new(core.GasPool).AddGas(message.GasLimit))
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 think these changes are correct based on the upstream diff: ethereum/go-ethereum@67ac5f0

axic
axic commented Apr 16, 2023
View reviewed changes
geth-utils/gethutil/trace.go
txAccessList,
false,
)
messages[i] = core.Message{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of this local translation, could consider using upstream Transaction type as well as the TransactionToMessage helper. It should help with handling EIP-1559 fields better.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good! Feel free to open an issue for that and start working on it. Probably this would also require changing parts in the external-tracer to encode different transaction types.

@github-actions github-actions bot added the CI Issues related to the Continuous Integration mechanisms of the repository. label Apr 16, 2023
@ed255
Copy link
Member

ed255 commented Apr 17, 2023

Can you also update this line?

zkevm-circuits/integration-tests/docker-compose.yml

Line 4 in c88d82d

image: "ethereum/client-go:v1.10.18"

This is for the integration tests

ed255
ed255 approved these changes Apr 17, 2023
View reviewed changes
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your contribution!

geth-utils/gethutil/trace.go
txAccessList,
false,
)
messages[i] = core.Message{
Copy link
Member

Choose a reason for hiding this comment

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

Sounds good! Feel free to open an issue for that and start working on it. Probably this would also require changing parts in the external-tracer to encode different transaction types.

ed255
ed255 suggested changes Apr 17, 2023
View reviewed changes
Copy link
Member

@ed255 ed255 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm moving my review to "request changes" while this is addressed #1363 (comment) but otherwise, everything looks good!

@github-actions github-actions bot added the crate-integration-tests Issues related to the integration-tests workspace member label Apr 17, 2023
@axic
Copy link
Contributor Author

axic commented Apr 17, 2023

Updated docker.

@axic axic requested a review from ed255 April 17, 2023 15:46
@ChihChengLiang ChihChengLiang mentioned this pull request Apr 18, 2023
Open
ed255
ed255 suggested changes Apr 18, 2023
View reviewed changes
@axic
Copy link
Contributor Author

axic commented Apr 18, 2023

Also rebased this on current main.

@axic axic requested a review from ed255 April 18, 2023 10:23
ed255
ed255 approved these changes Apr 18, 2023
View reviewed changes
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM!

han0110
han0110 approved these changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ed255 ed255 added this pull request to the merge queue Apr 18, 2023
Merged via the queue into privacy-scaling-explorations:main with commit 3429f83 Apr 18, 2023
@axic
Copy link
Contributor Author

axic commented Apr 18, 2023

Thank you for the speedy review!

@axic axic deleted the geth-utils branch April 18, 2023 18:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@han0110 han0110 han0110 approved these changes

@ed255 ed255 ed255 approved these changes

Assignees
No one assigned
Labels
CI Issues related to the Continuous Integration mechanisms of the repository. crate-geth-utils Issues related to the geth-utils workspace member crate-integration-tests Issues related to the integration-tests workspace member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@axic @ed255 @han0110