chore(state): review usages of `TrieState` #2912

qdm12 · 2022-11-01T10:49:38Z

Changes

Split out snapshotting feature out of the TrieState method
Add TrieState method Snapshot returning a snapshotted trie state
GetRuntimeMetadata and GetRuntimeVersion do not need the trie
Temporary modifiable trie snapshot for each pending transaction (instead of sharing the same trie)
Move snapshotting operations closer to where they are needed
Rework some related error wrappings and tests
Remove impossible nil trie error check

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

Primary Reviewer

@timwu20

dot/core/messages_test.go

dot/core/service.go

timwu20 · 2023-01-21T03:15:10Z

dot/core/messages.go

 	validity, err = rt.ValidateTransaction(externalExt)
 	if err != nil {
 		logger.Debugf("failed to validate transaction: %s", err)
 		return nil, err
 	}
+	rt.SetContextStorage(nil)


why do we have to do this now?

Clearing the storage trie state for that runtime, so that it can be garbage collected. Otherwise the runtime may hold it or some time/forever (especially since we are keeping older runtimes in memory right now). In most cases, these are temporary states that we don't want to keep anyway (such as here).
The point is to reduce/avoid memory leaks, so this aims at setting the state and then clearing it from the runtime structure; all calls get a new trie state anyway

timwu20 · 2023-01-21T03:17:38Z

dot/core/service.go

 		txnValidity, err := rt.ValidateTransaction(externalExt)
+		rt.SetContextStorage(nil)


why do we set it as nil? Shouldn't we restore back to original ts?

timwu20 · 2023-01-21T03:19:00Z

dot/core/service.go

 	}
-	return rt.Version(), nil
+
+	return runtime.Version(), nil


it would be nice to just cache this, so we don't need to execute the Version function every time. Can also help when we don't store every running runtime instance.

It's already cached, see

gossamer/lib/runtime/types.go

Line 59 in 4ea0a70

Version Version

timwu20 · 2023-01-21T03:20:10Z

dot/core/service.go

 	}
-	return rt.Metadata()
+
+	return runtime.Metadata()


same with this.. should we consider caching this?

Yes, created #3065

timwu20 · 2023-01-21T03:21:36Z

dot/state/storage_notify_test.go

