fix(runtime/storage): support nested storage transactions #3670

EclesioMeloJunior · 2024-01-10T01:08:32Z

Changes

The following host functions can be called multiple times in a row
ext_storage_start_transaction
ext_storage_rollback_transaction
ext_storage_commit_transaction

Basically, these functions can be nested in multiple calls like:

- start_transaction
  - operations ...
  - start_transaction
     - operations ...
     - commit or rollback
  - commit or rollback

This PR aims to implement the following approach:

First, to define a storage transaction:
- A storage transaction contains the current state which is a copy/snapshot of the previous state and it will be modified by the operations under the transaction
- A storage transaction can rollback, in this case we basically discard the current state returning back to the previous and unmodified state
- Commit a storage transaction means the current state will replace the previous state
Gossamer holds a transactions stack of *trie.Trie (state), every time the runtime makes a call to ext_storage_start_transaction Gossamer creates a snapshot of the current state and insert in the transactions stack
The function getCurrentTrie()
- Returns the original trie if no storage transaction has started (the runtime can perform operations to the trie w/o a transaction)
- If a transaction is started then this function returns the snapshot of that transaction, so any operations will be performed in a copy of the original trie that can be commited or discarded.
Rollback method simply pops the top of the stack, discarding all the operations applied to that snapshot
Commit method pops the top of stack and assign it to be the current state of a previous transaction OR be the original trie

Tests

go test -timeout 10m -run ^TestTrieState_NestedTransactions$ github.com/ChainSafe/gossamer/lib/runtime/storage

Issues

dimartiro · 2024-01-10T01:12:23Z

I think it also closes #3508 right?

EclesioMeloJunior · 2024-01-10T13:00:03Z

I think it also closes #3508 right?

