For historical lookups, load block coordinates from older epochs, as well #4386

andreibancioiu · 2022-08-21T09:45:05Z

Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)

The initial implementation of historical lookup could not access non-active persisters to prepare block coordinates (e.g. to acquire the necessary root hash given a block nonce in an older epoch).

Proposed Changes

For historical lookup, load block coordinates from older epochs, as well - rely on dblookupext, on storer.GetFromEpoch() etc.
Pass epoch (hint) to accountsRepository, then to accountsDBApiWithHistory.
Note that the epoch (hint) that reaches accountsDBApiWithHistory isn't used in this PR. A separate PR will make use of the epoch hint to appropriately recreate the trie (from older root hashes).

Testing procedure

Make sure to set NumEpochsToKeep = 10 (or so) and NumActivePersisters = 5 or 6 or 7 (or so).
Same as Implement queries for historical balances (state) #4371, but try to access older blocks. Though, key not found errors could (should) occur at this moment (depending on the queried epoch), since the accounts API cannot currently re-create a trie given an older rootHash (not found in the active persisters) - this will be fixed in a separate PR.

…chs, as well.

…unit tests. Refactoring of "nodeBlocks.go".

codecov-commenter · 2022-08-21T20:31:45Z

Codecov Report

❗ No coverage uploaded for pull request base (historical-balances-08-12@32fb586). Click here to learn what that means.
The diff coverage is n/a.

@@                     Coverage Diff                      @@
##             historical-balances-08-12    #4386   +/-   ##
============================================================
  Coverage                             ?   73.79%           
============================================================
  Files                                ?      674           
  Lines                                ?    86853           
  Branches                             ?        0           
============================================================
  Hits                                 ?    64094           
  Misses                               ?    17965           
  Partials                             ?     4794

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

iulianpascalau

Minor stuff. Will green after the target PR will get merged in the rc as to not double review this.

iulianpascalau · 2022-08-22T06:11:43Z

dblookupext/todo.txt

@@ -0,0 +1,3 @@
+ - Move the interface "HistoryRepository" where it's needed, not in the package where it's implemented


Mixed feelings about this todo file. Although will help us restructure the work I'm afraid it will be forgotten.

Right. Removed file, added EN-12965.

iulianpascalau · 2022-08-22T06:20:23Z

process/common.go

@@ -711,6 +712,24 @@ func GetSortedStorageUpdates(account *vmcommon.OutputAccount) []*vmcommon.Storag
 	return storageUpdates
 }

+func CreateHeader(shardId uint32, marshalizer marshal.Marshalizer, headerBuffer []byte) (data.HeaderHandler, error) {


missing comments here and on L723

iulianpascalau · 2022-08-22T06:21:13Z

state/accountsRepository.go

 	return accountsAdapter.GetAccountWithBlockInfo(address, convertedOptions)
 }

