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

Problem: there's no compact historical state storage #722

Closed
wants to merge 17 commits into from

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Sep 27, 2022

Closes: #704
Solution:

  • Integration version store and streaming service.

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

  • Have you read the CONTRIBUTING.md?
  • Does your PR follow the C4 patch requirements?
  • Have you rebased your work on top of the latest master?
  • Have you checked your code compiles? (make)
  • Have you included tests for any non-trivial functionality?
  • Have you checked your code passes the unit tests? (make test)
  • Have you checked your code formatting is correct? (go fmt)
  • Have you checked your basic code style is fine? (golangci-lint run)
  • If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
  • If your changes affect the client infrastructure, have you run the integration test?
  • If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
  • If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
  • If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

app/app.go Fixed Show fixed Hide fixed
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 14 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 14 potential problems in the proposed changes. Check the Files changed tab for more details.

app/app.go Fixed Show fixed Hide fixed
versiondb/streaming_service.go Fixed Show fixed Hide fixed
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 14 potential problems in the proposed changes. Check the Files changed tab for more details.

@codecov
Copy link

codecov bot commented Sep 27, 2022

Codecov Report

Merging #722 (f46daef) into main (be8d53d) will increase coverage by 0.59%.
The diff coverage is 35.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #722      +/-   ##
==========================================
+ Coverage   33.99%   34.59%   +0.59%     
==========================================
  Files          28       37       +9     
  Lines        1506     2246     +740     
==========================================
+ Hits          512      777     +265     
- Misses        941     1388     +447     
- Partials       53       81      +28     
Impacted Files Coverage Δ
versiondb/backend_test_utils.go 0.00% <0.00%> (ø)
versiondb/multistore.go 0.00% <0.00%> (ø)
versiondb/store.go 0.00% <0.00%> (ø)
versiondb/streaming_service.go 0.00% <0.00%> (ø)
versiondb/utils.go 0.00% <0.00%> (ø)
versiondb/tmdb/history.go 58.13% <58.13%> (ø)
versiondb/tmdb/store.go 68.00% <68.00%> (ø)
versiondb/tmdb/iterator.go 88.00% <88.00%> (ø)
versiondb/sync.go 91.66% <91.66%> (ø)

if err := historyBatch.WriteSync(); err != nil {
return err
}
return plainBatch.WriteSync()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will we have dirty data if one WriteSync fails?

Copy link
Collaborator Author

@yihuang yihuang Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It happens in end blocker, so it's before the chain state commit, if any error happens here, the block is not committed, when restart, the block will be replayed and the versiondb is written again for the same block.
The writes here should be idempotent, so it should be fine I think.

It'll be in an inconsistency state before the block is replayed though. Any ideas how to check the inconsistency? maybe write a block number into the plain db.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh if eventually consistent, maybe just only allow query after block get replayed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can write a block number into plain db, that'd be the latest height, and we don't support query height higher than that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https:/crypto-org-chain/cronos/pull/722/files#diff-aa83981ed9bef3f7410fa0d5177ca4934c6e4b3c7280205511237da9b3dc02abR92
you've updated the latest version, so only if plainBatch.WriteSync() succeeded and it will be updated. Right?

Copy link
Collaborator Author

@yihuang yihuang Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, a batch write is atomic.

app/app.go Outdated Show resolved Hide resolved
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 13 potential problems in the proposed changes. Check the Files changed tab for more details.

@yihuang
Copy link
Collaborator Author

yihuang commented Sep 28, 2022

Some numbers get from local testing with rocksdb:

Space Amplification
(file size / data size)
Data Size1 File Size2 KV Pairs3 (manual compaction)
Space Amplification (file size)
Iavl 0.77 3368379952 2578108675 27994442 0.73 (2465786706)
VersionDB 0.84 566304124 473096919 5792002 0.79 (449149073)
- change set 0.82 536711835 438836315 5591300 0.8 (430133115)
- history index 0.54 20596897 11088590 100353 0.54 (11135326)
- plain db 2.58 8995392 23172014 100349 0.88 (7880632)

The ratio of versiondb/iavl file size reduction is: 0.18

Footnotes

  1. Sum of sizes of all key value pairs

  2. Sum of sizes of db files.

  3. Number of key value pairs.

@yihuang
Copy link
Collaborator Author

yihuang commented Sep 29, 2022

