En 6597 state code pruning #1998
Conversation
data/state/journalEntries_test.go
Outdated
| Code: []byte("newCode"), | ||
| NumReferences: 1, | ||
| } | ||
| marsh := &mock.MarshalizerMock{} |
There was a problem hiding this comment.
marsh sounds strange :)
| } | ||
|
|
||
| return &journalEntryCode{ | ||
| codeHash: codeHash, | ||
| updater: updater, | ||
| oldCodeEntry: oldCodeEntry, |
There was a problem hiding this comment.
oldCodeEntry could be nil? If not you should check it above.
There was a problem hiding this comment.
oldCodeEntry could be nil in the following case: If a newCode is set to an account that had no code before, the oldCodeEntry should be nil, in case that a revert occurs.
| if jea.oldCodeEntry.NumReferences == 1 { | ||
| delete(jea.codeForEviction, string(jea.oldCodeHash)) | ||
| } | ||
|
|
There was a problem hiding this comment.
jea.oldCodeEntry.NumReferences-- ?
There was a problem hiding this comment.
No, because oldCodeEntry already has the numReferences set as before the changes. You can see in the accountDB that updateOldCodeEntry() returns unmodifiedOldCodeEntry that is used when creating a new journal entry.
data/state/journalEntries.go
Outdated
| return nil | ||
| } | ||
|
|
||
| if jea.oldCodeEntry.NumReferences == 1 { |
data/state/accountsDB_test.go
Outdated
| @@ -940,3 +945,294 @@ func TestAccountsDB_GetAllLeaves(t *testing.T) { | |||
| assert.True(t, recreateCalled) | |||
| assert.True(t, getAllLeavesCalled) | |||
| } | |||
|
|
|||
| func getTestAccountsDbAndTrie(marsh marshal.Marshalizer, hsh hashing.Hasher) (*state.AccountsDB, data.Trie) { | |||
There was a problem hiding this comment.
marsh sounds strange :)
data/state/accountsDB.go
Outdated
| return oldAccount.GetCodeHash(), nil | ||
| } | ||
|
|
||
| func (adb *AccountsDB) saveCode(accountHandler baseAccountHandler, oldAcc AccountHandler) error { |
There was a problem hiding this comment.
accountHandler to newAcc
| if jea.oldCodeEntry.NumReferences == 1 { | ||
| delete(jea.codeForEviction, string(jea.oldCodeHash)) | ||
| } | ||
|
|
| @@ -1665,3 +1666,146 @@ func testNodeStateCheckpointSnapshotAndPruning( | |||
| assert.NotNil(t, err) | |||
| } | |||
| } | |||
|
|
|||
| func TestContinuouslyAccountCodeChanges(t *testing.T) { | |||
There was a problem hiding this comment.
function too big. make a few smaller ones.
| @@ -20,6 +20,9 @@ type AccountsDB struct { | |||
| marshalizer marshal.Marshalizer | |||
| accountFactory AccountFactory | |||
|
|
|||
| codeForEviction map[string]struct{} | |||
There was a problem hiding this comment.
codeForEviction cannot be used with current state of smart contract process, as delete function, delete smart contract is not called at all.
There was a problem hiding this comment.
If a code is updated and the old version is not used anymore, it needs to be evicted.
| newCodeHash = adb.hasher.Compute(string(newCode)) | ||
| } | ||
|
|
||
| if bytes.Equal(oldCodeHash, newCodeHash) { |
There was a problem hiding this comment.
saveCode should be called only there is a new code. if there is something in old code - delete account / code should be called. If a code is delete the whole account is deleted.
There was a problem hiding this comment.
This also sets the codeHash for the account. If a code is updated, isn't the same account used? In this case, there is something in both oldCode and newCode.
data/state/accountsDB.go
Outdated
| @@ -90,7 +95,7 @@ func (adb *AccountsDB) SaveAccount(account AccountHandler) error { | |||
|
|
|||
| baseAcc, ok := account.(baseAccountHandler) | |||
| if ok { | |||
| err = adb.saveCode(baseAcc) | |||
| err = adb.saveCode(baseAcc, oldAccount) | |||
There was a problem hiding this comment.
make a separate function to saveCode and saveDataTrie where you can handle all the cases.
There was a problem hiding this comment.
make type assertion for oldAccount as well. for save code you need baseAccountHandler only.
| } | ||
|
|
||
| newCode := accountHandler.GetCode() | ||
| newCode := newAcc.GetCode() |
There was a problem hiding this comment.
newAcc is always not nil? If not, add a check above
There was a problem hiding this comment.
newAccount is always not nil. It is checked on L72, when SaveAccount() is called.
When SC code is saved to the trie, now also the number of references to the same code is saved in the same db. If the number of references to a certain code reaches 0, the trie node that contains the code is marked for eviction, and it will be pruned from the db.