Block coordinates on vm query #5447

sstanculeanu · 2023-07-21T12:51:15Z

Reasoning behind the pull request

add block coordinates on vm query

Proposed changes

added block coordinates option on vm query

Testing procedure

will be tested on feat branch

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 · 2023-07-21T13:05:53Z

Codecov Report

❗ No coverage uploaded for pull request base (feat/vmquery_with_block_coordinates@7c5d69e). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 4743403 differs from pull request most recent head 342c99b. Consider uploading reports for the commit 342c99b to get more accurate results

Additional details and impacted files

@@                          Coverage Diff                           @@
##             feat/vmquery_with_block_coordinates    #5447   +/-   ##
======================================================================
  Coverage                                       ?   80.07%           
======================================================================
  Files                                          ?      706           
  Lines                                          ?    93619           
  Branches                                       ?        0           
======================================================================
  Hits                                           ?    74965           
  Misses                                         ?    13303           
  Partials                                       ?     5351

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

bogdan-rosianu

small comments. tested it, works well 👍

api/groups/vmValuesGroup.go

dataRetriever/blockchain/apiBlockchain.go

bogdan-rosianu · 2023-07-26T10:35:12Z

dataRetriever/blockchain/apiBlockchain.go

+
+// GetCurrentBlockRootHash returns the current block root hash from the main chain handler, if no local root hash is set
+// if there is a local root hash, it will be returned until its reset
+func (abc *apiBlockchain) GetCurrentBlockRootHash() []byte {


in a normal VM Query, is SetCurrentBlockHeaderAndRootHash called each time? Otherwise, a gateway user might send a request for a block from yesterday and the next request (from another user, without coordinates) will use the root hash from yesterday instead of the current one

tried to cover all the cases when executeScCall would early exit, in order to properly reset the root hash and avoid this situation

Why would it need to reset at exit?

I would instead, from the API call prepare the apiBlockchain with either the main blockchain current data, or with the preferred blockchain data given by the developer, and then do the sc query/etc.

In that case there should be no edge cases that need to be taken care of, and will be more straight forward as you will always set the block coordinates you are working with on each of the calls.

implemented the suggestion

bogdan-rosianu · 2023-07-26T10:48:03Z

genesis/process/genesisBlockCreator.go

+		return process.ErrNilHistoryRepository
+	}
+	if check.IfNil(arg.ScheduledTxsExecutionHandler) {
+		return process.ErrNilScheduledTxsExecutionHandler


not sure, but I think that for genesis, a disabled scheduled txs execution handler would have been enough

exactly, I do not see the reason for injecting this on the genesis processor

process/errors.go

api/groups/vmValuesGroup.go

iulianpascalau · 2023-07-27T07:06:24Z

factory/processing/processComponents.go

 	pcf.txLogsProcessor = txLogsProcessor
-	genesisBlocks, initialTxs, err := pcf.generateGenesisHeadersAndApplyInitialBalances()
+	genesisBlocks, initialTxs, err := pcf.generateGenesisHeadersAndApplyInitialBalances(scheduledTxsExecutionHandler)


why this change? Why do we need to inject the scheduledTxsExecutionHandler as a parameter?

agree, I don't see where this will be needed for the VM query

iulianpascalau · 2023-07-27T07:07:11Z

genesis/process/genesisBlockCreator.go

+		return process.ErrNilHistoryRepository
+	}
+	if check.IfNil(arg.ScheduledTxsExecutionHandler) {
+		return process.ErrNilScheduledTxsExecutionHandler


exactly, I do not see the reason for injecting this on the genesis processor

iulianpascalau · 2023-07-27T07:11:19Z

factory/api/apiResolverFactory.go

 	}

 	return smartContract.NewSCQueryService(argsNewSCQueryService)
 }

