Fix EngineNewPayloadV4Test unit test

[lucassaldanha]

PR description

There were a few inconsistencies in the original test (like using the wrong version of the method). This PR fixes those inconsistencies and makes everything happy.

Fixed Issue(s)

N/A

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:

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

[lucassaldanha]

We don't need this early check as this is already handled by the DepositValidator on AbstractEngineNewPayload. However, if we do want to have this early check, we would need to modify some tests to support it.

IMHO, this comes down to: do we ever have a scenario where GetPayloadV4 is called pre-Prague?

[siladu]

Agree it appears to be a duplicate check. If we are having to validate params for other fields here already then think it makes sense to do it all in one place, rather than spread the check out.

Feels like the domain-level DepositsValidator shouldn't be doing this check if we already have this here

[lucassaldanha]

We have shared logic between the different version of engine_newPayload implemented on AbstractEngineNewPayload, because of that we still need the "conditional" validation on AbstractEngineNewPayload, determined by protocol schedule + DepositsValidator (choosing a validator implementation that makes sense for what fork we are in).

When it comes to EngineNewPayloadV4#validateParameters, we could enforce checking that the deposits field isn't empty, assuming that we only use V4 after Prague (need to confirm this).

Depending on what we think is the correct thing to do, we will need to adjust some of the testing because as of now, EnginePayloadV4 test extends EnginePayloadV3 and etc, and a lot of these tests will fail.

[lucassaldanha]

Ok this method should not be used pre-Prague. I'll adjust a few things.

[siladu]

@jflo might have the most recent context about the best way to migrate to V4.

[jflo]

yeah, this happened a ton on v3 as well, we really need to factor out all the inheritance and replace it with a composition approach.

[macfarla]

there's an AT failing

ExecutionEnginePragueAcceptanceTest > test(URI) > org.hyperledger.besu.tests.acceptance.jsonrpc.ExecutionEnginePragueAcceptanceTest.test(URI)[8] FAILED
    java.lang.AssertionError: {
      "jsonrpc" : "2.0",
      "id" : 67,
      "error" : {
        "code" : -[326](https://github.com/hyperledger/besu/actions/runs/8411584594/job/23031332619?pr=6798#step:9:327)02,
        "message" : "Invalid params",
        "data" : "Invalid deposits"
      }
    }
    did not equal
     {
      "jsonrpc" : "2.0",
      "id" : 67,
      "error" : {
        "code" : -32602,
        "message" : "Invalid params",
        "data" : "Missing deposit field"
      }
    }

[lucassaldanha]

there's an AT failing

In its current state, if I fix the unit test I break the AT and vice versa hehe :)