State changes collector #5585

BeniaminDrasovean · 2023-09-18T07:53:49Z

Reasoning behind the pull request

The lite clients will need the state changes delta for each transaction. Currently, the protocol does not collect the state changes triggered by each transaction.

Proposed changes

The accountsDB will collect all state changes in a new struct. When an account is saved, the new stateChangesCollector will save all the changes made to that account. This includes code changes, data trie changes, data trie migration, etc. No extra processing is needed, the new data is already available in the trackableDataTrie
After each tx executed by the txProcessor, get the collected state changes, and reset the stateChangesCollector so that it will be ready for the next tx execution.
Add a new flag in config CollectStateChangesEnabled which is disabled by default.

Testing procedure

Normal testing procedure
Create a separate trie which is modified only by applying the collected state changes. The real trie and the new trie should have the same rootHash after each block.

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-09-18T08:07:06Z

Codecov Report

Attention: Patch coverage is 76.84729% with 47 lines in your changes are missing coverage. Please review.

Project coverage is 80.17%. Comparing base (edcbe7b) to head (5331072).

Files	Patch %	Lines
state/accountsDB.go	78.87%	12 Missing and 3 partials ⚠️
state/trackableDataTrie/trackableDataTrie.go	78.57%	11 Missing and 4 partials ⚠️
factory/state/stateComponents.go	82.60%	2 Missing and 2 partials ⚠️
...ocess/transactionEvaluator/simulationAccountsDB.go	0.00%	4 Missing ⚠️
state/accountsDBApi.go	0.00%	4 Missing ⚠️
state/accountsDBApiWithHistory.go	0.00%	3 Missing ⚠️
update/genesis/import.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                     @@
##           feat/light-client-support    #5585    +/-   ##
===========================================================
  Coverage                      80.17%   80.17%            
===========================================================
  Files                            752      753     +1     
  Lines                          97907    98035   +128     
===========================================================
+ Hits                           78493    78601   +108     
- Misses                         14043    14056    +13     
- Partials                        5371     5378     +7

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

…t-client-support Merge rc/v1.7.0 in feat/light client support

…llector # Conflicts: # epochStart/metachain/systemSCs_test.go # factory/api/apiResolverFactory.go # factory/processing/blockProcessorCreator_test.go # factory/state/stateComponents.go # genesis/process/memoryComponents.go # integrationTests/state/stateTrie/stateTrie_test.go # integrationTests/testInitializer.go # process/transaction/metaProcess.go # state/accountsDB.go # state/accountsDB_test.go # state/errors.go # state/factory/accountsAdapterAPICreator_test.go # state/interface.go # state/storagePruningManager/storagePruningManager_test.go # state/trackableDataTrie/trackableDataTrie.go # state/trackableDataTrie/trackableDataTrie_test.go # testscommon/integrationtests/factory.go # testscommon/state/accountsAdapterStub.go # update/genesis/import.go

…n-feat/light-client-support

…-client-support Merge rc/1.7.0 in feat/light client support

# Conflicts: # cmd/node/config/config.toml # state/accountsDB_test.go # state/errors.go # state/interface.go # state/trackableDataTrie/trackableDataTrie.go

ssd04 · 2024-06-28T09:17:34Z

state/stateChangesCollector.go

+	stateChanges      []StateChangeDTO
+	stateChangesForTx []StateChangesForTx


do we need lock protection for these?

This file was removed in your PR. Will leave like this so it won't generate any more conflicts.

ssd04 · 2024-06-28T09:28:54Z

state/stateChangesCollector_test.go