Application.db File Size KV Pairs Data Size Space Amplification
Rocksdb, archive 4.9G 31938977 3.7G 1.3
Rocksdb, prune=everything 438M 1481079 182.8M 2.4
MDBX, archive 6.5G 1.8
MDBX, prune=everything 735M 4
VersionDB File Size KV Pairs Data Size Space Amplification
Rocksdb
- Change set 153.5M 2082966 190.6M 0.8
- History index 39M 300456 30.9M 1.26
- Plain state 27.4M 300452 25.6M 1.07
MDBX
- Change set 218M 1.14
- History index 72M 2.4
- Plain state 62M 2.4

Just run some more tests to compare mdbx and rocksdb in terms of db size.
Here we don't use the fancy features of mdbx like dupsort, just use the normal get/set/cursor apis to fit into the tm-db interface.
So in terms of db size, rocksdb is not bad, it's kind of expected since mdbx don't do compression at all, and we don't use dupsort to compress the key prefixes. It's possible to optimize for mdbx in versiondb implementation though.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gosec found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

if latest == height {
return s.plainDB.Get(rawKey)
}

Copy link
Collaborator

@JayT106 JayT106 Oct 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe return nil or error here if height > latest ? what's the expected behavior in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= Sounds good

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a check, so it'll fail if height is in the future.

return s.plainDB.Get(rawKey)
}

found, err := SeekHistoryIndex(s.historyDB, rawKey, uint64(height))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any chance found will be equal to 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think no, it only finds height larger than target.

Comment on lines +38 to +40
for k, v := range s.transientStores {
stores[k] = v
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Comment on lines +83 to +80
for _, key := range keys {
s.transientStores[key] = transient.NewStore()
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Comment on lines +89 to +86
for _, key := range keys {
s.transientStores[key] = mem.NewStore()
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
versiondb/multistore.go Outdated Show resolved Hide resolved
Closes: crypto-org-chain#704
Solution:
- Integration version store and streaming service.

multiple backends

db size benchmark

support mdbx backend through tm-db

store latest version

support GetLatestVersion

query multistore

test versiondb streamer

fix lint

fix state listener

temp

fix state listening

optimize common case

fix lint

try to fix mdbx build

update cosmos-sdk

fix clone append

add test case

fix subkey problem in history index

revert chunking, hard to work with variable length key

support iterator

check future height

fix lint

new state listener

fix latest state in query

fix integration test

fix prune node test

update dependency

add utility to read from file streamer

Update versiondb/multistore.go

Signed-off-by: yihuang <[email protected]>

add unit test

create common backend test cases

update dependency

update with new file streamer format

Problem: python3.10 is not used in integration tests

Solution:
- start using python3.10, prepare to later PRs which need the new features
- update nixpkgs nesserary for the nix stuff to work.

python-roaring64

remove debug log

add test cases, improve coverage
Comment on lines +384 to +386
for _, storeKey := range keys {
exposeStoreKeys = append(exposeStoreKeys, storeKey)
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
@yihuang yihuang marked this pull request as ready for review December 7, 2022 08:45
@yihuang yihuang requested a review from a team as a code owner December 7, 2022 08:45
@yihuang yihuang requested review from mmsqe and leejw51crypto and removed request for a team December 7, 2022 08:45
changesetBatch := s.changesetDB.NewBatch()
defer changesetBatch.Close()

for _, pair := range changeSet {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will we have big changeSet, which need allocate capacity for key for reduce?

integration_tests/test_benchmark_storage.py Outdated Show resolved Hide resolved
@@ -5,6 +5,7 @@ config {
'cmd-flags': '--unsafe-experimental',
'app-config'+: {
'minimum-gas-prices': '100000000000basetcro',
store:: super.store,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might need rm store for these two as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only need this for upgrade test, because it's not supported in previous version.

"github.com/RoaringBitmap/roaring/roaring64"
)

var ChunkLimit = uint64(1950) // threshold beyond which MDBX overflow pages appear: 4096 / 2 - (keySize + 8)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we also keep this chunk?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@yihuang yihuang marked this pull request as draft December 14, 2022 03:05
@yihuang
Copy link
Collaborator Author

yihuang commented Dec 27, 2022

there's a better approach to use user-timestamp feature of rocksdb v7: #791

@yihuang yihuang closed this Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem: there's no compact historical state storage
3 participants