Yes! Thanks @dimartiro

) Co-authored-by: Diego Romero <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…1.18.0 (#3663) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

….8 (#3662) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

dimartiro · 2024-01-10T19:12:17Z

@EclesioMeloJunior since there are a lot of test failing, I think will be simpler if we divide this PR into 2, one keeping the original name TrieState but changing the behavior to support transactions and another one to change the name from TrieState to TransactionalTrieState and updating the tests, etc.

What do you think?

EclesioMeloJunior · 2024-01-10T19:38:03Z

@EclesioMeloJunior since there are a lot of test failing, I think will be simpler if we divide this PR into 2, one keeping the original name TrieState but changing the behavior to support transactions and another one to change the name from TrieState to TransactionalTrieState and updating the tests, etc.

What do you think?

I should open as draft PR since I still working on tests. I think we can keep them in a single PR, the renaming makes the files changed quite big but is just renaming which means that it does not require a huge review effort

dimartiro · 2024-01-10T19:42:25Z

@EclesioMeloJunior since there are a lot of test failing, I think will be simpler if we divide this PR into 2, one keeping the original name TrieState but changing the behavior to support transactions and another one to change the name from TrieState to TransactionalTrieState and updating the tests, etc.
What do you think?

I should open as draft PR since I still working on tests. I think we can keep them in a single PR, the renaming makes the files changed quite big but is just renaming which means that it does not require a huge review effort

Yeah I was not talking only about that file but also the other files you have to change due the renaming.
Since it is a fix + a refactor I think we can have it done (code + review) quicker if we divide it into two, one with the fix (the transaction support) and another one with the name refactor. But is only a suggestion 😅

EclesioMeloJunior · 2024-01-10T20:00:28Z

@dimartiro I see your point, I will revert the renaming and then address it in a later point

…r into eclesio/trie-get-panic

…hup` test

codecov · 2024-01-11T13:43:26Z

Codecov Report

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

Comparison is base (f7e690f) 50.20% compared to head (891b2b2) 50.55%.

Additional details and impacted files

@@               Coverage Diff               @@
##           development    #3670      +/-   ##
===============================================
+ Coverage        50.20%   50.55%   +0.34%     
===============================================
  Files              229      229              
  Lines            28590    28598       +8     
===============================================
+ Hits             14354    14457     +103     
+ Misses           12715    12620      -95     
  Partials          1521     1521

dimartiro

Approved with suggestions to make it more straightforward

lib/runtime/storage/trie.go

lib/runtime/interfaces.go

lib/runtime/storage/trie.go

jimjbrettj

Looks good just a few comments. We also do not need .DS_STORE and the testdata files in this do we?

jimjbrettj

great work!

# [0.9.0](v0.8.0...v0.9.0) (2024-3-1) ### Bug Fixes * add a limit of number of bytes while scale decoding a slice ([#3733](#3733)) ([5edbf89](5edbf89)) * **docs:** Fixing link to polkadot runtime fundamentals to the right one ([#3763](#3763)) ([a785d32](a785d32)) * don't panic if we fail to convert hex to bytes ([#3734](#3734)) ([12234de](12234de)) * **dot/sync:** execute p2p handshake when there is no target ([#3695](#3695)) ([a9db0ec](a9db0ec)) * fix index out of range undeterministic error in rpc test ([#3718](#3718)) ([d099384](d099384)) * fix non deterministic panic during TestStableNetworkRPC integration test ([#3756](#3756)) ([ee3d243](ee3d243)) * **lib/trie:** use `MustBeHashed` for V1 trie nodes with larger storage values ([#3739](#3739)) ([f5e48a9](f5e48a9)) * **mocks:** Set fixed version for uber mockgen in CI ([#3656](#3656)) ([ea9877e](ea9877e)) * **runtime/storage:** support nested storage transactions ([#3670](#3670)) ([3e99f6d](3e99f6d)) * segfault on node restart ([#3736](#3736)) ([d1ca7aa](d1ca7aa)) * **state-version:** should be uint8 instead of uint32 ([#3779](#3779)) ([c8fdb14](c8fdb14)) * update paseo chain spec ([#3770](#3770)) ([6a54f28](6a54f28)) * use last finalized block on startup ([#3737](#3737)) ([c262642](c262642)) ### Features * **config:** dynamically set version based on environment ([#3693](#3693)) ([5c534c9](5c534c9)) * **staging:** Expose RPC on Westend Staging Node ([#3687](#3687)) ([c374eaa](c374eaa)) * **tests/scripts:** create script to retrieve trie state via rpc ([#3714](#3714)) ([5ccea40](5ccea40))

github-actions · 2024-03-01T16:44:35Z

🎉 This PR is included in version 0.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Kirill <[email protected]> Co-authored-by: Diego Romero <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

# [0.9.0](v0.8.0...v0.9.0) (2024-3-1) ### Bug Fixes * add a limit of number of bytes while scale decoding a slice ([#3733](#3733)) ([5edbf89](5edbf89)) * **docs:** Fixing link to polkadot runtime fundamentals to the right one ([#3763](#3763)) ([a785d32](a785d32)) * don't panic if we fail to convert hex to bytes ([#3734](#3734)) ([12234de](12234de)) * **dot/sync:** execute p2p handshake when there is no target ([#3695](#3695)) ([a9db0ec](a9db0ec)) * fix index out of range undeterministic error in rpc test ([#3718](#3718)) ([d099384](d099384)) * fix non deterministic panic during TestStableNetworkRPC integration test ([#3756](#3756)) ([ee3d243](ee3d243)) * **lib/trie:** use `MustBeHashed` for V1 trie nodes with larger storage values ([#3739](#3739)) ([f5e48a9](f5e48a9)) * **mocks:** Set fixed version for uber mockgen in CI ([#3656](#3656)) ([ea9877e](ea9877e)) * **runtime/storage:** support nested storage transactions ([#3670](#3670)) ([3e99f6d](3e99f6d)) * segfault on node restart ([#3736](#3736)) ([d1ca7aa](d1ca7aa)) * **state-version:** should be uint8 instead of uint32 ([#3779](#3779)) ([c8fdb14](c8fdb14)) * update paseo chain spec ([#3770](#3770)) ([6a54f28](6a54f28)) * use last finalized block on startup ([#3737](#3737)) ([c262642](c262642)) ### Features * **config:** dynamically set version based on environment ([#3693](#3693)) ([5c534c9](5c534c9)) * **staging:** Expose RPC on Westend Staging Node ([#3687](#3687)) ([c374eaa](c374eaa)) * **tests/scripts:** create script to retrieve trie state via rpc ([#3714](#3714)) ([5ccea40](5ccea40))

EclesioMeloJunior assigned timwu20 and jimjbrettj Jan 10, 2024

EclesioMeloJunior requested review from edwardmack, timwu20, jimjbrettj, kishansagathiya, dimartiro, kanishkatn and axaysagathiya as code owners January 10, 2024 01:08

EclesioMeloJunior changed the title ~~fix: support nested storage transactions~~ fix(runtime/storage): support nested storage transactions Jan 10, 2024

EclesioMeloJunior and others added 6 commits January 10, 2024 09:23

fix: manage transactions correctly (commits and rollbacks)

c125f69

docs(.github/contrib): add commits and PR styleguide to contribmd (#3634

e16da76

) Co-authored-by: Diego Romero <[email protected]>

chore(deps): bump golang.org/x/term from 0.15.0 to 0.16.0 (#3661)

d6a632a

Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

chore(deps): bump github.com/prometheus/client_golang from 1.17.0 to …

4aaeb97

…1.18.0 (#3663) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

chore(deps): bump github.com/spf13/viper from 1.18.1 to 1.18.2 (#3664)

e6260b4

Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

chore(deps): bump github.com/ethereum/go-ethereum from 1.13.5 to 1.13…

3d68312

….8 (#3662) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

EclesioMeloJunior force-pushed the eclesio/trie-get-panic branch from 54faab7 to 3d68312 Compare January 10, 2024 13:25

EclesioMeloJunior added 2 commits January 10, 2024 09:27

chore: remove uneeded diffs

2e81d89

Merge branch 'development' into eclesio/trie-get-panic

3765e3c

EclesioMeloJunior marked this pull request as draft January 10, 2024 19:38

EclesioMeloJunior added 4 commits January 10, 2024 17:25

chore: use TrieState

6932dbd

Merge branch 'eclesio/trie-get-panic' of github.com:ChainSafe/gossame…

a6d3b03

…r into eclesio/trie-get-panic

Merge branch 'development' into eclesio/trie-get-panic

7567bf4

chore: fix lint

02ed484

chore: include tests for nested transactions

38c4501

EclesioMeloJunior force-pushed the eclesio/trie-get-panic branch from 5948c66 to 38c4501 Compare January 11, 2024 12:51

EclesioMeloJunior added 3 commits January 11, 2024 09:07

chore: fix `Test_chainSync_onBlockAnnounceHandshake_tipModeNeedToCatc…

a6a6988

…hup` test

chore: replace NewTrieState(nil) to NewTrieState(tries.EmptyTrie())

38bf119

chore: use trie.EmptyTrie() instead of nil

b4fcea8

dimartiro approved these changes Jan 11, 2024

View reviewed changes

lib/runtime/storage/trie.go Show resolved Hide resolved

lib/runtime/storage/trie.go Show resolved Hide resolved

lib/runtime/storage/trie.go Outdated Show resolved Hide resolved

lib/runtime/storage/trie.go Show resolved Hide resolved

chore: use container/list instead of arrays

7f8bbcb

jimjbrettj reviewed Jan 11, 2024

View reviewed changes

lib/runtime/interfaces.go Outdated Show resolved Hide resolved

jimjbrettj reviewed Jan 11, 2024

View reviewed changes

lib/runtime/storage/trie.go Show resolved Hide resolved

jimjbrettj reviewed Jan 11, 2024

View reviewed changes

lib/runtime/storage/trie.go Show resolved Hide resolved

jimjbrettj reviewed Jan 11, 2024

View reviewed changes

EclesioMeloJunior added 4 commits January 12, 2024 08:18

Delete .DS_Store

fd13f68

Merge branch 'development' into eclesio/trie-get-panic

35bf025

chore: renaming methods

89d8f45

chore: fix panic messag

aff3927

jimjbrettj approved these changes Jan 12, 2024

View reviewed changes

EclesioMeloJunior added 3 commits January 12, 2024 12:13

Merge branch 'development' into eclesio/trie-get-panic

aebb4ed

Merge branch 'development' into eclesio/trie-get-panic

cbb8f0c

Merge branch 'development' into eclesio/trie-get-panic

891b2b2

EclesioMeloJunior merged commit 3e99f6d into development Jan 15, 2024
23 of 24 checks passed

EclesioMeloJunior deleted the eclesio/trie-get-panic branch January 15, 2024 12:58

q9f added the P-high this should be addressed ASAP. label Jan 16, 2024

EclesioMeloJunior added this to the Release v0.9.0 milestone Jan 22, 2024

github-actions bot added the released label Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(runtime/storage): support nested storage transactions #3670

fix(runtime/storage): support nested storage transactions #3670

EclesioMeloJunior commented Jan 10, 2024 •

edited

Loading

dimartiro commented Jan 10, 2024

EclesioMeloJunior commented Jan 10, 2024

dimartiro commented Jan 10, 2024

EclesioMeloJunior commented Jan 10, 2024

dimartiro commented Jan 10, 2024

EclesioMeloJunior commented Jan 10, 2024

codecov bot commented Jan 11, 2024 •

edited

Loading

dimartiro left a comment •

edited

Loading

jimjbrettj left a comment

jimjbrettj left a comment

github-actions bot commented Mar 1, 2024

fix(runtime/storage): support nested storage transactions #3670

fix(runtime/storage): support nested storage transactions #3670

Conversation

EclesioMeloJunior commented Jan 10, 2024 • edited Loading

Changes

Tests

Issues

dimartiro commented Jan 10, 2024

EclesioMeloJunior commented Jan 10, 2024

dimartiro commented Jan 10, 2024

EclesioMeloJunior commented Jan 10, 2024

dimartiro commented Jan 10, 2024

EclesioMeloJunior commented Jan 10, 2024

codecov bot commented Jan 11, 2024 • edited Loading

Codecov Report

dimartiro left a comment • edited Loading

Choose a reason for hiding this comment

jimjbrettj left a comment

Choose a reason for hiding this comment

jimjbrettj left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 1, 2024

EclesioMeloJunior commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 11, 2024 •

edited

Loading

dimartiro left a comment •

edited

Loading