+		}
+	})
+
+	t.Run("getStateChanges without tx hash", func(t *testing.T) {


add a test case for these 2 scenarios combined?
AddStateChange -> AddTxHashToCollectedStateChanges -> AddStateChange and then GetStateChanges

This file was removed in your PR. Will leave like this so it won't generate any more conflicts.

ssd04 · 2024-06-28T09:34:34Z

state/accountsDB.go

+	adb.stateChangesCollector.AddTxHashToCollectedStateChanges(txHash)
+}
+
+func (adb *AccountsDB) ResetStateChangesCollector() []StateChangesForTx {


maybe rename to FetchAndReset...?

so this function will be used if we want to collect and save state changes?

This func was removed in your PR. Will leave like this so it won't generate any more conflicts.

ssd04 · 2024-06-28T12:52:48Z

state/stateChangesCollector.go

+	if len(scc.stateChanges) > 0 {
+		scc.AddTxHashToCollectedStateChanges([]byte{})


so if GetStateChanges will be called after several AddStateChange without AddTxHashToCollectedStateChanges those state changes will remain with empty tx hash, does it help to reference state changes without an identifier?

This was addressed in your PR.

AdoAdoAdo · 2024-01-30T12:09:50Z

epochStart/metachain/systemSCs_test.go

@@ -913,6 +913,7 @@ func createAccountsDB(
 		StoragePruningManager: spm,
 		AddressConverter:      &testscommon.PubkeyConverterMock{},
 		SnapshotsManager:      disabledState.NewDisabledSnapshotsManager(),
+		StateChangesCollector: state.NewStateChangesCollector(),


maybe use the disabled collector

AdoAdoAdo · 2024-01-30T12:10:59Z

factory/processing/blockProcessorCreator_test.go

@@ -205,6 +205,7 @@ func createAccountAdapter(
 		StoragePruningManager: disabled.NewDisabledStoragePruningManager(),
 		AddressConverter:      &testscommon.PubkeyConverterMock{},
 		SnapshotsManager:      disabledState.NewDisabledSnapshotsManager(),
+		StateChangesCollector: state.NewStateChangesCollector(),


maybe use the disabled collector, and only in the specific test use a real state collector.

AdoAdoAdo · 2024-02-16T12:45:23Z

factory/state/stateComponents.go

@@ -228,6 +235,11 @@ func (scf *stateComponentsFactory) createPeerAdapter(triesContainer common.Tries
 		return nil, err
 	}

+	stateChangesCollector := disabled.NewDisabledStateChangesCollector()


maybe we should have the state changes collected since the genesis if we only use these to export the data and index it.
Then use a flag for when we add the changes to the block.

There can also be a config flag (not enable epoch), if we want it to be exported from the node (through the outport driver) or not.

This was refactored in the PR from Darius

AdoAdoAdo · 2024-02-16T12:49:29Z

process/transactionEvaluator/simulationAccountsDB.go

@@ -172,6 +172,16 @@ func (r *simulationAccountsDB) GetStackDebugFirstEntry() []byte {
 	return nil
 }

+// SetTxHashForLatestStateChanges -
+func (r *simulationAccountsDB) SetTxHashForLatestStateChanges(txHash []byte) {


this will not affect also the collected changes during processing right?

No, it will not affect

AdoAdoAdo · 2024-03-19T14:11:13Z

state/accountsDB.go

+	}
+	adb.stateChangesCollector.AddStateChange(stateChange)
+
+	return err


maybe return nil? as err!=nil was already returned at 273

AdoAdoAdo · 2024-08-26T08:29:25Z

process/transaction/shardProcess.go

@@ -195,6 +195,8 @@ func (txProc *txProcessor) ProcessTransaction(tx *transaction.Transaction) (vmco
 		txProc.pubkeyConv,
 	)

+	defer txProc.accounts.SetTxHashForLatestStateChanges(txHash)


the defer would call this method after return, and I see processing happens before, shouldn't the set happen before the processing?

The idea was to link all the changes triggered by a tx to the txHash after the tx was executed. It can be refactored as you say, but I would do that in the next PR to avoid generating more conflicts with @ssd04's PR

AdoAdoAdo · 2024-08-26T14:00:18Z

state/accountsDB.go

@@ -217,7 +222,8 @@ func (adb *AccountsDB) ImportAccount(account vmcommon.AccountHandler) error {
 	}

 	mainTrie := adb.getMainTrie()
-	return adb.saveAccountToTrie(account, mainTrie)
+	_, err := adb.saveAccountToTrie(account, mainTrie)


what was the purpose of this save?
also does it affect the trie state? if it does then we should collect the data as state change

This is used only on the old hardFork component, which will be removed.

AdoAdoAdo · 2024-08-27T11:04:37Z

state/accountsDBApi.go

+
+// ResetStateChangesCollector returns nil
+func (accountsDB *accountsDBApi) ResetStateChangesCollector() []StateChangesForTx {
+	return nil


this doesn't seem to reset anything but for the setTxHashForLatestStateChanges a txhash seems to be registered.

so is this empty reset ok and should setTxHashForLatestStateChanges do nothing as well?

Changed for consistency. This component uses a disabledStateChangesCollector, so it would not do anything.

AdoAdoAdo · 2024-08-27T11:21:36Z

state/journalEntries.go

@@ -66,7 +66,7 @@ func (jea *journalEntryCode) revertOldCodeEntry() error {
 		return nil
 	}

-	err := saveCodeEntry(jea.oldCodeHash, jea.oldCodeEntry, jea.trie, jea.marshalizer)
+	_, err := saveCodeEntry(jea.oldCodeHash, jea.oldCodeEntry, jea.trie, jea.marshalizer)


I don't think I have seen any treatment for the revert in the state collector, I guess that is on another PR?

Yes, this is treated in the PR from @ssd04

AdoAdoAdo · 2024-08-27T11:23:50Z

state/stateChangesCollector.go

+}
+
+type stateChangesCollector struct {
+	stateChanges      []StateChangeDTO


what are the separate state changes not linked to a txhash for?

I guess they should still be linked to a reason of the change, even if there is no tx for that - e.g rating change

Indeed. This will be done in the next PR

# Conflicts: # testscommon/state/accountsAdapterStub.go

BeniaminDrasovean and others added 7 commits September 8, 2023 17:34

collect state changes

00b54ef

add more unit tests

75dff98

get state changes for each transaction

ed0f9d6

pass stateChangesCollector as argument

a05808d

add flag for collectStateChanges

db454f1

add comments and unit test for stateChangesCollector

01d832b

Merge branch 'rc/v1.6.0' into state-changes-collector

49a8c8a

BeniaminDrasovean requested a review from AdoAdoAdo September 18, 2023 08:12

BeniaminDrasovean changed the base branch from rc/v1.6.0 to feat/light-client-support September 18, 2023 08:20

iulianpascalau and others added 9 commits January 26, 2024 14:26

Merge pull request #5879 from multiversx/merge-rc/v1.7.0-in-feat/ligh…

676a23c

…t-client-support Merge rc/v1.7.0 in feat/light client support

fix after merge

7e6aa45

Merge branch 'feat/light-client-support' into state-changes-collector

820264b

correlate state changes to a tx hash

33b6daf

Merge remote-tracking branch 'origin/rc/v1.7.0' into merge-rc/1.7.0-i…

93c0e5b

…n-feat/light-client-support

Merge pull request #6049 from multiversx/merge-rc/1.7.0-in-feat/light…

edcbe7b

…-client-support Merge rc/1.7.0 in feat/light client support

Merge branch 'feat/light-client-support' into state-changes-collector

858ae75

# Conflicts: # cmd/node/config/config.toml # state/accountsDB_test.go # state/errors.go # state/interface.go # state/trackableDataTrie/trackableDataTrie.go

fix after merge

5331072

ssd04 reviewed Jun 28, 2024

View reviewed changes

ssd04 changed the base branch from feat/light-client-support to feat/state-changes August 26, 2024 07:38

AdoAdoAdo reviewed Aug 27, 2024

View reviewed changes

BeniaminDrasovean added 2 commits September 4, 2024 15:35

fixes after review

9836fde

Merge branch 'feat/state-changes' into state-changes-collector

5bf6ee7

# Conflicts: # testscommon/state/accountsAdapterStub.go

BeniaminDrasovean marked this pull request as ready for review September 4, 2024 13:09

ssd04 approved these changes Sep 4, 2024

View reviewed changes

AdoAdoAdo approved these changes Sep 4, 2024

View reviewed changes

AdoAdoAdo merged commit ccffe6d into feat/state-changes Sep 4, 2024
6 checks passed

AdoAdoAdo deleted the state-changes-collector branch September 4, 2024 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

State changes collector #5585

State changes collector #5585

BeniaminDrasovean commented Sep 18, 2023 •

edited

Loading

codecov bot commented Sep 18, 2023 •

edited

Loading

ssd04 Jun 28, 2024

BeniaminDrasovean Sep 4, 2024

ssd04 Jun 28, 2024

BeniaminDrasovean Sep 4, 2024

ssd04 Jun 28, 2024

ssd04 Jun 28, 2024

BeniaminDrasovean Sep 4, 2024

ssd04 Jun 28, 2024

BeniaminDrasovean Sep 4, 2024

AdoAdoAdo Jan 30, 2024

BeniaminDrasovean Sep 4, 2024

AdoAdoAdo Jan 30, 2024

BeniaminDrasovean Sep 4, 2024

AdoAdoAdo Feb 16, 2024

BeniaminDrasovean Sep 4, 2024

AdoAdoAdo Feb 16, 2024

BeniaminDrasovean Sep 4, 2024

AdoAdoAdo Mar 19, 2024

BeniaminDrasovean Sep 4, 2024

AdoAdoAdo Aug 26, 2024

BeniaminDrasovean Sep 4, 2024

BeniaminDrasovean Sep 4, 2024

AdoAdoAdo Aug 26, 2024

BeniaminDrasovean Sep 4, 2024

AdoAdoAdo Aug 27, 2024

BeniaminDrasovean Sep 4, 2024

AdoAdoAdo Aug 27, 2024

BeniaminDrasovean Sep 4, 2024

AdoAdoAdo Aug 27, 2024

BeniaminDrasovean Sep 4, 2024

		stateChanges []StateChangeDTO
		stateChangesForTx []StateChangesForTx

		if len(scc.stateChanges) > 0 {
		scc.AddTxHashToCollectedStateChanges([]byte{})

State changes collector #5585

State changes collector #5585

Conversation

BeniaminDrasovean commented Sep 18, 2023 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov bot commented Sep 18, 2023 • edited Loading

Codecov Report

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

BeniaminDrasovean commented Sep 18, 2023 •

edited

Loading

codecov bot commented Sep 18, 2023 •

edited

Loading