refactor dataTrieTracker #4491

BeniaminDrasovean · 2022-09-21T12:25:56Z

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)

There were some duplicated mocks
TrackableDataTrie was exported

Proposed Changes

Remove duplicated mocks
Unexport trackableDataTrie and make in inaccessible form outside the account

Testing procedure

Normal test

…refactor # Conflicts: # common/interface.go # go.mod # go.sum

iulianpascalau · 2022-09-21T14:33:07Z

common/interface.go

@@ -50,6 +50,13 @@ type KeyBuilder interface {
 	Clone() KeyBuilder
 }

+// DataTrie is an interface that declares the methods used for dataTries
+type DataTrie interface {


DataTrieHandler ?

iulianpascalau · 2022-09-21T14:50:19Z

state/journalEntries.go

@@ -187,16 +188,21 @@ func NewJournalEntryDataTrieUpdates(trieUpdates map[string][]byte, account baseA

 // Revert applies undo operation
 func (jedtu *journalEntryDataTrieUpdates) Revert() (vmcommon.AccountHandler, error) {
+	trie, ok := jedtu.account.DataTrie().(common.Trie)


why does the account.DataTrie() method returns a subset of the Trie interface but we have the code swarming with casts like this?

There are only 2 casts in productive code (the rest are in tests). The idea behind this was to not give access directly to the trie (although this is possible through casting).

ok, as agreed, we will keep this as it is

iulianpascalau · 2022-09-21T15:19:11Z

state/trackableDataTrie.go

+	if check.IfNil(tdaw.tr) {
+		newDataTrie, err := mainTrie.Recreate(make([]byte, 0))
+		if err != nil {
+			return map[string][]byte{}, err


please return nil, err here to keep the consistency across the project

iulianpascalau · 2022-09-21T15:21:16Z

state/trackableDataTrie.go

+	}
+
+	tdaw.dirtyData = make(map[string][]byte)
+	return oldValues, nil


we return the old data for jurnalizing purposes, I guess. Can you please check where do we use this?

Yes, this is used only for journalizing. (accountsDB -> saveDataTrie())

ok, will remain as it is

iulianpascalau · 2022-09-21T15:22:29Z

state/trackableDataTrie.go

 		tr:         tr,
 		dirtyData:  make(map[string][]byte),
 		identifier: identifier,
 	}
 }

-// ClearDataCaches empties the dirtyData map and original map
-func (tdaw *TrackableDataTrie) ClearDataCaches() {


this doesn't need to be called anymore due to this refactoring

Correct. It is called inside SaveDirtyData(), but we don't need a method for this.

iulianpascalau · 2022-09-21T15:27:35Z

go.mod

@@ -7,8 +7,8 @@ require (
 	github.com/ElrondNetwork/arwen-wasm-vm/v1_3 v1.3.42-0.20220729115131-85ecca868e90
 	github.com/ElrondNetwork/arwen-wasm-vm/v1_4 v1.4.59-0.20220729115431-a6c93119bdda
 	github.com/ElrondNetwork/covalent-indexer-go v1.0.6
-	github.com/ElrondNetwork/elastic-indexer-go v1.2.41
-	github.com/ElrondNetwork/elrond-go-core v1.1.19
+	github.com/ElrondNetwork/elastic-indexer-go v1.2.42-0.20220921110140-860b4b8c7fc3


do not forget about proper releases

iulianpascalau · 2022-09-21T15:33:59Z

state/accountsDB.go


 	if check.IfNil(adb.dataTries.Get(accountHandler.AddressBytes())) {
-		adb.dataTries.Put(accountHandler.AddressBytes(), accountHandler.DataTrie())
+		trie, ok := accountHandler.DataTrie().(common.Trie)


another cast

will remain as it is

…refactor # Conflicts: # factory/consensus/consensusComponents_test.go # factory/state/stateComponentsHandler_test.go # go.mod

bogdan-rosianu · 2022-09-22T09:14:10Z

node/node.go

@@ -1172,7 +1172,7 @@ func (n *Node) getAccountRootHashAndVal(address []byte, accBytes []byte, key []b
 		return nil, nil, fmt.Errorf("empty dataTrie rootHash")
 	}

-	retrievedVal, err := userAccount.RetrieveValueFromDataTrieTracker(key)
+	retrievedVal, err := userAccount.RetrieveValue(key)


when system-testing this branch, please make sure you test the /address/erd1.../esdt endpoint. it should validate that the affected functionalities (save key value, retrieve value) work the same

It works as expected.

codecov-commenter · 2022-09-22T09:53:31Z

Codecov Report

Base: 71.18% // Head: 71.16% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (01bdd33) compared to base (534f082).
Patch coverage: 70.00% of modified lines in pull request are covered.

Additional details and impacted files

@@              Coverage Diff              @@
##           rc/v1.4.0    #4491      +/-   ##
=============================================
- Coverage      71.18%   71.16%   -0.02%     
=============================================
  Files            662      662              
  Lines          85839    85842       +3     
=============================================
- Hits           61106    61092      -14     
- Misses         20242    20249       +7     
- Partials        4491     4501      +10

Impacted Files	Coverage Δ
process/smartContract/process.go	`65.94% <0.00%> (ø)`
state/accountsDB.go	`77.12% <50.00%> (-1.01%)`	⬇️
state/journalEntries.go	`80.73% <60.00%> (-1.35%)`	⬇️
state/baseAccount.go	`70.37% <63.63%> (-1.06%)`	⬇️
state/trackableDataTrie.go	`85.96% <76.92%> (-9.28%)`	⬇️
epochStart/metachain/baseRewards.go	`94.25% <100.00%> (ø)`
epochStart/metachain/systemSCs.go	`70.48% <100.00%> (ø)`
node/node.go	`73.30% <100.00%> (ø)`
process/rewardTransaction/process.go	`96.55% <100.00%> (ø)`
process/scToProtocol/stakingToPeer.go	`62.44% <100.00%> (ø)`
... and 4 more

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

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

gabi-vuls

System test passed.

iulianpascalau

Just the proper release comment remains

…refactor # Conflicts: # go.mod

gabi-vuls

System test passed.

BeniaminDrasovean added 3 commits September 21, 2022 15:24

refactor dataTrieTracker

33bfb47

Merge remote-tracking branch 'origin/rc/v1.4.0' into dataTrieTracker-…

1166693

…refactor # Conflicts: # common/interface.go # go.mod # go.sum

fix after merge

0bffd13

BeniaminDrasovean marked this pull request as ready for review September 21, 2022 14:04

BeniaminDrasovean self-assigned this Sep 21, 2022

BeniaminDrasovean added area:trie impact:low interested:protocol size:medium type:refactoring complexity:medium ignore-for-release-notes Do not include item in release notes labels Sep 21, 2022

iulianpascalau self-requested a review September 21, 2022 14:26

iulianpascalau reviewed Sep 21, 2022

View reviewed changes

BeniaminDrasovean added 2 commits September 22, 2022 11:54

fix after review

9c87724

Merge remote-tracking branch 'origin/rc/v1.4.0' into dataTrieTracker-…

64b7950

…refactor # Conflicts: # factory/consensus/consensusComponents_test.go # factory/state/stateComponentsHandler_test.go # go.mod

bogdan-rosianu self-requested a review September 22, 2022 09:04

bogdan-rosianu reviewed Sep 22, 2022

View reviewed changes

fix after merge

01bdd33

gabi-vuls previously approved these changes Sep 23, 2022

View reviewed changes

iulianpascalau reviewed Sep 23, 2022

View reviewed changes

BeniaminDrasovean added 2 commits September 26, 2022 10:53

change go mod

805c09d

Merge remote-tracking branch 'origin/rc/v1.4.0' into dataTrieTracker-…

6a2897b

…refactor # Conflicts: # go.mod

BeniaminDrasovean dismissed gabi-vuls’s stale review via 6a2897b September 26, 2022 07:54

BeniaminDrasovean removed the ignore-for-release-notes Do not include item in release notes label Sep 26, 2022

bogdan-rosianu approved these changes Sep 26, 2022

View reviewed changes

iulianpascalau approved these changes Sep 26, 2022

View reviewed changes

gabi-vuls approved these changes Sep 26, 2022

View reviewed changes

BeniaminDrasovean merged commit 5b8eb89 into rc/v1.4.0 Sep 26, 2022

BeniaminDrasovean deleted the dataTrieTracker-refactor branch September 26, 2022 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor dataTrieTracker #4491

refactor dataTrieTracker #4491

BeniaminDrasovean commented Sep 21, 2022 •

edited

Loading

iulianpascalau Sep 21, 2022

BeniaminDrasovean Sep 22, 2022

iulianpascalau Sep 21, 2022

BeniaminDrasovean Sep 22, 2022

iulianpascalau Sep 22, 2022

iulianpascalau Sep 21, 2022

BeniaminDrasovean Sep 22, 2022

iulianpascalau Sep 21, 2022

BeniaminDrasovean Sep 22, 2022

iulianpascalau Sep 22, 2022

iulianpascalau Sep 21, 2022

BeniaminDrasovean Sep 22, 2022

iulianpascalau Sep 21, 2022

BeniaminDrasovean Sep 22, 2022

iulianpascalau Sep 21, 2022

iulianpascalau Sep 22, 2022

bogdan-rosianu Sep 22, 2022

BeniaminDrasovean Sep 26, 2022

codecov-commenter commented Sep 22, 2022

gabi-vuls left a comment

iulianpascalau left a comment

gabi-vuls left a comment

refactor dataTrieTracker #4491

refactor dataTrieTracker #4491

Conversation

BeniaminDrasovean commented Sep 21, 2022 • edited Loading

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

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

codecov-commenter commented Sep 22, 2022

Codecov Report

gabi-vuls left a comment

Choose a reason for hiding this comment

iulianpascalau left a comment

Choose a reason for hiding this comment

gabi-vuls left a comment

Choose a reason for hiding this comment

BeniaminDrasovean commented Sep 21, 2022 •

edited

Loading