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.

Sign up for GitHub

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

Jump to bottom

Deep VM queries: deep-history observer vs. regular observer #6005

Merged
merged 10 commits into from
Mar 5, 2024
Merged

Deep VM queries: deep-history observer vs. regular observer #6005

merged 10 commits into from
Mar 5, 2024

Conversation

andreibancioiu
Copy link
Collaborator

@andreibancioiu andreibancioiu commented Mar 1, 2024 •
edited
Loading

Reasoning behind the pull request

  • In some cases, deep VM queries against old epochs caused subsequent VM queries on recent blocks to fail.
print("Causing the issue...")
do_contract_query(recent_block - 6 * 14400)
do_contract_query(recent_block - 5 * 14400)
do_contract_query(recent_block - 4 * 14400)
print("This will fail")
do_contract_query(recent_block)

print("Repairing the issue...")
do_contract_query(recent_block - 3 * 14400)

print("Now it will work")
do_contract_query(recent_block)

Proposed changes

If observer is started in deep-history mode, perform deep VM queries, using recreateTrieFromEpoch. Otherwise, rely on recreateTrie. Most important work is done by #6008.

Testing procedure

  • Regular testing
  • VM queries stressing (default)
  • VM queries against both old and new blocks (not only sequential queries, but also random access queries)

Log level: *:DEBUG,process/smartcontract.queryService:TRACE.

Inspect logs: calling RecreateTrie|calling RecreateTrieFromEpoch.

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

@andreibancioiu andreibancioiu self-assigned this Mar 1, 2024
iulianpascalau
iulianpascalau reviewed Mar 1, 2024
View reviewed changes
process/smartContract/scQueryService.go Outdated
return err
}

service.rememberQueriedEpoch(blockHeader.GetEpoch())
Copy link
Contributor

Choose a reason for hiding this comment

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

missing return here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, fixed.

BeniaminDrasovean
BeniaminDrasovean reviewed Mar 1, 2024
View reviewed changes
process/smartContract/scQueryService.go Outdated
}

service.rememberQueriedEpoch(blockHeader.GetEpoch())
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

return nil to be more obvious

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, fixed.

@andreibancioiu andreibancioiu marked this pull request as ready for review March 1, 2024 12:34
@codecov Codecov
Copy link

codecov bot commented Mar 1, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.20%. Comparing base (688b8ff) to head (ca68006).

Additional details and impacted files 
@@                Coverage Diff                @@
##           rc/v1.6.next1    #6005      +/-   ##
=================================================
- Coverage          80.21%   80.20%   -0.01%     
=================================================
  Files                708      709       +1     
  Lines              94139    94171      +32     
=================================================
+ Hits               75510    75533      +23     
- Misses             13280    13289       +9     
  Partials            5349     5349

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

iulianpascalau
iulianpascalau previously approved these changes Mar 1, 2024
View reviewed changes
@iulianpascalau iulianpascalau dismissed stale reviews from BeniaminDrasovean and themself via ca68006 March 4, 2024 09:01
@andreibancioiu andreibancioiu changed the title Deep VM queries: remember latest queried epoch Deep VM queries: deep-history observer vs. regular observer Mar 4, 2024
gabi-vuls
gabi-vuls approved these changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

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

Normal allin test: v1.6.15-dev-config-eb2e06c06d -> vm-queries-remember-epoch-ca68006b99

--- Specific errors ---

block hash does not match 1877
wrong nonce in block 912
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0

/------/

--- Statistics ---

Nr. of all ERRORS: 0
Nr. of all WARNS: 1064
Nr. of new ERRORS: 0
Nr. of new WARNS: 40
Nr. of PANICS: 0

/------/

--- ERRORS ---

/------/

--- WARNINGS ---

/------/

@iulianpascalau iulianpascalau merged commit f731d80 into rc/v1.6.next1 Mar 5, 2024
8 checks passed
@iulianpascalau iulianpascalau deleted the vm-queries-remember-epoch branch March 5, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iulianpascalau iulianpascalau iulianpascalau approved these changes

@BeniaminDrasovean BeniaminDrasovean BeniaminDrasovean approved these changes

@gabi-vuls gabi-vuls gabi-vuls approved these changes

Assignees

@andreibancioiu andreibancioiu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andreibancioiu @iulianpascalau @BeniaminDrasovean @gabi-vuls