Skip to content

Change contract creation halt reason when account already exists#6518

Merged
macfarla merged 7 commits intobesu-eth:mainfrom
gfukushima:contract-creation-halt-reason
Feb 6, 2024
Merged

Change contract creation halt reason when account already exists#6518
macfarla merged 7 commits intobesu-eth:mainfrom
gfukushima:contract-creation-halt-reason

Conversation

@gfukushima
Copy link
Copy Markdown
Contributor

@gfukushima gfukushima commented Feb 2, 2024

PR description

Change the execution halt reason from INSUFFICIENT_GAS to ILLEGAL_STATE_CHANGE to reflect the real reason behind this case which is a possible contract creation collision with a already existing a account at that address.

Note that some of the trace ATs had to be adjusted after this change, possibly because the frame where we fail execution was getting included into the trace result as we don't ignore frames that halted due to INSUFFICIENT_GAS https://github.com/hyperledger/besu/blob/2a30dfba1a03ad1c2b9d42857615984c439f4970/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/tracing/vm/VmTraceGenerator.java#L110

…ccount already exists

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 2, 2024

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests
  • I thought about running CI.
  • If I did not run CI, I ran as much locally as possible before pushing.

@gfukushima gfukushima marked this pull request as ready for review February 2, 2024 05:32
Copy link
Copy Markdown
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

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

We need to figure out why the VM traces need to be altered to get it to pass tests and fix it. The error text changing int he call trace is fine, but losing the CALL instruction in the VM trace breaks a lot of downstream users.

},
"pc" : 31,
"sub" : null
}, {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should not be happening. We need to fing the code that is dropping this trace line when the error kind changes and fix it.

Copy link
Copy Markdown
Contributor Author

@gfukushima gfukushima Feb 5, 2024

Choose a reason for hiding this comment

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

Hi @shemnon, thanks for the feedback. As I mentioned in the PR description, I knew the line was not included in the trace due to the fact that any frame that halts with a reason different from INVALID_JUMP_DESTINATION or INSUFFICIENT_GAS is ignored. I wasn't sure if it adding ILLEGAL_STATE_CHANGE was the right thing to do there. I've made the change that includes the trace line back.

@shemnon
Copy link
Copy Markdown
Contributor

shemnon commented Feb 2, 2024

Probably in org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.tracing.vm.VmTraceGenerator

…ignored when producing the vm trace

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
Copy link
Copy Markdown
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

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

LGTM

@macfarla macfarla merged commit dee608f into besu-eth:main Feb 6, 2024
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.

3 participants