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

Build against geth merge-v1.11.2 branch #1517

Merged
merged 24 commits into from
May 8, 2023
Merged

Build against geth merge-v1.11.2 branch #1517

merged 24 commits into from
May 8, 2023

Conversation

Tristan-Wilson
Copy link
Member

@Tristan-Wilson Tristan-Wilson commented Mar 7, 2023

This PR contains the changes needed to Nitro to use the OffchainLabs geth fork with upstream's v.1.11.2 merged into it.

The most notable change was that upstream implemented JWT auth, so we use that instead of our custom implementation.

Note: This PR currently changes the git submodule for go-ethereum to use the merge-v1.11.2 branch, which is being reviewed in OffchainLabs/go-ethereum#205. We'll have to change it back to using master after the other PR is merged.

Geth b4ea2bf7dd (PR #26003) renames Prepare to SetTxContext, and
PrepareAccessList becomes Prepare.

Geth 1f35988a00 (PR #26291) removes the time field from trace output.
@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Mar 7, 2023
@Tristan-Wilson Tristan-Wilson marked this pull request as ready for review April 6, 2023 20:16
@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Merging #1517 (c842385) into master (3fbe7ed) will decrease coverage by 3.40%.
The diff coverage is 65.71%.

❗ Current head c842385 differs from pull request most recent head 06177e6. Consider uploading reports for the commit 06177e6 to get more accurate results

@@            Coverage Diff             @@
##           master    #1517      +/-   ##
==========================================
- Coverage   53.05%   49.65%   -3.40%     
==========================================
  Files         281      268      -13     
  Lines       36029    31864    -4165     
  Branches      555      555              
==========================================
- Hits        19114    15823    -3291     
+ Misses      14744    13872     -872     
+ Partials     2171     2169       -2     

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.

only 1 small comment

_, err := EnsureTxSucceeded(ctx, l1client, tx)
if err != nil {
Fail(t, err)
_, err = EnsureTxSucceeded(ctx, l1client, tx)
Copy link
Contributor

Choose a reason for hiding this comment

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

EnsureTxSucceeded was done outside the loop on purpose. That way we only we for a transaction one time and not 30 times. Probably deserves a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved it inside the loop since it started failing with timeouts if it was outside the loop.

@@ -135,7 +135,7 @@ func PreCheckTx(bc *core.BlockChain, chainConfig *params.ChainConfig, header *ty
if tx.Nonce() < stateNonce {
return MakeNonceError(sender, tx.Nonce(), stateNonce)
}
intrinsic, err := core.IntrinsicGas(tx.Data(), tx.AccessList(), tx.To() == nil, chainConfig.IsHomestead(header.Number), true)
intrinsic, err := core.IntrinsicGas(tx.Data(), tx.AccessList(), tx.To() == nil, chainConfig.IsHomestead(header.Number), true, true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These last two parameters should be replaced with chainConfig.IsIstanbul(header.Number) (always true) and the new one with chainConfig.IsShanghai(header.Number) (always false for now, but will eventually become true on new blocks -- this is important because it's a breaking change).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Fixed it.

Now that v1.11.2 is merged in to master, we can set the submodule to
point back to master.
@Tristan-Wilson Tristan-Wilson requested a review from PlasmaPower May 8, 2023 19:52
Copy link
Collaborator

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

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

LGTM

@PlasmaPower PlasmaPower enabled auto-merge May 8, 2023 20:19
@PlasmaPower PlasmaPower merged commit a9124f3 into master May 8, 2023
@PlasmaPower PlasmaPower deleted the merge-v1.11.2 branch May 8, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants