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

[EVM] Direct call hash calculation #5369

Merged
merged 14 commits into from
Feb 20, 2024
Merged

[EVM] Direct call hash calculation #5369

merged 14 commits into from
Feb 20, 2024

Conversation

sideninja
Copy link
Contributor

@sideninja sideninja commented Feb 9, 2024

Closes: #5370

This PR changes how direct call calculation is done. It now relies on the implementation of hash calculation from the geth Transaction.
This was changed because the direct call hash was included in the block as transaction hashes, which meant that any client would expect to be able to fetch that transaction and then recalculate the same hash, but since the direct call had a custom hash calculation the transaction calculated hash would differ.

@sideninja
Copy link
Contributor Author

@ramtinms one question I have about the implementation that I'm not sure how to address. We now have hash calculated out of a certain subset of direct call data, which means there might be multiple direct calls that produce the same hash actually since direct calls are not using nonces that might have been true even before. What if you make two direct call deposits with the same amount, and address etc, I believe they would produce the same hash. I don't believe this change introduces this problem, but it makes it maybe a bit worse even, since now we don't have the subtype included which means multiple subtype calls could even have the same hash.

@codecov-commenter
Copy link

codecov-commenter commented Feb 9, 2024

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (8b2113a) 55.96% compared to head (48df683) 60.66%.

Files Patch % Lines
fvm/evm/types/call.go 63.33% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5369      +/-   ##
==========================================
+ Coverage   55.96%   60.66%   +4.69%     
==========================================
  Files        1024      678     -346     
  Lines       99184    66805   -32379     
==========================================
- Hits        55512    40526   -14986     
+ Misses      39423    23132   -16291     
+ Partials     4249     3147    -1102     
Flag Coverage Δ
unittests 60.66% <79.24%> (+4.69%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ramtinms ramtinms enabled auto-merge February 20, 2024 19:33
@ramtinms ramtinms added this pull request to the merge queue Feb 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 20, 2024
@ramtinms ramtinms enabled auto-merge February 20, 2024 22:43
@ramtinms ramtinms added this pull request to the merge queue Feb 20, 2024
Merged via the queue into master with commit 53dc73e Feb 20, 2024
50 of 51 checks passed
@ramtinms ramtinms deleted the gregor/evm/direct-call branch February 20, 2024 23:41
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.

[EVM] Change direct call hash calculation
4 participants