Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store: Write regression tests & more benchmarks/tests #13224

Closed
3 tasks
Tracked by #12986
tac0turtle opened this issue Sep 9, 2022 · 2 comments
Closed
3 tasks
Tracked by #12986

Store: Write regression tests & more benchmarks/tests #13224

tac0turtle opened this issue Sep 9, 2022 · 2 comments
Assignees
Labels

Comments

@tac0turtle
Copy link
Member

tac0turtle commented Sep 9, 2022

  • Write regression tests so that if we migrate to a new tree we know that the previous semantics are met
  • Write more tests for things like concurrency and where we are unsure about how things work (TBD)
  • Add more benchmarks if needed to properly bench and follow changes in the store
@tac0turtle tac0turtle mentioned this issue Sep 9, 2022
22 tasks
@tac0turtle tac0turtle changed the title Write benchmarks and regression tests Store: Write regression tests & more benchmarks/tests Sep 9, 2022
@tac0turtle tac0turtle moved this to 📝 Todo in Cosmos-SDK Sep 9, 2022
@ethanfrey
Copy link
Contributor

ethanfrey commented Sep 12, 2022

A few points I would like to add. Not "regressions" but undefined / non-determinstic behavior I am aware of.

When committing a multi-store, the order of committing each sub-store is random. If you change the write order within an iavl tree, that will change the hash. Now, in theory, each sub-store is it's own iavl store, so random write order between stores shouldn't have an effect, but this can make actually debugging out of order writes with tracestore near impossible and may introduce non-determinism.

Some invariants we should enforce:

  • Given a use case of multiple cache writes (see below)
  • AppHash of substores should be constant (defined in test)
  • AppHash of multistore should be constant (defined in test)
  • Ordering of writes (eg tracestore output) is constant (defined in test, same in every run)

In the current codebase, hash matches for all cases I know of (but not sure it holds with fuzzing). But write order is random between substores, which makes tracing near impossible and is playing with fire with modules that are order-dependent hashes.

Some example scenarios:

All have 3 stores: apple, banana, carrot

  1. Write 2 items (different keys/value) to each of the 3 stores, directly on multi-store
  2. One level cache wrap, write same items to each store, commit to multi-store, should match 1
  3. One level cache wrap, write these values, second cache wrap, write some new value, overwrite some values, commit second cache wrap, delete 1 item in banana and carrot, commit this cache wrap
  4. Total of three levels like (3)
  5. Like 3 but after committing the second level cache wrap, make another second level cache wrap, read 4 keys that exist, overwrite 2, and rollback that cache, before continuing as in (3). Should match 3 (revert and reads should have no effect)

@tac0turtle
Copy link
Member Author

@facundomedica what is the status of this? saw two prs but not sure when we can close this or move forward

@github-project-automation github-project-automation bot moved this from 📝 Todo to 👏 Done in Cosmos-SDK May 15, 2023
@tac0turtle tac0turtle removed this from Cosmos-SDK Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants