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

Feat/blockdata unique gas limit constant #1699

Open
wants to merge 12 commits into
base: arith-dev
Choose a base branch
from

Conversation

amkCha
Copy link
Collaborator

@amkCha amkCha commented Jan 14, 2025

Regenerate trace files on tests launch to dynamically account for a different Min and Max gas limit between Ethereum and Linea

With Ethereum Gas limits
StateRefTests
BlockchainRefTests

With Linea Gas limits
FastReplayTests
Unit tests

Copy link

cla-assistant bot commented Jan 14, 2025

CLA assistant check
All committers have signed the CLA.

@amkCha amkCha force-pushed the feat/blockdata-unique-gas-limit-constant branch 3 times, most recently from 41fd16c to 0527728 Compare January 14, 2025 20:30

// row i + 1
// comparison to maximum
wcpCallToLEQ(1, data, EWord.of(Bytes.ofUnsignedLong(LINEA_GAS_LIMIT_MAXIMUM)));
wcpCallToLEQ(1, data, EWord.of(Bytes.ofUnsignedLong(GAS_LIMIT_MAXIMUM)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@DavePearce DavePearce force-pushed the feat/blockdata-unique-gas-limit-constant branch from 5e6f964 to b507971 Compare January 14, 2025 22:49
@DavePearce DavePearce self-requested a review January 14, 2025 23:14
@DavePearce DavePearce force-pushed the feat/blockdata-unique-gas-limit-constant branch from b507971 to 0a8da2e Compare January 14, 2025 23:20
DavePearce and others added 12 commits January 15, 2025 12:21
This ensures that, when running `gradle build`, it also arises the
`buildTracer` is run beforehand.  This is necessary to ensure that all
`Trace.java` files (and related) are generated.
This attempts to force the `Trace.java` files to be built before
anything else important happens.
This configures the build process to be conditional on a system-wide
property called "blockchain".  This property can be specified on the
command-line with e.g. "-Dblockchain=Ethereum" to set the target chain
to Ethereum.  By default, the blockchain is assumed to be Linea.
Therefore, its only when running the reference tests that this property
needs to be set.  Furthermore, if the reference tests are run without
setting this property ... then an error is reported and the build fails.
@DavePearce DavePearce force-pushed the feat/blockdata-unique-gas-limit-constant branch from 0a8da2e to 33f92a7 Compare January 14, 2025 23:22
Copy link
Collaborator

@DavePearce DavePearce left a comment

Choose a reason for hiding this comment

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

Basically, I think this looks good. Just unsure about the EqualsAndHashCode.Include for BlockdataOperation.

@@ -60,7 +57,7 @@ public class BlockdataOperation extends ModuleOperation {

private final boolean firstBlockInConflation;
private final int ctMax;
@EqualsAndHashCode.Include @Getter private final OpCode opCode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change looks odd, and potentially unexpected. Is there a specific reason we wanted to remove this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see that on my local branch

image

But it's strange that there should be an @EqualsAndHashCode.Include in the first place. We don't need to compare BlockdataOperation. And if we did we would have to include some extra identifier (e.g. the blockHeader)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, well if you believe that's safe ... then I'm happy to merge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's a copy paste to get the OpCode type that I got from another file. I forgot to remove the annotations, I don't see the need there

@amkCha amkCha force-pushed the feat/blockdata-unique-gas-limit-constant branch from 33f92a7 to 2517114 Compare January 15, 2025 08:29
@DavePearce DavePearce force-pushed the feat/blockdata-unique-gas-limit-constant branch from 9b857ea to 33f92a7 Compare January 15, 2025 08:43
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