-func (repository *accountsRepository) convertAccountQueryOptions(options api.AccountQueryOptions) (common.RootHashHolder, error) {
-	return holders.NewRootHashHolder(options.BlockRootHash), nil
+func (repository *accountsRepository) convertAccountQueryOptions(options api.AccountQueryOptions) common.RootHashHolder {


👍 for removing the error output parameter

iulianpascalau · 2022-08-22T06:22:08Z

state/accountsRepository.go

-func (repository *accountsRepository) convertAccountQueryOptions(options api.AccountQueryOptions) (common.RootHashHolder, error) {
-	return holders.NewRootHashHolder(options.BlockRootHash), nil
+func (repository *accountsRepository) convertAccountQueryOptions(options api.AccountQueryOptions) common.RootHashHolder {
+	if options.HintEpoch.HasValue {


instead of this if, you could have send the optionalUint32 parameter in the NewRootHashHolder constructor

iulianpascalau · 2022-08-22T06:24:03Z

node/nodeBlocks.go

-		n.coreComponents.Uint64ByteSliceConverter(),
-		n.coreComponents.InternalMarshalizer(),
-	)
+func (n *Node) getScheduledRootHash(epoch uint32, headerHash []byte) ([]byte, error) {


does this method is somehow the copy of the GetScheduledRootHashForHeader ?

Yes, but uses storer.GetFromEpoch() under the hood, instead of storer.Get(). Removed, refactored, added a new function: scheduledTxsExecution.GetScheduledRootHashForHeaderWithEpoch.

I didn't change the signature of the existing GetScheduledRootHashForHeader (only the private function it relies on) - is it all right?

yes, should be ok

bogdan-rosianu · 2022-08-22T07:37:18Z

api/groups/addressGroupOptions_test.go

@@ -48,5 +53,14 @@ func TestExtractAccountQueryOptions(t *testing.T) {
 		options, err = extractAccountQueryOptions(testscommon.CreateGinContextWithRawQuery("onStartOfEpoch=7&blockRootHash=bbbb"))
 		require.ErrorContains(t, err, "onStartOfEpoch is not compatible")
 		require.Equal(t, api.AccountQueryOptions{}, options)
+
+		options, err = extractAccountQueryOptions(testscommon.CreateGinContextWithRawQuery("onFinalBlock=true&hintEpoch=7"))
+		require.ErrorContains(t, err, "hintEpoch is optional, but only compatible with blockRootHash")


bogdan-rosianu · 2022-08-22T07:38:21Z

common/holders/rootHashHolder.go

+	}
+}
+
+// NewRootHashHolderWithEpoch creates a rootHashHolder


extend the comment by also setting the epoch of the rootHash

Removed this constructor (per Iulian's comment).

bogdan-rosianu · 2022-08-22T07:50:30Z

process/common.go

@@ -711,6 +712,24 @@ func GetSortedStorageUpdates(account *vmcommon.OutputAccount) []*vmcommon.Storag
 	return storageUpdates
 }

+func CreateHeader(shardId uint32, marshalizer marshal.Marshalizer, headerBuffer []byte) (data.HeaderHandler, error) {


maybe rename to UnmarshalHeader / UnmarshalMetaheader / UnmarshalShardHeader ?

Create makes me think of a new header, created by constructor

Right :) renamed.

…derWithEpoch".

iulianpascalau · 2022-08-22T09:07:43Z

process/block/preprocess/scheduledTxsExecution.go

@@ -590,13 +607,17 @@ func (ste *scheduledTxsExecution) getScheduledInfoForHeader(headerHash []byte) (
 		}
 	}()

-	marshalledSCRsSavedData, err := ste.storer.Get(headerHash)
+	if epoch.HasValue {


yup, this avoids the duplicated code 👍

iulianpascalau · 2022-08-22T09:13:20Z

state/accountsRepository.go

@@ -54,10 +54,7 @@ func (repository *accountsRepository) GetAccountWithBlockInfo(address []byte, op
 }

 func (repository *accountsRepository) convertAccountQueryOptions(options api.AccountQueryOptions) common.RootHashHolder {


kind of useless function now

Removed, inlined.

AdoAdoAdo · 2022-08-22T09:23:18Z

go.mod

@@ -9,7 +9,7 @@ require (
 	github.com/ElrondNetwork/concurrent-map v0.1.3
 	github.com/ElrondNetwork/covalent-indexer-go v1.0.6
 	github.com/ElrondNetwork/elastic-indexer-go v1.2.38
-	github.com/ElrondNetwork/elrond-go-core v1.1.19-0.20220813193723-a7d8c4aeaa87


proper tag?

Fixed, using v1.1.19.

AdoAdoAdo · 2022-08-22T09:31:27Z

node/nodeBlocks.go

 	)
 }
+
+func (n *Node) getBlockHeaderByHash(headerHash []byte) (data.HeaderHandler, error) {
+	epoch, err := n.getEpochByHash(headerHash)


can we add some support in case dblookup extensions is not enabled?
so we do not return error for example when we activate API calls on validators, and we query the recent (current epoch) block hashes.

With these changes, for validators the api will always return error (db lookup extension not activated) and previously it was not.

Fixed to incorporate a fallback mechanism when dblookup extensions is not enabled (theoretically, wasn't a breaking change - the new query parameters have been recently added).

gabi-vuls

System test passed.

Sketch: for historical lookups, load block coordinates from older epo…

afdc185

…chs, as well.

andreibancioiu self-assigned this Aug 21, 2022

andreibancioiu added 3 commits August 21, 2022 14:56

Fix tests.

2fb198d

Improve unit tests.

781b1eb

Pass epoch (hint) to accounts repository, then to accountsDBApi. Add …

8c09e8d

…unit tests. Refactoring of "nodeBlocks.go".

andreibancioiu mentioned this pull request Aug 21, 2022

Historical balances lookup: add hint for older epochs multiversx/mx-chain-core-go#88

Merged

Parse and check URL parameter hintEpoch.

95cd4dd

andreibancioiu added area:node-api Node API complexity:medium size:medium type:improvement interested:rosetta labels Aug 21, 2022

Re-order methods.

9a61f86

andreibancioiu marked this pull request as ready for review August 21, 2022 20:21

bogdan-rosianu self-requested a review August 22, 2022 06:05

iulianpascalau self-requested a review August 22, 2022 06:07

iulianpascalau reviewed Aug 22, 2022

View reviewed changes

AdoAdoAdo self-requested a review August 22, 2022 07:22

Fix after review (part 1).

81e4bd4

bogdan-rosianu reviewed Aug 22, 2022

View reviewed changes

andreibancioiu added 2 commits August 22, 2022 11:30

Fix after review (part 2): define and use "GetScheduledRootHashForHea…

f68073a

…derWithEpoch".

Fix after review (part 3) - renamings etc.

79d03a9

iulianpascalau previously approved these changes Aug 22, 2022

View reviewed changes

bogdan-rosianu previously approved these changes Aug 22, 2022

View reviewed changes

AdoAdoAdo requested changes Aug 22, 2022

View reviewed changes

Base automatically changed from historical-balances-08-12 to rc/2022-july August 22, 2022 09:45

andreibancioiu and others added 2 commits August 22, 2022 12:45

Merge branch 'rc/2022-july' into historical-lookup-past-epochs

0406692

Fix after review (part 3).

57e1d8d

andreibancioiu dismissed bogdan-rosianu’s stale review via 57e1d8d August 22, 2022 10:46

andreibancioiu dismissed iulianpascalau’s stale review via 57e1d8d August 22, 2022 10:46

andreibancioiu requested review from AdoAdoAdo, bogdan-rosianu and iulianpascalau August 22, 2022 10:50

iulianpascalau approved these changes Aug 22, 2022

View reviewed changes

bogdan-rosianu approved these changes Aug 22, 2022

View reviewed changes

AdoAdoAdo approved these changes Aug 22, 2022

View reviewed changes

andreibancioiu mentioned this pull request Aug 22, 2022

Recreate trie from epoch #4388

Merged

gabi-vuls approved these changes Aug 23, 2022

View reviewed changes

gabi-vuls merged commit e15520a into rc/2022-july Aug 23, 2022

gabi-vuls deleted the historical-lookup-past-epochs branch August 23, 2022 08:38

andreibancioiu mentioned this pull request Aug 25, 2022

Merge feat/historical into main multiversx/mx-chain-rosetta#33

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For historical lookups, load block coordinates from older epochs, as well #4386

For historical lookups, load block coordinates from older epochs, as well #4386

andreibancioiu commented Aug 21, 2022 •

edited

codecov-commenter commented Aug 21, 2022

iulianpascalau left a comment

iulianpascalau Aug 22, 2022

andreibancioiu Aug 22, 2022

iulianpascalau Aug 22, 2022

andreibancioiu Aug 22, 2022

iulianpascalau Aug 22, 2022

iulianpascalau Aug 22, 2022

andreibancioiu Aug 22, 2022

iulianpascalau Aug 22, 2022

andreibancioiu Aug 22, 2022 •

edited

iulianpascalau Aug 22, 2022

bogdan-rosianu Aug 22, 2022

bogdan-rosianu Aug 22, 2022

andreibancioiu Aug 22, 2022

bogdan-rosianu Aug 22, 2022

bogdan-rosianu Aug 22, 2022

andreibancioiu Aug 22, 2022

iulianpascalau Aug 22, 2022

iulianpascalau Aug 22, 2022

andreibancioiu Aug 22, 2022 •

edited

AdoAdoAdo Aug 22, 2022

andreibancioiu Aug 22, 2022

AdoAdoAdo Aug 22, 2022

andreibancioiu Aug 22, 2022

gabi-vuls left a comment

		@@ -0,0 +1,3 @@
		- Move the interface "HistoryRepository" where it's needed, not in the package where it's implemented

		@@ -54,10 +54,7 @@ func (repository *accountsRepository) GetAccountWithBlockInfo(address []byte, op
		}

		func (repository *accountsRepository) convertAccountQueryOptions(options api.AccountQueryOptions) common.RootHashHolder {

For historical lookups, load block coordinates from older epochs, as well #4386

For historical lookups, load block coordinates from older epochs, as well #4386

Conversation

andreibancioiu commented Aug 21, 2022 • edited

Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)

Proposed Changes

Testing procedure

codecov-commenter commented Aug 21, 2022

Codecov Report

iulianpascalau 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

andreibancioiu Aug 22, 2022 • edited

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

andreibancioiu Aug 22, 2022 • edited

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

gabi-vuls left a comment

Choose a reason for hiding this comment

andreibancioiu commented Aug 21, 2022 •

edited

andreibancioiu Aug 22, 2022 •

edited

andreibancioiu Aug 22, 2022 •

edited