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

EN-7307: transaction execution simulator #2218

Merged
merged 12 commits into from
Aug 20, 2020

Conversation

bogdan-rosianu
Copy link
Contributor

@bogdan-rosianu bogdan-rosianu commented Aug 12, 2020

Transaction execution simulator. It will work via the API endpoint /transaction/simulate which receives the same request as the /transaction/send.
Created a new component that will simulate the execution of a transaction without affecting the state of the blockchain. In order to achieve this, some components of the new transaction processor were disabled or changed to read-only.

Testing steps:

  1. Run a testnet with the branch's configuration (be careful that the new endpoint has to be added to the api.toml file)
  2. Send requests to the /transaction/simulate endpoint. Multiple scenarios are expected:
  • move-balance transaction with invalid fields / insufficient balance / gas limit and so on. the proper error message should be returned
  • move-balance transaction with ok values. The response should signal that the transaction is correct. Please check that the receiver account hasn't updated the balance (the state was not affected). Also, in case that the gas limit was higher than the needed one, you should see a receipt inside the response
  • same two above scenarios, but for smart contract calls.

}

executionResults.Hash = hex.EncodeToString(txHash)

Copy link
Contributor

Choose a reason for hiding this comment

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

delete empty line

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

@@ -2,6 +2,7 @@ package factory

import (
"errors"
"github.com/ElrondNetwork/elrond-go/epochStart/bootstrap/disabled"
Copy link
Contributor

Choose a reason for hiding this comment

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

goimports

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

@@ -1323,6 +1332,13 @@ func newShardBlockProcessor(
return nil, errors.New("could not create transaction statisticsProcessor: " + err.Error())
}

txSimulatorProcessorArgs := argsNewTxProcessor
Copy link
Contributor

Choose a reason for hiding this comment

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

do the same for the underlying smart contract processor - you need to feed diabled fee handler and new intermediate processor for the simulator.

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

@@ -1323,6 +1332,13 @@ func newShardBlockProcessor(
return nil, errors.New("could not create transaction statisticsProcessor: " + err.Error())
}

txSimulatorProcessorArgs := argsNewTxProcessor
txSimulatorProcessorArgs.Accounts = disabled.NewAccountsAdapter()
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of disabled accounts adapter - you have to create a read only accounts adapter. thus you need a new component which gets the accountsDB as parameter and does all the reading from it, and in case of writes it writes into a cache, or it has an empty implementation

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

node/txsimulator/interface.go Show resolved Hide resolved

// ProcessTx will process the transaction in a special environment, where state-writing is not allowed
func (ts *transactionSimulator) ProcessTx(accounts state.AccountsAdapter, tx *transaction.Transaction) (*transaction.SimulationResults, error) {
newAccounts, err := NewReadOnlyAccountsDB(accounts)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can create this only once - at constructor level. and you have access to all the current state. no need to recreate for every simulation process.

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


// JournalLen will call the original accounts' function with the same name
func (w *readOnlyAccountsDB) JournalLen() int {
w.mutAccounts.RLock()
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for mutex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

)

//TODO increase code coverage

type interf interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this :) no need for that and for the next 20 lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed by mistake. removed

}

// New returns a new instance of a transactionSimulator
func New(txProcessor TransactionProcessor, accountsAdapter state.AccountsAdapter) (*transactionSimulator, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

New what ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the package is called txsimulator so the constructor's call will result in txsimulator.New(). txsimulator.NewTxSimulator() would have been redundant

@@ -20,6 +21,16 @@ type metaTxProcessor struct {
txTypeHandler process.TxTypeHandler
}

// GetSmartContractResults returns the smart contract results
func (txProc *metaTxProcessor) GetSmartContractResults() map[string]data.TransactionHandler {
return make(map[string]data.TransactionHandler)
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

meta sc processor creates smart contract results as well. you do not these. As in txSimulator you could have the scrForwarder and receiptForwarders and take those data from there.

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. refactored and created structs for a more friendly API result

}

// SetAccountsAdapter updates the accounts adapter
func (txProc *metaTxProcessor) SetAccountsAdapter(accounts state.AccountsAdapter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for this set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. this resulted in overwriting the read-only accounts db into the real one


// GetSmartContractResults returns the smart contract results in the current round
func (txProc *txProcessor) GetSmartContractResults() map[string]data.TransactionHandler {
return txProc.scrForwarder.GetAllCurrentFinishedTxs()
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

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

// SetAccountsAdapter will update the accounts adapter if the provided one is not nil
func (txProc *txProcessor) SetAccountsAdapter(accounts state.AccountsAdapter) {
if !check.IfNil(accounts) {
txProc.accounts = accounts
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for this set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@bogdan-rosianu bogdan-rosianu marked this pull request as ready for review August 17, 2020 11:41
@bogdan-rosianu bogdan-rosianu changed the title EN-7307: transaction execution simulator (WIP) EN-7307: transaction execution simulator Aug 17, 2020
@bogdan-rosianu bogdan-rosianu self-assigned this Aug 17, 2020
cmd/node/main.go Outdated
)
processComponents, err := factory.ProcessComponentsFactory(processArgs)
if err != nil {
return err
}

transactionSimulator, err := txsimulator.New(*txSimulatorProcessorArgs)
Copy link
Contributor

Choose a reason for hiding this comment

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

New what ? make a better naming for the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. renamed

if err != nil {
return err
}
receiptsForwarder, err := ts.intermProcContainer.Get(block.ReceiptBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no receipt block in case of metachain - thus this will fail.
What you should do it iterate all processors and call createblockstarted. Receipts are created only in intra shard move balance transfers with too much gas.

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. in case of meta, apply only the smart contract results and in case of shard, also the receipts. and on defer now it closes all the processors

valentin-lup
valentin-lup previously approved these changes Aug 18, 2020
Copy link
Collaborator

@valentin-lup valentin-lup left a comment

Choose a reason for hiding this comment

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

Tested on AZ-WEU with proxy EN-7303-tx-simulation
Upgrade path master -> EN-7307-tx-execution-simulator tested on AZ-UKS

sasurobert
sasurobert previously approved these changes Aug 18, 2020
Copy link
Contributor

@LucianMincu LucianMincu left a comment

Choose a reason for hiding this comment

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

System tests passed.

@LucianMincu LucianMincu merged commit 8af6755 into development Aug 20, 2020
@LucianMincu LucianMincu deleted the EN-7307-tx-execution-simulator branch August 20, 2020 17:15
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.

None yet

5 participants