State changes outport #6429

cristure · 2024-09-02T09:28:19Z

Reasoning behind the pull request

State changes will be also exported through the host driver (and possible other methods)

Proposed changes

Add a new method to the Driver interface that will send the state changes collected.

Testing procedure

Will need to run a local-testnet along with an observer node that will have the configuration for exporting through host driver or other means.

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?

cristure · 2024-09-04T07:10:09Z

There are somethings that need to be addressed. Mainly about coupling stateChangesCollectore in the outportDriver. The other way around is not possible (there is a TODO left, implying the outportDriver should be injected as a dependency in the stateChangesCollector, but doing this will result in a import cycle and therefore the code won't compile. Therefore, I injected the stateChangesCollector in the outportDriver, but that will require some additional changes on the integration between these two.

… into state-changes-outport # Conflicts: # epochStart/metachain/systemSCs_test.go # go.mod # go.sum # process/transaction/shardProcess.go # state/accountsDB.go # state/accountsDBApi.go # state/interface.go

…nto state-changes-outport # Conflicts: # epochStart/metachain/systemSCs_test.go # process/transaction/shardProcess.go # state/accountsDB.go # state/interface.go # state/stateChanges/writeCollector.go

cristure · 2024-09-10T08:09:46Z

Also moved some structs in proto definition files. PR can be seen here.

Moreover, the mx-chain-go-storage had to be updated with the subsequent change in the proto strucutres. PR here

outport/disabled/disabledOutport.go

outport/factory/hostDriverFactory.go

outport/factory/outportFactory.go

outport/mock/driverStub.go

outport/notifier/eventNotifier.go

scripts/testnet/variables.sh

state/disabled/disabledStateChangesCollector.go

state/stateChanges/dataAnalysisCollector.go

state/stateChanges/writeCollector.go

AdoAdoAdo · 2024-09-12T08:37:41Z

go.mod

 	github.com/multiversx/mx-chain-crypto-go v1.2.12
-	github.com/multiversx/mx-chain-es-indexer-go v1.7.5-0.20240807095116-4f2f595e52d9
+	github.com/multiversx/mx-chain-es-indexer-go v1.4.19-0.20240129150813-a772c480d33a


is this indexer downgrade correct? @miiu96

AdoAdoAdo · 2024-09-12T09:00:20Z

outport/process/outportDataProvider.go

@@ -134,6 +139,9 @@ func (odp *outportDataProvider) PrepareOutportSaveBlockData(arg ArgPrepareOutpor
 		return nil, err
 	}

+	stateChanges := odp.stateChangesCollector.GetStateChangesForTxs()
+	odp.stateChangesCollector.Reset()


I would move the reset on the commit block, after the indexing part.
Otherwise, in case there is no client for the outport block, you would not end up in this method and the reset would not be done.

AdoAdoAdo · 2024-09-12T09:02:06Z

state/accountsDB.go

 	baseNewAcc, newAccOk := newAcc.(baseAccountHandler)
 	baseOldAccount, _ := oldAcc.(baseAccountHandler)

 	if !newAccOk {
-		return nil, nil
+		return make([]*stateChange.DataTrieChange, 0), nil


Is this required because of a panic?

Frankly, I don't remember. Reverted to the previous implementation.

@BeniaminDrasovean can you check if this change would be problematic?
do we need to change the test instead?

It's ok both ways.

AdoAdoAdo · 2024-09-12T09:08:52Z

state/accountsDB.go

@@ -918,7 +918,8 @@ func (adb *AccountsDB) commit() ([]byte, error) {
 		return nil, err
 	}

-	adb.stateChangesCollector.Reset()
+	//TODO: discuss the workflow. If reset here, the outport driver won't be able to pick up the changes.
+	//adb.stateChangesCollector.Reset()


I would move the reset in the processor (shardProcessor & metaProcessor) in the CommitBlock, after calling sp.indexBlockIfNeeded(bodyHandler, headerHash, headerHandler, lastBlockHeader) on shardProcessor, and
after calling mp.indexBlock(header, headerHash, body, lastMetaBlock, notarizedHeadersHashes, rewardsTxs) on metaProcessor

don't forget to remove also the commented code after we agree on the workflow.

AdoAdoAdo · 2024-09-12T09:38:12Z

state/stateChanges/writeCollector.go

@@ -136,6 +103,47 @@ func (scc *stateChangesCollector) getStateChangesForTxs() ([]StateChangesForTx,
 	return stateChangesForTxs, nil
 }

+// GetStateChangesForTxs will retrieve the state changes linked with the tx hash.
+func (scc *stateChangesCollector) GetStateChangesForTxs() map[string]*data.StateChanges {
+	scc.stateChangesMut.Lock()


I think a RLock would be sufficient, as you are just reading the state changes not modifying

we are actually appending to the map in case the hash is already present. Can be seen at state/stateChanges/writeCollector.go:130

AdoAdoAdo · 2024-09-12T11:40:20Z

state/accountsDB.go

 	baseNewAcc, newAccOk := newAcc.(baseAccountHandler)
 	baseOldAccount, _ := oldAcc.(baseAccountHandler)

 	if !newAccOk {
-		return nil, nil
+		return make([]*stateChange.DataTrieChange, 0), nil


@BeniaminDrasovean can you check if this change would be problematic?
do we need to change the test instead?

AdoAdoAdo · 2024-09-12T11:41:59Z

state/accountsDB.go

@@ -918,7 +918,8 @@ func (adb *AccountsDB) commit() ([]byte, error) {
 		return nil, err
 	}

-	adb.stateChangesCollector.Reset()
+	//TODO: discuss the workflow. If reset here, the outport driver won't be able to pick up the changes.
+	//adb.stateChangesCollector.Reset()


don't forget to remove also the commented code after we agree on the workflow.

AdoAdoAdo · 2024-09-12T11:49:40Z

state/stateChanges/writeCollector.go

+			},
+			}
+		} else {
+			stateChangesForTxs[txHash].StateChanges = append(sc.StateChanges,


here we should not append to the sc.StateChanges, but to the stateChangesForTxs[txHash].StateChanges
then we can have the RWMutex as well.

can you also add some unit tests for GetStateChangesForTxs ?

sc is assigned that exact value in the if statement above.
if sc, ok := stateChangesForTxs[txHash]; !ok {

but it is very confusing as it is. Will use the same variable name for both.

BeniaminDrasovean · 2024-09-13T07:23:51Z

outport/host/errors.go

@@ -7,3 +7,6 @@ var ErrHostIsClosed = errors.New("server is closed")

 // ErrNilHost signals that a nil host has been provided
 var ErrNilHost = errors.New("nil host provided")
+
+// ErrNilStateChangesCollector signals that a nil state change collector has been provided.
+var ErrNilStateChangesCollector = errors.New("nil state changes collector provided")


This is not used anywhere.

it is used here: state/accountsDB.go:163

BeniaminDrasovean · 2024-09-13T07:43:25Z

state/accountsDB.go

 	baseNewAcc, newAccOk := newAcc.(baseAccountHandler)
 	baseOldAccount, _ := oldAcc.(baseAccountHandler)

 	if !newAccOk {
-		return nil, nil
+		return make([]*stateChange.DataTrieChange, 0), nil


It's ok both ways.

BeniaminDrasovean · 2024-09-13T07:43:59Z

testscommon/state/stateChangesCollectorStub.go

+	"github.com/multiversx/mx-chain-go/state/stateChanges"
+)
+
+type StateChangesCollectorStub struct {


Missing comment here.

AdoAdoAdo · 2024-09-13T10:33:33Z

process/block/metablock_test.go

@@ -919,6 +920,9 @@ func TestMetaProcessor_CommitBlockStorageFailsForHeaderShouldNotReturnError(t *t
 		return &block.Header{}, []byte("hash"), nil
 	}
 	arguments.BlockTracker = blockTrackerMock
+	arguments.StateChangesCollector = &stateMock.StateChangesCollectorStub{
+		ResetCalled: func() {},


I see that the resetCalled is empty, is this any different from the default implementation in StateChangesCollectorStub?

otherwise I think it can be removed here and just have
arguments.StateChangesCollector = &stateMock.StateChangesCollectorStub{}

same for line 1057

AdoAdoAdo · 2024-09-13T10:34:57Z

state/accountsDB.go

-	if check.IfNil(args.StateChangesCollector) {
-		return ErrNilStateChangesCollector
-	}
+	if check.IfNil(args.StateChangesCollector) { return ErrNilStateChangesCollector }


can you reformat this line? (the return on the next line, like in previous checks)

cristure added 2 commits September 2, 2024 11:51

added state changes collector to hostDriver.

0310268

some fixes.

682e417

cristure changed the base branch from state-changes-collector to state-changes-read-operations September 2, 2024 09:30

miiu96 self-requested a review September 4, 2024 11:01

cristure added 6 commits September 5, 2024 15:18

migrated stateChanges to proto structure in mx-chain-core-go.

f6bff48

moved all the state changes in mx-chain-core-go.

a57053e

fixed nil pointer reference.

f8c9ba5

moved collected state changes in outport block.

09802ae

update core-go.

28f38a8

commit go.sum.

f492061

cristure force-pushed the state-changes-outport branch 3 times, most recently from e95122d to f492061 Compare September 9, 2024 08:17

cristure changed the base branch from state-changes-read-operations to feat/state-changes September 9, 2024 08:18

cristure added 9 commits September 9, 2024 11:19

Merge remote-tracking branch 'refs/remotes/origin/feat/state-changes'…

1623567

… into state-changes-outport # Conflicts: # epochStart/metachain/systemSCs_test.go # go.mod # go.sum # process/transaction/shardProcess.go # state/accountsDB.go # state/accountsDBApi.go # state/interface.go

update dependencies.

b4b49d6

Merge remote-tracking branch 'origin/state-changes-read-operations' i…

4869c40

…nto state-changes-outport # Conflicts: # epochStart/metachain/systemSCs_test.go # process/transaction/shardProcess.go # state/accountsDB.go # state/interface.go # state/stateChanges/writeCollector.go

fix after merge.

291f6c0

fix version and imports.

2250354

fix some tests.

62d2326

fix broken import.

c860684

another round of tests fixed.

c482523

fix linter.

30d5dd9

cristure changed the base branch from feat/state-changes to state-changes-read-operations September 10, 2024 08:02

miiu96 reviewed Sep 10, 2024

View reviewed changes

cristure self-assigned this Sep 10, 2024

cristure added 2 commits September 10, 2024 15:42

fixes after review.

9d983f1

fix missing argument.

776d2b1

miiu96 approved these changes Sep 11, 2024

View reviewed changes

AdoAdoAdo self-requested a review September 12, 2024 08:38

AdoAdoAdo reviewed Sep 12, 2024

View reviewed changes

cristure added 3 commits September 12, 2024 13:43

fixes after review.

1f90b5d

revert due to failing test.

e774eea

fix failing test.

494a7b3

AdoAdoAdo reviewed Sep 12, 2024

View reviewed changes

fix more tests.

4b971e2

BeniaminDrasovean reviewed Sep 13, 2024

View reviewed changes

cristure added 5 commits September 13, 2024 11:14

cosmetic changes.

676c5a5

added test for new method and minor fixes.

d892b5e

update mx-chain-es-indexer

8bd428b

revert variables.sh

6a8dc41

improved readability.

0ffca72

AdoAdoAdo reviewed Sep 13, 2024

View reviewed changes

fixes after review.

43e7cb8

AdoAdoAdo approved these changes Sep 17, 2024

View reviewed changes

bumped mx-chain-core-go version.

260b7b4

cristure merged commit bfc6add into state-changes-read-operations Sep 18, 2024
3 checks passed

cristure deleted the state-changes-outport branch September 18, 2024 07:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

State changes outport #6429

State changes outport #6429

cristure commented Sep 2, 2024

cristure commented Sep 4, 2024

cristure commented Sep 10, 2024

AdoAdoAdo Sep 12, 2024

AdoAdoAdo Sep 12, 2024

AdoAdoAdo Sep 12, 2024

cristure Sep 12, 2024 •

edited

Loading

AdoAdoAdo Sep 12, 2024

BeniaminDrasovean Sep 13, 2024

AdoAdoAdo Sep 12, 2024

cristure Sep 12, 2024

AdoAdoAdo Sep 12, 2024

AdoAdoAdo Sep 12, 2024

cristure Sep 12, 2024

AdoAdoAdo Sep 12, 2024

AdoAdoAdo Sep 12, 2024

AdoAdoAdo Sep 12, 2024

AdoAdoAdo Sep 12, 2024

cristure Sep 13, 2024 •

edited

Loading

cristure Sep 13, 2024 •

edited

Loading

BeniaminDrasovean Sep 13, 2024

cristure Sep 13, 2024

BeniaminDrasovean Sep 13, 2024

BeniaminDrasovean Sep 13, 2024

cristure Sep 13, 2024

AdoAdoAdo Sep 13, 2024

cristure Sep 17, 2024

AdoAdoAdo Sep 13, 2024

cristure Sep 17, 2024

State changes outport #6429

State changes outport #6429

Conversation

cristure commented Sep 2, 2024

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

cristure commented Sep 4, 2024

cristure commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristure Sep 12, 2024 • edited Loading

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

cristure Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

cristure Sep 13, 2024 • edited Loading

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

cristure Sep 12, 2024 •

edited

Loading

cristure Sep 13, 2024 •

edited

Loading

cristure Sep 13, 2024 •

edited

Loading