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

fix: Inconsistent handling of failed EVM transactions #3158

Merged

Conversation

victor-yanev
Copy link
Contributor

Description:

This PR fixes the issue discussed in the comments of #3115

Related issue(s):

Fixes #3115

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@victor-yanev victor-yanev added bug Something isn't working P1 labels Oct 25, 2024
@victor-yanev victor-yanev added this to the 0.59.0 milestone Oct 25, 2024
@victor-yanev victor-yanev self-assigned this Oct 25, 2024
Copy link

github-actions bot commented Oct 25, 2024

Test Results

 20 files  ± 0  273 suites  +1   37m 12s ⏱️ + 3m 39s
605 tests + 4  594 ✅ + 3  4 💤 ±0  7 ❌ +1 
684 runs   - 57  673 ✅  - 56  4 💤  - 2  7 ❌ +1 

For more details on these failures, see this check.

Results for commit ad8d69d. ± Comparison against base commit 6442c98.

This pull request removes 2 and adds 6 tests. Note that renamed tests count towards both.
"before all" hook for "should associate to a token" ‑ RPC Server Acceptance Tests Acceptance tests @tokencreate HTS Precompile Token Create Acceptance Tests "before all" hook for "should associate to a token"
"before all" hook in "Debug API Test Suite" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests Debug API Test Suite "before all" hook in "Debug API Test Suite"
"before all" hook in "@api-batch-2 RPC Server Acceptance Tests" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before all" hook in "@api-batch-2 RPC Server Acceptance Tests"
Should eventually exhaust the hbar limit for EXTENDED user ‑ RPC Server Acceptance Tests Acceptance tests @hbarlimiter HBAR Limiter Acceptance Tests HBAR Rate Limit Tests HBAR Rate Limit For Different Spending Plan Tiers Preconfigured Tiers EXTENDED Tier Should eventually exhaust the hbar limit for EXTENDED user
Should eventually exhaust the hbar limit for PRIVILEGED user ‑ RPC Server Acceptance Tests Acceptance tests @hbarlimiter HBAR Limiter Acceptance Tests HBAR Rate Limit Tests HBAR Rate Limit For Different Spending Plan Tiers Preconfigured Tiers PRIVILEGED Tier Should eventually exhaust the hbar limit for PRIVILEGED user
Should increase the amount spent of the spending plan by the transaction cost ‑ RPC Server Acceptance Tests Acceptance tests @hbarlimiter HBAR Limiter Acceptance Tests HBAR Rate Limit Tests HBAR Rate Limit For Different Spending Plan Tiers Preconfigured Tiers EXTENDED Tier Should increase the amount spent of the spending plan by the transaction cost
Should increase the amount spent of the spending plan by the transaction cost ‑ RPC Server Acceptance Tests Acceptance tests @hbarlimiter HBAR Limiter Acceptance Tests HBAR Rate Limit Tests HBAR Rate Limit For Different Spending Plan Tiers Preconfigured Tiers PRIVILEGED Tier Should increase the amount spent of the spending plan by the transaction cost
Should successfully populate all pre-configured spending plans ‑ RPC Server Acceptance Tests Acceptance tests @hbarlimiter HBAR Limiter Acceptance Tests HBAR Rate Limit Tests HBAR Rate Limit For Different Spending Plan Tiers Preconfigured Tiers given a valid JSON file with pre-configured spending plans Should successfully populate all pre-configured spending plans

♻️ This comment has been updated with latest results.

@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
@hashgraph hashgraph deleted a comment from github-actions bot Oct 25, 2024
ebadiere
ebadiere previously approved these changes Oct 25, 2024
Copy link
Contributor

@ebadiere ebadiere left a comment

Choose a reason for hiding this comment

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

LG.

natanasow
natanasow previously approved these changes Oct 25, 2024
Copy link
Collaborator

@natanasow natanasow left a comment

Choose a reason for hiding this comment

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

@victor-yanev
Copy link
Contributor Author

LGTM, but we should consider adding an exception for this check as well https://github.com/hashgraph/hedera-services/blob/develop/hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/TransactionProcessor.java#L233 in a follow-up PR.

@natanasow Could you open a new issue for that, we could prioritize it in the next sprint. 😄

@natanasow
Copy link
Collaborator

LGTM, but we should consider adding an exception for this check as well https://github.com/hashgraph/hedera-services/blob/develop/hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/TransactionProcessor.java#L233 in a follow-up PR.

@natanasow Could you open a new issue for that, we could prioritize it in the next sprint. 😄

Okay.

Signed-off-by: Victor Yanev <[email protected]>
@victor-yanev victor-yanev dismissed stale reviews from natanasow and ebadiere via ad8d69d October 28, 2024 08:39
Copy link

Copy link

🚨 Memory Leak Detected 🚨

A potential memory leak has been detected in the test titled validates enforcement of request id. This may impact the application's performance and stability.

Details

📊 Memory Leak Detection Report 📊

GC Type: MarkSweepCompact
Cost: 28,397.9 ms

Heap Statistics (before vs after executing the test):

  • Total Heap Size: increased with 1.46 MB
  • Total Heap Size Executable: no changes
  • Total Physical Size: decreased with 348.16 KB
  • Total Available Size: increased with 2.84 MB
  • Total Global Handles Size: no changes
  • Used Global Handles Size: decreased with 64.00 bytes
  • Used Heap Size: decreased with 3.58 MB
  • Heap Size Limit: no changes
  • Malloced Memory: no changes
  • External Memory: no changes
  • Peak Malloced Memory: no changes

Heap Space Statistics (before vs after executing the test):

  • Old Space:

    • Space Size: increased with 1.84 MB
    • Space Used Size: increased with 1.93 MB
    • Space Available Size: increased with 99.46 KB
    • Physical Space Size: increased with 1.84 MB
  • Large Object Space:

    • Space Size: increased with 835.58 KB
    • Space Used Size: increased with 813.50 KB
    • Space Available Size: no changes
    • Physical Space Size: increased with 835.58 KB

Recommendations

Please investigate the memory allocations in this test, focusing on objects that are not being properly deallocated.

@victor-yanev victor-yanev merged commit 3979ee0 into main Oct 30, 2024
43 of 44 checks passed
@victor-yanev victor-yanev deleted the 3115-Inconsistent-Handling-of-Failed-EVM-Transactions branch October 30, 2024 09:06
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.28%. Comparing base (6442c98) to head (ad8d69d).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
packages/relay/src/lib/eth.ts 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3158      +/-   ##
==========================================
+ Coverage   83.19%   83.28%   +0.09%     
==========================================
  Files          66       66              
  Lines        4254     4260       +6     
  Branches      829      832       +3     
==========================================
+ Hits         3539     3548       +9     
+ Misses        472      471       -1     
+ Partials      243      241       -2     
Flag Coverage Δ
config-service 98.14% <ø> (ø)
relay 85.47% <87.50%> (+0.11%) ⬆️
server 83.52% <ø> (ø)
ws-server 36.87% <ø> (ø)

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

Files with missing lines Coverage Δ
packages/relay/src/lib/eth.ts 82.33% <87.50%> (+0.36%) ⬆️

... and 2 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent Handling of Failed EVM Transactions in JSON-RPC Relay
6 participants