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

Add Eof Execution EIP-7692 (EIP-663, EIP-3540, EIP-3670, EIP-4200, EIP-4750, EIP-5450, EIP-6206, EIP-7069, EIP-7480, EIP-7620, EIP-7698, EIP-7756) #8176

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

Conversation

benaadams
Copy link
Member

Closes #4584
Closes #4586
Closes #4906
Closes #4907
Closes #4956

Changes

  • Refactor Evm to make it less unwieldy to opcode changes & optimizations

Types of changes

What types of changes does your code introduce?

  • New feature (a non-breaking change that adds functionality)

Testing

Requires testing

  • Yes

If yes, did you write tests?

  • Yes

Notes on testing

Optional. Remove if not applicable.

Documentation

Requires documentation update

  • Yes

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

  • Yes

If yes, fill in the details here. Remove if not applicable.

Remarks

Optional. Remove if not applicable.

@@ -13,6 +13,7 @@
<PackageVersion Include="BouncyCastle.Cryptography" Version="2.5.0" />
<PackageVersion Include="Ckzg.Bindings" Version="2.0.1.1258" />
<PackageVersion Include="Colorful.Console" Version="1.2.15" />
<PackageVersion Include="CommandLineParser" Version="2.9.1" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This is redundant. We already use the same CLI parser for everything else.

Copy link
Contributor

@Scooletz Scooletz left a comment

Choose a reason for hiding this comment

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

A few remarks. I love both, the direction and the execution. Long live the delegate* calls!

@@ -431,5 +437,9 @@ public interface IReleaseSpec : IEip1559Spec, IReceiptSpec
bool IsAuthorizationListEnabled => IsEip7702Enabled;

public bool RequestsEnabled => ConsolidationRequestsEnabled || WithdrawalRequestsEnabled || DepositsEnabled;

public object? EvmInstructions { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have some comments here? What kind of object is it etc?


public object? EvmInstructions { get; set; }

public object? EvmTracedInstructions { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above? Maybe refer to Trace, NoTrace?

deployingAddress.Bytes.CopyTo(bytes.Slice(1, 20));
salt.CopyTo(bytes.Slice(21, salt.Length));
ValueKeccak.Compute(initCode).BytesAsSpan.CopyTo(bytes.Slice(21 + salt.Length, 32));
deployingAddress.Bytes.CopyTo(bytes.Slice(1, Address.Size));
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for removing magic 🙇

}
private static void WaitForAnalysisToComplete(ManualResetEventSlim resetEvent)
{
Thread thread = Thread.CurrentThread;
Copy link
Contributor

Choose a reason for hiding this comment

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

There was the scoping mechanism elsewhere. Can't we do using var priority = ... here?


for (int i = 0; i < lookup.Length; i++)
{
lookup[i] = &InstructionBadInstruction;
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!

lookup[(int)Instruction.BYTE] = &InstructionByte;

// Conditional: enable shift opcodes if the spec allows.
if (spec.ShiftOpcodesEnabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you saying you extracted all spec checks? ❤️

Copy link
Member Author

@benaadams benaadams Feb 11, 2025

Choose a reason for hiding this comment

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

Yep all gone; are in Create instructions (once ish) now rather than in the instructions themselves (many times)

@LukaszRozmej's suggestion

_codeInfoRepository.InsertCode(WorldState, code, env.ExecutingAccount, spec);

unspentGas -= codeDepositGasCost;
}
}

if (tx.IsEofContractCreation)
Copy link
Contributor

Choose a reason for hiding this comment

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

Extract method? Gets really big.


namespace Nethermind.Evm;

public interface IFlag
Copy link
Contributor

Choose a reason for hiding this comment

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

As you generalized on/off using generic parameter trick, should it be commented for future generations how and why to use it?

debugger?.TryWait(ref _vmState, ref programCounter, ref gasAvailable, ref stack.Head);
#endif
// Fetch the current instruction from the code section.
Instruction instruction = codeSection[programCounter];
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this can be trusted. You removed so much of the beautiful code that was here and all we're left is an array lookup... On a serious note: beautiful 😍

@Scooletz
Copy link
Contributor

A lot of changes seem to be reported as the master is merged back?

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

WIth VM changed this way we could massively simplify our tracing API basing it on how it looks in Geth.

Think about how can we use plugins to extend instruction set!

Comment on lines +32 to +36
TestType testType = testsDir.Contains("state_tests", StringComparison.InvariantCultureIgnoreCase)
? TestType.GeneralState
: testsDir.Contains("eof_tests", StringComparison.InvariantCultureIgnoreCase)
? TestType.Eof
: TestType.Blockchain;
Copy link
Member

Choose a reason for hiding this comment

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

Change TestType to { State, EoF, Blockchain } and base the test type detenction on a loop with condition testsDir.Contains($"{testType}_tests", StringComparison.InvariantCultureIgnoreCase)

Comment on lines +68 to +73
IEnumerable<IEthereumTest> tests = testType switch
{
TestType.Eof => fileTestsSource.LoadEofTests(),
TestType.GeneralState => fileTestsSource.LoadGeneralStateTests(),
_ => fileTestsSource.LoadBlockchainTests()
};
Copy link
Member

Choose a reason for hiding this comment

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

Can we do it in OO way?

Comment on lines +83 to +89
IEthereumTest failedTest = testType switch
{
TestType.Eof => new EofTest(),
TestType.GeneralState => new GeneralStateTest(),
_ => new BlockchainTest()
};

Copy link
Member

Choose a reason for hiding this comment

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

Same here

Comment on lines +25 to +27
return loader.LoadTests().Cast<EofTest>().Select(t => new TestCaseData(t)
.SetName(t.Name)
.SetCategory(t.Category));
Copy link
Member

Choose a reason for hiding this comment

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

this part is very similar in all types with only the difference of the generic type, maybe we can abstract that to base types and or generics on type?

[TestFixture]
[Parallelizable(ParallelScope.All)]
[Explicit("These tests are not ready yet")]
public class PragueStateTests : GeneralStateTestBase
Copy link
Member

Choose a reason for hiding this comment

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

No more Prague state tests?

Comment on lines +71 to +86
public const long DataLoad = 4;
public const long DataLoadN = 3;
public const long DataCopy = 3;
public const long DataSize = 2;
public const long ReturnContract = 0;
public const long EofCreate = 32000;
public const long ReturnDataLoad = 3;
public const long RJump = 2;
public const long RJumpi = 4;
public const long RJumpv = 4;
public const long Exchange = 3;
public const long Swapn = 3;
public const long Dupn = 3;
public const long Callf = 5;
public const long Jumpf = 5;
public const long Retf = 3;
Copy link
Member

Choose a reason for hiding this comment

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

add comment that those are eofv1


public static bool IsValid(this Instruction instruction, bool IsEofContext)
{
if (!Enum.IsDefined(instruction))
Copy link
Member

Choose a reason for hiding this comment

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

is Enum now as fast as FastEnum?

/// <summary>
/// The gas cost for executing the bitwise operation.
/// </summary>
virtual static long GasCost => GasCostOf.VeryLow;
Copy link
Member

Choose a reason for hiding this comment

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

static first

/// <param name="a">The first operand vector.</param>
/// <param name="b">The second operand vector.</param>
/// <returns>The result of the bitwise operation.</returns>
abstract static Word Operation(Word a, Word b);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better?

Suggested change
abstract static Word Operation(Word a, Word b);
abstract static Word Operation(in Word a, in Word b);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants