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

Refactored transaction simulation construction #5201

Merged
merged 17 commits into from May 17, 2023

Conversation

iulianpascalau
Copy link
Contributor

Reasoning behind the pull request

  • the processing AccountsDB instance can be dirtied in some cases if transaction simulation processes are running

Proposed changes

  • complete refactor of the transaction simulation components in order to avoid sending the same AccountsDB instance used in processing

Testing procedure

  • standard system test doing also transaction simulation operations. Log error prints should not occur.

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?

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Patch coverage: 76.26% and project coverage change: +0.04 🎉

Comparison is base (0dfb43b) 79.69% compared to head (6fdf983) 79.74%.

❗ Current head 6fdf983 differs from pull request most recent head 5630cf7. Consider uploading reports for the commit 5630cf7 to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##           rc/v1.6.0    #5201      +/-   ##
=============================================
+ Coverage      79.69%   79.74%   +0.04%     
=============================================
  Files            682      683       +1     
  Lines          88256    88348      +92     
=============================================
+ Hits           70340    70453     +113     
+ Misses         12789    12762      -27     
- Partials        5127     5133       +6     
Impacted Files Coverage Δ
api/middleware/responseLogger.go 66.66% <0.00%> (ø)
factory/processing/blockProcessorCreator.go 81.88% <ø> (+2.22%) ⬆️
factory/processing/txSimulatorProcessComponents.go 75.70% <75.70%> (ø)
factory/processing/processComponents.go 83.02% <100.00%> (+0.01%) ⬆️
vm/systemSmartContracts/eei.go 75.00% <100.00%> (-0.06%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@iulianpascalau iulianpascalau self-assigned this Apr 27, 2023
@iulianpascalau iulianpascalau changed the title [WIP]Refactored transaction simulation construction Refactored transaction simulation construction Apr 27, 2023
@iulianpascalau iulianpascalau marked this pull request as ready for review April 27, 2023 12:30
@bogdan-rosianu bogdan-rosianu self-requested a review April 27, 2023 12:37
"github.com/multiversx/mx-chain-vm-common-go/parsers"
)

func (pcf *processComponentsFactory) createTxSimulatorProcessor() (factory.TransactionSimulatorProcessor, process.VirtualMachinesContainerFactory, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename this file to txSimulatorProcessComponents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

func (pr *ProcessorRunner) CreateDeploySCTx(
tb testing.TB,
owner []byte,
pathToContract string,
Copy link
Contributor

Choose a reason for hiding this comment

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

contractPath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

newConfigsPath := path.Join(tempDir, "config")

// TODO refactor this cp to work on all OSes
cmd := exec.Command("cp", "-r", originalConfigsPath, newConfigsPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need this copy? since you don't change the files, you could have used them as they are

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was also my first impression when I started constructing the first implementation of this function.
If you look at L28, I have this function correctTestPathInGenesisSmartContracts that does a sort of sed operation, replacing the string ./config to the actually temporary directory. Without this change, the genesis process will always fail.

bogdan-rosianu
bogdan-rosianu previously approved these changes Apr 27, 2023
miiu96
miiu96 previously approved these changes Apr 28, 2023
gabi-vuls
gabi-vuls previously approved these changes May 6, 2023
Copy link
Collaborator

@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.

System test passed.
@@ Log scanner @@

tx-simulator-construction-refactor

================================================================================

  • Known Warnings 10
  • New Warnings 2
  • Known Errors 0
  • New Errors 1
  • Panics 0
    ================================================================================
  • block hash does not match 10033
  • wrong nonce in block 3624
  • 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
    ================================================================================
  • No jailed nodes on the testnet
    ================================================================================

# Conflicts:
#	factory/processing/export_test.go
#	factory/processing/processComponents.go
bogdan-rosianu
bogdan-rosianu previously approved these changes May 10, 2023
miiu96
miiu96 previously approved these changes May 10, 2023
@iulianpascalau iulianpascalau dismissed stale reviews from bogdan-rosianu and miiu96 via 20400d4 May 17, 2023 07:10
Copy link
Collaborator

@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.

System test passed

@iulianpascalau iulianpascalau merged commit 2406623 into rc/v1.6.0 May 17, 2023
5 checks passed
@iulianpascalau iulianpascalau deleted the tx-simulator-construction-refactor branch May 17, 2023 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants