Revert tx that migrates data bugfix #5611

BeniaminDrasovean · 2023-09-29T11:57:18Z

Reasoning behind the pull request

There is an edgecase relating to trie pruning + migrate data trie + revert. If a transaction that triggers the migration of a certain data trie value is reverted, the old version of the trie leaf that was migrated is added back to the trie, but the new migrated leaf is not removed. This means that the root hash of the data trie after the revert will not be the same as before.
This bug can be noticed only if pruning is enabled because even though the data trie will reload the old rootHash correctly, it will also mark it for eviction.
Another edgecase which is fixed in this PR: If a trie branch node migrates only one of it's children and then that child is removed/the migration is reverted, the branch node will have an initialized ChildrenVersion slice instead of a nil byte slice. This will result in a different root hash.

Proposed changes

To prevent the eviction of the needed root hash, the data trie needs to have the same root hash after the revert as before. This will mean that the root hash will be present in both oldHashes and newHashes, thus the removal will be canceled.

Testing procedure

Start the chain with snapshots disabled and pruning enabled, and after the autoBalanceDataTriesFlag is enabled, send some txs (which will change the existing data trie leaves) that will fail.
Normal testing procedure

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?

… the trie

iulianpascalau

👍

state/accountsDB_test.go

state/trackableDataTrie/trackableDataTrie.go

codecov · 2023-09-29T12:09:57Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ac5f985) 80.16% compared to head (6addb23) 80.12%.
Report is 1 commits behind head on rc/v1.6.0.

❗ Current head 6addb23 differs from pull request most recent head 89e77e4. Consider uploading reports for the commit 89e77e4 to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##           rc/v1.6.0    #5611      +/-   ##
=============================================
- Coverage      80.16%   80.12%   -0.04%     
=============================================
  Files            708      708              
  Lines          93860    93875      +15     
=============================================
- Hits           75241    75221      -20     
- Misses         13282    13316      +34     
- Partials        5337     5338       +1

Files	Coverage Δ
trie/branchNode.go	`77.94% <100.00%> (+0.29%)`	⬆️
state/trackableDataTrie/trackableDataTrie.go	`83.85% <91.66%> (+0.61%)`	⬆️

... and 5 files with indirect coverage changes

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

…vert

iulianpascalau · 2023-10-02T08:28:35Z

trie/branchNode.go


 	return nil
 }

