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

7702 refactor, remove CodeDelegationAccount abstraction #8408

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

daniellehrner
Copy link
Contributor

PR description

Removes the CodeDelegationAbstraction to avoid calling Account.getCode() every time that we retrieve an account.

Fixed Issue(s)

fixes #8392

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • spotless: ./gradlew spotlessApply
  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@daniellehrner daniellehrner requested a review from Copilot March 12, 2025 10:12

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors code delegation handling by removing the CodeDelegationAccount abstraction and centralizing delegation logic into helper classes and services. Key changes include:

  • Removal of CodeDelegationAccount, MutableCodeDelegationDelegationAccount, and related files.
  • Introduction of CodeDelegationHelper and a simplified CodeDelegationService.
  • Updates to tests, protocol spec configurations, and transaction processing logic to use the new delegation approach.

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
evm/src/main/java/org/hyperledger/besu/evm/operation/InsufficientGasException.java New exception class for signaling insufficient gas during delegation resolution.
evm/src/main/java/org/hyperledger/besu/evm/worldstate/CodeDelegationHelper.java Introduces helper methods for code delegation resolution.
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/CodeDelegationProcessor.java Refactored to use WorldUpdater and a newly instantiated CodeDelegationService.
evm/src/main/java/org/hyperledger/besu/evm/operation/AbstractCallOperation.java Updates to delegate code resolution, integrating the new helper methods and exception handling.
Other files Removal of legacy code delegation account abstractions and adjustments in tests and transaction processing.
Comments suppressed due to low confidence (1)

evm/src/main/java/org/hyperledger/besu/evm/operation/AbstractCallOperation.java:264

  • Consider replacing RuntimeException with a more specific exception type to clearly indicate the missing target account condition and improve downstream error handling.
throw new RuntimeException("A delegated code account must have a target account");
@daniellehrner daniellehrner force-pushed the feat/issue-8392/remove_code_delegation_abstraction branch 2 times, most recently from f8b0f59 to b7304b2 Compare March 14, 2025 12:40
@daniellehrner daniellehrner marked this pull request as ready for review March 19, 2025 13:19
@daniellehrner daniellehrner force-pushed the feat/issue-8392/remove_code_delegation_abstraction branch from 2888729 to 1897206 Compare March 19, 2025 13:20
@daniellehrner daniellehrner requested review from Copilot and removed request for Copilot March 19, 2025 13:24
Comment on lines 266 to 268
if (maybeTargetAccount.isEmpty()) {
throw new RuntimeException("A delegated code account must have a target account");
}
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 struggling to reconcile this error case with with the EIP spec or EELS - can you point out the relevant part of the spec please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also think we are doing it twice: here and in CodeDelegationGasCostHelper.codeDelegationGasCost

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is not part of the spec, just defensive programming. So whenever we get an optional we should check if it's empty before calling get()

Copy link
Contributor

@siladu siladu Mar 22, 2025

Choose a reason for hiding this comment

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

Is this a condition where we could diverge from other client behaviour and it could lead to consensus issues? If there are error conditions missing should we update the spec?

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 don't think so . Theoretically between line 259 where we call hasCodeDelegation() and between 266 where we call maybeTargetAccount.isEmpty() a bit could flip in memory and we are unable to recover the target account. I've just added it because AFAIK it best practive to always call isPresent() / !isEMpty() before getting an optional, but I can remove it if you think it's confusing.

Copy link
Contributor

@siladu siladu Mar 25, 2025

Choose a reason for hiding this comment

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

I don't think we should code around hardware bugs. But I think the error conditions should match across client implementations, and ideally be part of the spec.
If this can truly never happen then maybe it shouldn't be Optional?

@@ -191,24 +196,10 @@ public OperationResult execute(final MessageFrame frame, final EVM evm) {

final Account contract = frame.getWorldUpdater().get(to);

if (contract != null && contract.hasDelegatedCode()) {
if (contract.getCodeDelegationTargetCode().isEmpty()) {
throw new RuntimeException("A delegated code account must have delegated code");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we no longer care about this error case? Seems like the refactored version results in different control flow logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is not needed here anymore because CodeDelegationHelper.getTargetAccount() takes care of that

return CodeV0.EMPTY_CODE;
}

if (!hasCodeDelegation(contract.getCode())) {
Copy link
Contributor

@siladu siladu Mar 20, 2025

Choose a reason for hiding this comment

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

I'm probably missing something, but isn't this still calling the problematic getCode() that accesses from disk?
Based on what @ahamlat said, I was expecting it should come from memory...or maybe is it already in memory within this Account instance?
Was the difference compared to before that we're no longer getting the account from the EvmWorldUpdater wrapper, which I guess must have been causing the extra disk access?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With EVMWorldUpdater we called getCode() every time that we retrieved an account with the get() method, no matter if we needed it or not. That created an overhead because in most cases we aren't interested in the code.

Here we do need to get the code because we need it to execute the tx, so that is not an overhead and was already like that before. The other places where we need it are the CALL op codes because they operate on the code of an account as well.

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.

Remove getCode() from CodeDelegationService.processAccount
2 participants