Fix: CodeDelegation account clearing side-effect issue

[macfarla]

PR description

The problem is in CodeDelegationProcessor.java:122-123:

final Optional maybeAuthorityAccount =
Optional.ofNullable(worldUpdater.getAccount(authorizer.get()));

getAccount() returns a mutable account and calls track() in AbstractWorldUpdater, which adds the account to the updatedAccounts map — effectively marking it as touched.

Later, in MainnetTransactionProcessor.java:494-496, clearAccountsThatAreEmpty() iterates all touched accounts and deletes any that are empty. So a pre-existing empty authority account that was only read for validation gets deleted, changing the
post-state trie root.

The Fix

The WorldUpdater already has two account access methods:

• get(address) — read-only, does NOT mark as touched
• getAccount(address) — mutable, marks as touched via track()

The fix is to use get() for the initial validation read, and only call getAccount() when we've passed validation and actually need to mutate the account.

Changes Made

CodeDelegationProcessor.java (the fix):

• Changed the initial account lookup from worldUpdater.getAccount() (mutable, marks as touched) to worldUpdater.get() (read-only, no side effects)
• Moved the nonce validation for existing accounts before calling getAccount(), so invalid authorizations return early without ever touching the account
• worldUpdater.getAccount() is now only called after all validation passes and we're about to mutate the account

CodeDelegationProcessorTest.java (test updates):

• Updated mocks to use worldUpdater.get() for the read-only validation path
• Added worldUpdater.getAccount() mock only in tests where mutation actually occurs (valid delegation for existing account)
• Added assertions that getAccount() is never called on rejection paths (invalid nonce, canSetCodeDelegation fails)

Fixed Issue(s)

fixes #9840

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
• hive tests: Engine or other RPCs modified?

[Copilot]

Pull request overview

Fixes a side-effect where reading an authority account during code delegation validation could “touch” it and cause empty pre-existing accounts to be cleared later, changing the post-state.

Changes:

• Switches validation-only account reads from worldUpdater.getAccount() to side-effect-free worldUpdater.get().
• Reorders validation to only call getAccount() after all checks pass and mutation is required.
• Updates tests to mock/verify get() vs getAccount() usage on accept/reject paths.

Reviewed changes

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

File	Description
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/CodeDelegationProcessor.java	Avoids marking accounts as touched during validation by using get() and deferring getAccount() until mutation.
ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/CodeDelegationProcessorTest.java	Adjusts mocks and adds assertions to ensure getAccount() isn’t invoked on rejection paths.

Comments suppressed due to low confidence (2)

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/CodeDelegationProcessor.java:151

• maybeExistingAccount.get() is dereferenced multiple times and getNonce() is called twice, which makes the logic noisier than necessary. Consider assigning maybeExistingAccount.get() to a local (e.g., existingAccount) and reusing a single existingNonce for the comparison/logging.

      if (!codeDelegationService.canSetCodeDelegation(maybeExistingAccount.get())) {
        return;
      }

      if (codeDelegation.nonce() != maybeExistingAccount.get().getNonce()) {
        LOG.trace(
            "Invalid nonce for code delegation. Expected: {}, Actual: {}",
            maybeExistingAccount.get().getNonce(),
            codeDelegation.nonce());
        return;
      }

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/CodeDelegationProcessor.java:125

• This comment references a specific downstream cleanup method (clearAccountsThatAreEmpty()), which is an implementation detail outside this class. Consider rephrasing to document the local contract/intent (e.g., “use get() for validation-only reads because getAccount() tracks updates/touches accounts”) without tying the behavior to a particular external method name.

    // Use read-only get() to avoid marking the account as touched during validation.
    // getAccount() would mark it as touched, causing empty accounts to be incorrectly
    // deleted by clearAccountsThatAreEmpty() even when authorization is invalid/skipped.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[macfarla]

running the reproduction instructions from the issue #9840

The output from the test run is:

  Considering ConvertedTest001
  Running iteration 0
  Running ConvertedTest001
  Block 1 (0xfe3edfece56b178d7efdd25cff8de94e7c8f2859a761d75060b7375bb610bbd8) Imported in 48.81 ms (5.98 MGas/s)
  Chain import successful

  ================================================================================
  TEST SUMMARY
  ================================================================================
  Total tests:  1
  Passed:       1
  Failed:       0
  ================================================================================

[macfarla]

running some test mainnet nodes with this branch

[daniellehrner]

Looks good, this should solve the issue