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

Optimized trie re-creation for VM Queries, adjust some logs #5977

Merged
merged 14 commits into from
Feb 23, 2024

Conversation

andreibancioiu
Copy link
Collaborator

@andreibancioiu andreibancioiu commented Feb 17, 2024

Reasoning behind the pull request

  • fixed recreate trie in sc query service

Proposed changes

  • Optimize trie re-creation for VM queries: in SC query service, decide whether to call recreateTrie or recreateTrieFromEpoch - by @iulianpascalau, in Fixed recreate trie in sc query service #5983
  • Removed some dead code (added long time ago - it contained workarounds for the old way of Wasmer instance reuse), adjusted some logs

Testing procedure

  • Standard testing
  • Stress observers with VM queries (for recent blocks)
  • Stress observers with VM queries (for older blocks)

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 Feb 17, 2024
Copy link

codecov bot commented Feb 17, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 80.21%. Comparing base (6faf35a) to head (f9bcc00).

Files Patch % Lines
state/accountsDBApi.go 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           rc/v1.6.next1    #5977      +/-   ##
=================================================
+ Coverage          80.17%   80.21%   +0.03%     
=================================================
  Files                708      708              
  Lines              94129    94139      +10     
=================================================
+ Hits               75469    75511      +42     
+ Misses             13313    13281      -32     
  Partials            5347     5347              

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

iulianpascalau
iulianpascalau previously approved these changes Feb 19, 2024
Copy link
Contributor

@iulianpascalau iulianpascalau left a comment

Choose a reason for hiding this comment

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

maybe retarget towards master or a new rc/v1.6.next2 as the .next1 is ready to be shipped.

@@ -179,8 +181,7 @@ func (service *SCQueryService) shouldAllowQueriesExecution() bool {
}

func (service *SCQueryService) executeScCall(query *process.SCQuery, gasPrice uint64) (*vmcommon.VMOutput, common.BlockInfo, error) {
log.Trace("executeScCall", "function", query.FuncName, "numQueries", service.numQueries)
service.numQueries++
logQueryService.Trace("executeScCall", "address", query.ScAddress, "function", query.FuncName, "blockNonce", query.BlockNonce.Value, "blockHash", query.BlockHash)
Copy link
Contributor

Choose a reason for hiding this comment

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

Debate-able if L184 and L197 should be at debug level.

@andreibancioiu andreibancioiu marked this pull request as ready for review February 20, 2024 15:25
@andreibancioiu andreibancioiu changed the title Vm query logs Optimized trie recreation for VM Queries, adjust some logs Feb 20, 2024
@@ -193,15 +195,14 @@ func (service *SCQueryService) executeScCall(query *process.SCQuery, gasPrice ui
}

if len(blockRootHash) > 0 {
logQueryService.Trace("preparing execution for block and root hash", "block", blockHeader.GetNonce(), "rootHash", blockRootHash)
Copy link
Collaborator

@sstanculeanu sstanculeanu Feb 20, 2024

Choose a reason for hiding this comment

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

the check for nil blockHeader from L243 should also be done before this log

also needed for recreateTrie

Copy link
Collaborator Author

@andreibancioiu andreibancioiu Feb 20, 2024

Choose a reason for hiding this comment

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

Fixed (moved log in recreateTrie, as well). Could not reproduce this scenario using a localnet. VM queries are not available at genesis, though. Thus, added an extra simple unit test.

@@ -175,13 +175,21 @@ func (accountsDB *accountsDBApi) RecreateTrieFromEpoch(options common.RootHashHo
accountsDB.mutRecreatedTrieBlockInfo.Lock()
defer accountsDB.mutRecreatedTrieBlockInfo.Unlock()

if options == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if options == nil {
if check.IfNil(options) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Applied.

@andreibancioiu andreibancioiu changed the title Optimized trie recreation for VM Queries, adjust some logs Optimized trie re-creation for VM Queries, adjust some logs Feb 20, 2024
@iulianpascalau iulianpascalau self-assigned this Feb 21, 2024
Copy link
Contributor

@iulianpascalau iulianpascalau left a comment

Choose a reason for hiding this comment

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

Looks good from my side 👍 , won't approve since I've contributed with code on this PR

@bogdan-rosianu bogdan-rosianu self-requested a review February 21, 2024 08:31
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-query-logs-f9bcc00f93

--- Specific errors ---

block hash does not match 3516
wrong nonce in block 1183
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: 219
Nr. of new ERRORS: 0
Nr. of new WARNS: 8
Nr. of PANICS: 0

/------/

--- ERRORS ---

/------/

--- WARNINGS ---

@iulianpascalau iulianpascalau merged commit 688b8ff into rc/v1.6.next1 Feb 23, 2024
8 checks passed
@iulianpascalau iulianpascalau deleted the vm-query-logs branch February 23, 2024 07:51
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.

5 participants