+func (bn *branchNode) revertChildrenVersionSlice() {
+	for i := range bn.ChildrenVersion {
+		if bn.ChildrenVersion[i] == 1 {


hardcoded version?

AdoAdoAdo · 2023-10-02T09:29:06Z

trie/branchNode.go


 	return nil
 }

+func (bn *branchNode) revertChildrenVersionSlice() {


ca you add unit tests?

AdoAdoAdo · 2023-10-02T09:39:37Z

state/trackableDataTrie/trackableDataTrie.go

 		if err != nil {
 			return nil, err
 		}

 		index++
+
+		isFirstMigration := oldVal.Version == core.NotSpecified && dataEntry.newVersion == core.AutoBalanceEnabled
+		if isFirstMigration && len(newKey) != 0 {


we only have len(newKey) == 0 here in case of deletes right?

iulianpascalau · 2023-10-03T08:32:33Z

trie/branchNode_test.go

@@ -1436,7 +1436,7 @@ func TestBranchNode_VerifyChildrenVersionIsSetCorrectlyAfterInsertAndDelete(t *t
 		}
 		newBn, _, err := bn.insert(data, &testscommon.MemDbMock{})
 		assert.Nil(t, err)
-		assert.Equal(t, 0, len(newBn.(*branchNode).ChildrenVersion))
+		assert.Equal(t, []byte(nil), newBn.(*branchNode).ChildrenVersion)


assert.Nil here and on L1452 & L1492

gabi-vuls

Normal allin test: v1.5.13-dev-config-8c685c8e00 -> reverted-migration-tx-fix-a3cfd08194

--- Specific errors ---

block hash does not match 7771
wrong nonce in block 2861
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0

/------/

--- Statistics ---

Nr. of all ERRORS: 1
Nr. of all WARNS: 216
Nr. of new ERRORS: 1
Nr. of new WARNS: 3
Nr. of PANICS: 0

/------/

if a tx that migrates data is reverted, delete the migrated data from…

f313365

… the trie

BeniaminDrasovean added type:bug Something isn't working area:trie complexity:high impact:medium interested:protocol size:small labels Sep 29, 2023

iulianpascalau self-requested a review September 29, 2023 12:03

iulianpascalau reviewed Sep 29, 2023

View reviewed changes

state/accountsDB_test.go Outdated Show resolved Hide resolved

state/trackableDataTrie/trackableDataTrie.go Outdated Show resolved Hide resolved

BeniaminDrasovean added 2 commits October 2, 2023 11:04

fix diff between nil byte slice and zero bite slice on branch node re…

6e6cc98

…vert

fix ineffasign

5ee9821

AdoAdoAdo self-requested a review October 2, 2023 08:23

iulianpascalau reviewed Oct 2, 2023

View reviewed changes

remove hardcoded version

c6bb936

iulianpascalau previously approved these changes Oct 2, 2023

View reviewed changes

AdoAdoAdo reviewed Oct 2, 2023

View reviewed changes

remove the posibility of having an empty byte slice for ChildrenVersion

73bd258

BeniaminDrasovean dismissed iulianpascalau’s stale review via 73bd258 October 2, 2023 13:28

small refactor

a3cfd08

iulianpascalau previously approved these changes Oct 2, 2023

View reviewed changes

AdoAdoAdo previously approved these changes Oct 3, 2023

View reviewed changes

add unit tests

54431ce

BeniaminDrasovean dismissed stale reviews from AdoAdoAdo and iulianpascalau via 54431ce October 3, 2023 08:26

Merge branch 'rc/v1.6.0' into reverted-migration-tx-fix

8cad9ce

iulianpascalau previously approved these changes Oct 3, 2023

View reviewed changes

small tests fix

eea3a5c

BeniaminDrasovean dismissed iulianpascalau’s stale review via eea3a5c October 3, 2023 08:30

AdoAdoAdo previously approved these changes Oct 3, 2023

View reviewed changes

iulianpascalau reviewed Oct 3, 2023

View reviewed changes

fix after review

6161c06

BeniaminDrasovean dismissed AdoAdoAdo’s stale review via 6161c06 October 3, 2023 08:35

iulianpascalau approved these changes Oct 3, 2023

View reviewed changes

gabi-vuls approved these changes Oct 3, 2023

View reviewed changes

Merge branch 'rc/v1.6.0' into reverted-migration-tx-fix

89e77e4

AdoAdoAdo approved these changes Oct 3, 2023

View reviewed changes

BeniaminDrasovean merged commit b4df8fd into rc/v1.6.0 Oct 3, 2023
6 checks passed

BeniaminDrasovean deleted the reverted-migration-tx-fix branch October 3, 2023 11:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert tx that migrates data bugfix #5611

Revert tx that migrates data bugfix #5611

BeniaminDrasovean commented Sep 29, 2023 •

edited

iulianpascalau left a comment

codecov bot commented Sep 29, 2023 •

edited

iulianpascalau Oct 2, 2023

BeniaminDrasovean Oct 2, 2023

AdoAdoAdo Oct 2, 2023

BeniaminDrasovean Oct 3, 2023

AdoAdoAdo Oct 2, 2023

BeniaminDrasovean Oct 2, 2023

iulianpascalau Oct 3, 2023

BeniaminDrasovean Oct 3, 2023

gabi-vuls left a comment

Revert tx that migrates data bugfix #5611

Revert tx that migrates data bugfix #5611

Conversation

BeniaminDrasovean commented Sep 29, 2023 • edited

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

iulianpascalau left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 29, 2023 • edited

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

gabi-vuls left a comment

Choose a reason for hiding this comment

BeniaminDrasovean commented Sep 29, 2023 •

edited

codecov bot commented Sep 29, 2023 •

edited