+func createNewAccountsAdapterApi(args *scQueryElementArgs, chainHandler data.ChainHandler) (state.AccountsAdapterAPI, error) {


so we will create a new trie for each SC query service instance.
I think we have this, in one form or another, when we create the AccountsDBApi instance. We just need to create more of those instances and pass them to each SC query service element.

AccountsDBApi is indeed called below, what we were missing in the original create accounts adapter was a separate merkle trie instance for each of the sc query instances, and a separate chain handler.

We need more AccountsDBApi instances as it is easier to manage the inner trie instances that way. AccountsDB is just a wrapper over the trie.

iulianpascalau · 2023-07-27T07:14:31Z

process/smartContract/scQueryService.go

 	query = prepareScQuery(query)
 	vmInput := service.createVMCallInput(query, gasPrice)
 	vmOutput, err := vm.RunSmartContractCall(vmInput)
 	service.wasmVMChangeLocker.RUnlock()
 	if err != nil {
+		// Cleaning the current root hash so the real one would be returned further
+		_ = service.blockChain.SetCurrentBlockHeaderAndRootHash(nil, nil)


can call this on a defer function after L207

AdoAdoAdo · 2023-07-27T09:38:35Z

dataRetriever/blockchain/apiBlockchain.go

+
+// GetCurrentBlockRootHash returns the current block root hash from the main chain handler, if no local root hash is set
+// if there is a local root hash, it will be returned until its reset
+func (abc *apiBlockchain) GetCurrentBlockRootHash() []byte {


Why would it need to reset at exit?

I would instead, from the API call prepare the apiBlockchain with either the main blockchain current data, or with the preferred blockchain data given by the developer, and then do the sc query/etc.

In that case there should be no edge cases that need to be taken care of, and will be more straight forward as you will always set the block coordinates you are working with on each of the calls.

AdoAdoAdo · 2023-07-27T09:59:43Z

factory/api/apiResolverFactory.go

 		index:                 0,
+		processingMode:        args.processingMode,


what is this for?

it is used for ArgsAccountsDB

AdoAdoAdo · 2023-07-27T10:11:48Z

factory/api/apiResolverFactory.go

 	}

 	return smartContract.NewSCQueryService(argsNewSCQueryService)
 }

+func createNewAccountsAdapterApi(args *scQueryElementArgs, chainHandler data.ChainHandler) (state.AccountsAdapterAPI, error) {


AccountsDBApi is indeed called below, what we were missing in the original create accounts adapter was a separate merkle trie instance for each of the sc query instances, and a separate chain handler.

AdoAdoAdo · 2023-07-27T12:03:57Z

factory/processing/processComponents.go

 	pcf.txLogsProcessor = txLogsProcessor
-	genesisBlocks, initialTxs, err := pcf.generateGenesisHeadersAndApplyInitialBalances()
+	genesisBlocks, initialTxs, err := pcf.generateGenesisHeadersAndApplyInitialBalances(scheduledTxsExecutionHandler)


agree, I don't see where this will be needed for the VM query

AdoAdoAdo · 2023-07-27T12:09:45Z

process/smartContract/scQueryService.go

+}
+
+func (service *SCQueryService) getBlockRootHash(headerHash []byte, header data.HeaderHandler) []byte {
+	blockRootHash, err := service.scheduledTxsExecutionHandler.GetScheduledRootHashForHeaderWithEpoch(


OK I see why you added the scheduled execution handler, but you can use instead getBlockHeaderByNonce and request the header.Nonce+1 and check on that the scheduledRootHash.

We can discuss as well, I see that this is similar to how it is done for on nodeBlocks.go
This code appears duplicated, maybe it would be better to extract a component and reuse it in both places instead.

added a TODO for the duplicated code

api/groups/vmValuesGroup_test.go

factory/api/apiResolverFactory.go

process/smartContract/scQueryService.go

AdoAdoAdo · 2023-08-07T12:14:55Z

genesis/process/shardGenesisBlockCreator.go

+		StorageService:           arg.Data.StorageService(),
+		Marshaller:               arg.Core.InternalMarshalizer(),
+		Hasher:                   arg.Core.Hasher(),
+		Uint64ByteSliceConverter: arg.Core.Uint64ByteSliceConverter(),
 	}
 	queryService, err := smartContract.NewSCQueryService(argsNewSCQueryService)


Is it really necessary to have a real sc query service on the genesis block creator? @iulianpascalau
I see we execute the genesis node staking transactions and then we use the sc query system to verify that the node is indeed staked via sc query.
Also for the DNS contracts we use it to check the version.

Maybe we should discuss if we really need this.

At the time it was the easiest way to verify that the genesis was constructed correctly. As seen, during testing, without these checks we can have a malformed genesis state if we got some genesis parameters wrong (like the initial number of staked nodes). I strongly encourage to keep them as they are now in order to safeguard the genesis state creation.

process/interface.go

AdoAdoAdo · 2023-08-07T13:00:54Z

process/smartContract/scQueryService.go

+		}
+
+		header := service.getBlockHeader(blockHeader)
+		return header, header.GetRootHash(), nil


I see that you extract the roothash from the following block after the 'block by hash'

I think the header to return is exactly the block by hash, but the rootHash to return here should be the ScheduledRootHash from the following block after the block by hash

return blockHeader, header.GetScheduledRootHash, nil

AdoAdoAdo · 2023-08-07T13:02:51Z

process/smartContract/scQueryService.go

+}
+
+func (service *SCQueryService) getBlockHeader(currentHeader data.HeaderHandler) data.HeaderHandler {
+	header, _, err := service.getBlockHeaderByNonce(currentHeader.GetNonce() + 1)


why nonce +1 here?
Maybe rename the caller method to getNextBlockHeader?

also I don't think we need this method, as we already have getBlockByNonce (current header nonce +1), and an error on this get should not use the current header, but propagate the error.

removed the method and updated as suggested

AdoAdoAdo · 2023-08-07T13:16:56Z

process/smartContract/scQueryService.go

 }

 func (service *SCQueryService) checkForRootHashChanges(rootHashBefore []byte) error {
-	rootHashAfter := service.blockChain.GetCurrentBlockRootHash()
+	rootHashAfter := service.mainBlockChain.GetCurrentBlockRootHash()


shouldn't it verify this against the api blockchain block roothash?

I would only use the mainBlockChain to set the apiBlockChain data when the API is called without block parameters (so for current)

AdoAdoAdo · 2023-08-07T13:22:19Z

state/accountsDBApi.go

-	return ErrOperationNotPermitted
+// RecreateTrie is used to reload the trie based on an existing rootHash
+func (accountsDB *accountsDBApi) RecreateTrie(rootHash []byte) error {
+	_, err := accountsDB.doRecreateTrieWithBlockInfo(holders.NewBlockInfo([]byte{}, 0, rootHash))


do you afterwards use the blockInfo set on the accountsDBApi?
if yes, the maybe use instead

accountsDB.innerAccountsAdapter.RecreateTrie(rootHash)

reused doRecreateTrieWithBlockInfo method because the resulted blockInfo is then returned on GetAccountWithBlockInfo and RootHash

…ates_on_vm_query

AdoAdoAdo · 2023-08-21T07:01:11Z

process/smartContract/scQueryService.go

 		if err != nil {
 			return nil, nil, err
 		}

-		header := service.getBlockHeader(blockHeader)
-		return header, header.GetRootHash(), nil
+		blockHeader, _, err := service.getBlockHeaderByNonce(currentHeader.GetNonce() + 1)


this looks similar to lines 268-278, can you maybe extract a method (getRootHashForBlock)?

added support for block coordinates on vm query API

bbbf244

sstanculeanu added type:feature New feature or request area:node-api Node API impact:low interested:protocol complexity:medium labels Jul 21, 2023

sstanculeanu self-assigned this Jul 21, 2023

sstanculeanu added 3 commits July 24, 2023 14:08

added functionality on accountsDBApi RecreateTrie

fa303bf

added new chain handler for api

b3897a1

fixed tests

11c4e9f

sstanculeanu marked this pull request as ready for review July 26, 2023 08:26

AdoAdoAdo self-requested a review July 26, 2023 08:56

bogdan-rosianu self-requested a review July 26, 2023 10:15

bogdan-rosianu reviewed Jul 26, 2023

View reviewed changes

iulianpascalau reviewed Jul 27, 2023

View reviewed changes

AdoAdoAdo requested changes Jul 27, 2023

View reviewed changes

sstanculeanu added 5 commits August 4, 2023 17:13

fixes after review

923be2c

added block info on vm query response

a715d61

fix tests

a92a5d6

fixed get header hash

6ebc69b

fixed more tests

36d361d

bogdan-rosianu reviewed Aug 7, 2023

View reviewed changes

api/groups/vmValuesGroup_test.go Show resolved Hide resolved

factory/api/apiResolverFactory.go Outdated Show resolved Hide resolved

process/smartContract/scQueryService.go Outdated Show resolved Hide resolved

fixes after review

2d20a97

AdoAdoAdo requested changes Aug 7, 2023

View reviewed changes

fixes after review

7256aaf

sstanculeanu changed the base branch from feat/block_coordinates_on_vmquery to feat/vmquery_with_block_coordinates August 16, 2023 10:41

Merge branch 'feat/vmquery_with_block_coordinates' into block_coordin…

6a07d6c

…ates_on_vm_query

bogdan-rosianu previously approved these changes Aug 16, 2023

View reviewed changes

AdoAdoAdo reviewed Aug 21, 2023

View reviewed changes

fix after review

342c99b

sstanculeanu dismissed bogdan-rosianu’s stale review via 342c99b August 21, 2023 07:27

AdoAdoAdo approved these changes Aug 21, 2023

View reviewed changes

bogdan-rosianu approved these changes Aug 22, 2023

View reviewed changes

iulianpascalau approved these changes Aug 22, 2023

View reviewed changes

sstanculeanu merged commit 59ebb58 into feat/vmquery_with_block_coordinates Aug 22, 2023

sstanculeanu deleted the block_coordinates_on_vm_query branch August 22, 2023 10:27

Block coordinates on vm query #5447

Block coordinates on vm query #5447

Conversation

sstanculeanu commented Jul 21, 2023 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov bot commented Jul 21, 2023 • edited Loading

Codecov Report

bogdan-rosianu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu commented Jul 21, 2023 •

edited

Loading

codecov bot commented Jul 21, 2023 •

edited

Loading

sstanculeanu Aug 16, 2023 •

edited

Loading