@@ -60,6 +61,7 @@ func TestStorageState_RegisterStorageObserver_Multi(t *testing.T) {
 	ss := newTestStorageState(t)
 	ts, err := ss.TrieState(nil)
 	require.NoError(t, err)
+	ts = ts.Snapshot()


why do we need to snapshot if we are overwriting?

Because

Snapshot() was part of TrieState() previously

We don't want to modify the underlying data of the original ts from the storage state. We are overriding the ts variable, but that's just a pointer variable, and Snapshot gives it another pointer.

timwu20 · 2023-01-21T03:22:33Z

dot/state/test_helpers.go

@@ -240,6 +240,8 @@ func generateBlockWithRandomTrie(t *testing.T, serv *Service,
 	trieState, err := serv.Storage.TrieState(nil)
 	require.NoError(t, err)

+	trieState = trieState.Snapshot()


again why do we need to snapshot here?

See #2912 (comment)

timwu20 · 2023-01-21T03:26:21Z

dot/sync/chain_processor.go

 		return fmt.Errorf("failed to execute block %d: %w", block.Header.Number, err)
 	}

-	if err = s.blockImportHandler.HandleBlockImport(block, ts, announceImportedBlock); err != nil {
-		return err
+	rt.SetContextStorage(nil)


could we not just defer this line before line 267

We could, I did that more-explicit-less-deferred to highlight the storage is no longer needed right after rt.ExecuteBlock(block), but I can change it to a defer if you feel it's better.

qdm12 force-pushed the qdm12/state/triestate-review branch 4 times, most recently from ce7b981 to bfa2fb5 Compare November 2, 2022 15:22

qdm12 mentioned this pull request Nov 3, 2022

chore(dot/sync): simplify processBlockData and associated tests #2811

Merged

qdm12 force-pushed the qdm12/state/triestate-review branch from bfa2fb5 to 2fc8827 Compare November 3, 2022 13:39

qdm12 changed the base branch from development to qdm12/dot/sync/processblockdata November 3, 2022 13:39

qdm12 force-pushed the qdm12/state/triestate-review branch 2 times, most recently from 71f5f40 to 97fe789 Compare November 3, 2022 14:28

qdm12 force-pushed the qdm12/dot/sync/processblockdata branch from 015c45d to 294d3ed Compare November 4, 2022 13:15

qdm12 force-pushed the qdm12/state/triestate-review branch 3 times, most recently from 229542a to d1ee86d Compare November 7, 2022 15:19

qdm12 force-pushed the qdm12/dot/sync/processblockdata branch 3 times, most recently from 56ab6ce to e2429de Compare November 9, 2022 13:46

qdm12 force-pushed the qdm12/state/triestate-review branch 2 times, most recently from 0c6a076 to 4cbd713 Compare November 9, 2022 16:22

qdm12 force-pushed the qdm12/dot/sync/processblockdata branch from e2429de to 8c5bac2 Compare November 9, 2022 16:41

qdm12 force-pushed the qdm12/state/triestate-review branch 6 times, most recently from e5c675e to c69f6f0 Compare November 15, 2022 11:07

qdm12 force-pushed the qdm12/dot/sync/processblockdata branch from 8c5bac2 to eab9165 Compare November 15, 2022 11:20

qdm12 force-pushed the qdm12/state/triestate-review branch 2 times, most recently from bba6fbb to ed7a7e5 Compare November 15, 2022 13:08

qdm12 force-pushed the qdm12/dot/sync/processblockdata branch from eab9165 to 2de6f19 Compare November 15, 2022 15:08

qdm12 force-pushed the qdm12/state/triestate-review branch from ed7a7e5 to 4bf0404 Compare November 15, 2022 15:25

qdm12 mentioned this pull request Nov 25, 2022

chore(dot/core): remove trie state calls from GetRuntimeVersion #2769

Closed

qdm12 force-pushed the qdm12/state/triestate-review branch 2 times, most recently from 4710df0 to 1babd11 Compare December 5, 2022 20:20

qdm12 force-pushed the qdm12/state/triestate-review branch from 1babd11 to d2c3dae Compare January 10, 2023 14:39

qdm12 mentioned this pull request Jan 10, 2023

runtime/storage: DeleteChildLimit should delete keys from a child trie transactionally #3032

Closed

EclesioMeloJunior approved these changes Jan 10, 2023

View reviewed changes

dot/core/messages_test.go Outdated Show resolved Hide resolved

dot/core/service.go Outdated Show resolved Hide resolved

qdm12 force-pushed the qdm12/state/triestate-review branch from d2c3dae to 47ed6d1 Compare January 12, 2023 14:17

qdm12 added the Status: Review Needed label Jan 12, 2023

timwu20 reviewed Jan 21, 2023

View reviewed changes

qdm12 force-pushed the qdm12/state/triestate-review branch 2 times, most recently from e50c403 to b438802 Compare January 25, 2023 15:15

qdm12 added 15 commits January 26, 2023 08:51

Remove impossible error check

acd2138

GetRuntimeVersion does not need trie state

ba74a38

GetRuntimeMetadata does not need trie state

ca8a919

TrieState Snapshot method returns a snapshotted trie state

133afd7

Review a few error wrappings

cb11a82

Split out Snapshot out of TrieState

78712c7

Move SetContextStorage calls closer where it's needed

fcb9bdc

Each pending tx uses its own trie state

d812b4a

Clear runtime storage when possible

717c7da

Minor comment addressed

0196a8c

Remove snapshot in processBlockDataWithStateHeaderAndBody

14b7706

Remove some TODOs

6da3d8f

Fix data race in Test_chainProcessor_processReadyBlocks

1bc90f2

Test cases use underscores instead of spaces

189cc20

Polkadotjs is happy now

3555018

qdm12 force-pushed the qdm12/state/triestate-review branch from b438802 to 3555018 Compare January 26, 2023 08:54

qdm12 requested a review from timwu20 January 26, 2023 08:57

dimartiro mentioned this pull request May 30, 2023

feat(trie): Add trie v1 support #3276

Closed

27 tasks

timwu20 closed this Aug 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(state): review usages of `TrieState` #2912

chore(state): review usages of `TrieState` #2912

qdm12 commented Nov 1, 2022 •

edited

Loading

timwu20 Jan 21, 2023

qdm12 Jan 24, 2023 •

edited

Loading

timwu20 Jan 21, 2023

timwu20 Jan 21, 2023

qdm12 Jan 24, 2023

timwu20 Jan 21, 2023

qdm12 Jan 24, 2023

timwu20 Jan 21, 2023

qdm12 Jan 24, 2023

timwu20 Jan 21, 2023

qdm12 Jan 24, 2023

timwu20 Jan 21, 2023

qdm12 Jan 24, 2023 •

edited

Loading

		txnValidity, err := rt.ValidateTransaction(externalExt)
		rt.SetContextStorage(nil)

chore(state): review usages of TrieState #2912

chore(state): review usages of TrieState #2912

Conversation

qdm12 commented Nov 1, 2022 • edited Loading

Changes

Tests

Issues

Primary Reviewer

Choose a reason for hiding this comment

qdm12 Jan 24, 2023 • 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

qdm12 Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

chore(state): review usages of `TrieState` #2912

chore(state): review usages of `TrieState` #2912

qdm12 commented Nov 1, 2022 •

edited

Loading

qdm12 Jan 24, 2023 •

edited

Loading

qdm12 Jan 24, 2023 •

edited

Loading