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

EIP-7702 and EIP-7251 #7459

Open
wants to merge 137 commits into
base: pectra_payload_bodies
Choose a base branch
from
Open

Conversation

ak88
Copy link
Contributor

@ak88 ak88 commented Sep 18, 2024

https://eips.ethereum.org/EIPS/eip-7702
https://eips.ethereum.org/EIPS/eip-7251 (consolidations)

Changes

Adds an authorization_list to tx that is used to set delegations on EOAs in the form of 0xef0100 || address

When the EOA is called it is delegated to the address, in similar fashion as a DELEGATECALL.
EVM opcodes like EXTCODESIZE, EXTCODECOPY etc. are affected by a delegation.

Other things to note:

  • 7702 delegations and refunds will happen regardless of EVM execution outcome.
  • Account access gas is changed to count double if delegated. Once for the EOA and once for the delegation address.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Optional. Remove if not applicable.

Documentation

Requires documentation update

  • Yes
  • No

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
  • No

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

Remarks

Optional. Remove if not applicable.

@MarekM25 MarekM25 changed the title Pectra fix 7702 after merge EIP-7702 and EIP-7251 Sep 19, 2024

requestsList.AddRange(_depositsProcessor.ProcessDeposits(block, receipts, spec));
requestsList.AddRange(_withdrawalRequestsProcessor.ReadWithdrawalRequests(block, state, spec));
using ArrayPoolList<ConsensusRequest> requestsList = new(receipts.Length * 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to initialize capacity with block.Requests.Length

Receipts are only relevant for Deposits

cc: @LukaszRozmej

private AuthorizationTuple[] Tuples100;
private AuthorizationTuple[] Tuples1k;

private CodeInfoRepository sut;
Copy link
Contributor

Choose a reason for hiding this comment

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

cosmetic, but weird variable name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/Nethermind/Nethermind.Evm.Benchmark/EvmBenchmarks.cs Outdated Show resolved Hide resolved
@@ -137,12 +137,20 @@ protected EthereumTestResult RunTest(GeneralStateTest test, ITxTracer txTracer)

Block block = Build.A.Block.WithTransactions(test.Transaction).WithHeader(header).TestObject;

bool isValid = _txValidator.IsWellFormed(test.Transaction, spec) && IsValidBlock(block, specProvider);
bool blockIsValid = IsValidBlock(block, specProvider);
ValidationResult txIsValid = _txValidator.IsWellFormed(test.Transaction, spec);
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling BlockValidator should be enough and could replace those two lines:
bool blockIsValid = IsValidBlock(block, specProvider);
ValidationResult txIsValid = _txValidator.IsWellFormed(test.Transaction, spec);

if (!tx.HasAuthorizationList)
return AcceptTxResult.Accepted;

Metrics.PendingTransactionsWithExpensiveFiltering++;
Copy link
Contributor

Choose a reason for hiding this comment

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

This metric is incorrect here, as it counts transactions that reach the expensive filtering in TxPool. For SetCode transactions, we end up counting one transaction twice.

I think having a separate metric would be a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already Metrics.Eip7702TransactionsInBlock so maybe this one should just be removed?

@@ -282,3 +310,42 @@ public sealed class SignatureTxValidator : BaseSignatureTxValidator
public static readonly SignatureTxValidator Instance = new();
private SignatureTxValidator() { }
}

public sealed class SetCodeTxValidator : ITxValidator
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that this validation is being omitted in the BlockProductionTransactionPicker.

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 dont see the TxValidator checks being used there, maybe because the logic is that picked tx comes from TxPool?

@@ -78,6 +79,18 @@ public void RecoverData(Block block)
{
tx.SenderAddress = sender;

if (tx.HasAuthorizationList)
Copy link
Contributor Author

@ak88 ak88 Sep 20, 2024

Choose a reason for hiding this comment

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

@MarekM25 please take another look at RecoverSignatures. I have fixed an oversight here.

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.

4 participants