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

Merge 1.14.0 #345

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

Merge 1.14.0 #345

wants to merge 19 commits into from

Conversation

amsanghi
Copy link
Contributor

@amsanghi amsanghi commented Jul 18, 2024

fixes: NIT-2673

Merge upstream geth's v1.14.0 release into our geth fork:https://github.com/ethereum/go-ethereum/releases/tag/v1.14.0

nitro PR- OffchainLabs/nitro#2500

Testing done

geth and nitro tests pass

Copy link

cla-bot bot commented Jul 18, 2024

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2

@amsanghi amsanghi changed the base branch from master to merge-v1.13.15 July 18, 2024 11:57
@cla-bot cla-bot bot added the s label Jul 18, 2024
Base automatically changed from merge-v1.13.15 to master July 22, 2024 23:30
@amsanghi amsanghi marked this pull request as ready for review July 29, 2024 10:57
Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

comment on blockchain.go
question about transaction_arg.go

if err != nil {
panic(fmt.Sprintf("CallDefaults failed: %v", err))
}
return args.ToMessage(baseFee, globalGasCap, header, nil, runMode, chainID, gasLimitNotSetByUser) // we pass a nil to avoid another recursion
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 like to hear more about that.
Also: @Tristan-Wilson

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Earlier there was no concept of CallDefaults before calling ToMessage, but now there is.
So when we recurse in ToMessage for

// Arbitrum: raise the gas cap to ignore L1 costs so that it's compute-only
if state != nil {

We need to call CallDefaults to set the new globalGasCap (updated by InterceptRPCGasCap) again.
But because the CallDefaults was already called earlier the the args.Gas is already set to a non zero value (even though this value was not given by the user).
So to allow us to update args.Gas even to though it is set, i introduced a new input to function CallDefaults called gasLimitNotSetByUser which will help in determining whether we should be updating args.Gas even if it holds a non zero value (cause this non zero value was not set by the user).

core/blockchain.go Outdated Show resolved Hide resolved
@amsanghi amsanghi requested a review from tsahee August 22, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants