use error code 3 for reverts

[macfarla]

Signed-off-by: Sally MacFarlane [email protected]

PR description

error code 3 for reverts
Refer this commit to execution api repo ethereum/execution-apis@c138ce0

Fixed Issue(s)

Fixes these hive tests
eth_call/call-revert-abi-error
eth_call/call-revert-abi-panic

Note the eth_simulate tests with reverts are yet to be updated for the revert error code change.

eth_simulateV1/ethSimulate-eth-send-should-not-produce-logs-on-revert
eth_simulateV1/ethSimulate-eth-send-should-produce-no-logs-on-forward-revert

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?

[siladu]

This feels like quite a big breaking change to just release without warning, especially since it'll end up in the fusaka release. Does this warrant 3 months notice?

[garyschulte]

Ambivalent here. If geth and erigon, both of which are used for block explorers and rpc instances, have used this since 2022... how much would it really break?

Maybe private chains or alt-L1s?

[siladu]

If geth and erigon, both of which are used for block explorers and rpc instances, have used this since 2022

That lessens the concern somewhat.

how much would it really break?

Are you suggesting that noone is using eth_call/eth_estimateGas with besu on mainnet? I don't know the answer to that, I just thought it likely someone is.

Maybe private chains

For sure.

[macfarla]

yeah this was kind of my thinking. leaving it until after the fusaka release is nbd. but yeah this is bringing besu in line with other ELs so it's probably only really a breaking change if you're only using besu - so private chains.

maybe there is a bigger discussion to have around how we communicate changes to our RPCs

[siladu]

I believe historically the default for breaking RPC is 3 months notice. Previously think it was tied to quarterly releases, now think we agreed "upcoming breaking changes" is sufficient.
I'm not sure whether breaking RPCs for private networks should be treated differently to public networks.

I don't feel too strongly if you are convinced the impact is low, just wanted to flag it.

[macfarla]

since this change brings besu in line with other ELs, think it's ok to flag in the changelog in the same release. No advance warning needed.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.