[store] Introduce Flat store adapter #12123

shreyan-gupta · 2024-09-21T00:06:35Z

This is the first concept PR of having adapters on top of store. Most of the details for how it works can be found in core/store/src/adapter/mod.rs

The functions in core/store/src/adapter/flat_store.rs are moved from store_helper file.

codecov · 2024-09-22T11:10:41Z

Codecov Report

Attention: Patch coverage is 78.42742% with 107 lines in your changes missing coverage. Please review.

Project coverage is 71.59%. Comparing base (59814e3) to head (0ebf4df).
Report is 19 commits behind head on master.

Files with missing lines	Patch %	Lines
core/store/src/adapter/flat_store.rs	79.56%	25 Missing and 22 partials ⚠️
tools/flat-storage/src/commands.rs	0.00%	19 Missing ⚠️
core/store/src/flat/storage.rs	89.89%	1 Missing and 9 partials ⚠️
tools/database/src/state_perf.rs	0.00%	7 Missing ⚠️
core/store/src/adapter/mod.rs	72.72%	6 Missing ⚠️
core/store/src/trie/from_flat.rs	0.00%	3 Missing ⚠️
core/store/src/flat/manager.rs	83.33%	0 Missing and 2 partials ⚠️
tools/database/src/corrupt.rs	0.00%	2 Missing ⚠️
tools/fork-network/src/cli.rs	0.00%	2 Missing ⚠️
...s/fork-network/src/single_shard_storage_mutator.rs	0.00%	2 Missing ⚠️
... and 7 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12123      +/-   ##
==========================================
+ Coverage   71.51%   71.59%   +0.07%     
==========================================
  Files         820      822       +2     
  Lines      165037   165176     +139     
  Branches   165037   165176     +139     
==========================================
+ Hits       118034   118261     +227     
+ Misses      41864    41782      -82     
+ Partials     5139     5133       -6

Flag	Coverage Δ
backward-compatibility	`0.17% <0.00%> (+<0.01%)`	⬆️
db-migration	`0.17% <0.00%> (+<0.01%)`	⬆️
genesis-check	`1.26% <0.00%> (+<0.01%)`	⬆️
integration-tests	`38.73% <52.82%> (-0.03%)`	⬇️
linux	`71.36% <78.42%> (+0.05%)`	⬆️
linux-nightly	`71.13% <78.22%> (+0.03%)`	⬆️
macos	`54.08% <66.94%> (+0.11%)`	⬆️
pytests	`1.52% <0.00%> (+<0.01%)`	⬆️
sanity-checks	`1.32% <0.00%> (+<0.01%)`	⬆️
unittests	`65.32% <67.87%> (+0.09%)`	⬆️
upgradability	`0.21% <0.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Trisfald · 2024-09-23T16:58:45Z

core/store/src/adapter/mod.rs

+    }
+}
+
+pub trait StoreAdapter {


I like the fact store_helpers is gone 👍

IMO there's something off with the new API

StoreAdapter having a method flat_store() and at the same time introducing aFlatStoreAdapter implementation of StoreAdapter sounds confusing. It feels like the separation is not at the right place.

Should we take a step back and reason about the core idea?

Do we want to hide "flat storage", "memtries", etc behind an easy to use interface or do we want them to be concrete implementation of a more general "store"? Right now I see an implementation for the former with some sprinkles of the latter.

Aah right, that can definitely sound confusing but the idea is to have inter convertibility among all the adapter types. For example if I have an epoch_store_adapter, flat_store_adapter, trie_store_adapter, chain_store_adapter.... I should be able to call xxx_store_adapter.flat_store() to get an instance of flat_store_adapter

Flat storage and memtries are to remain as they are right now, this store abstraction is at a much much lower level, right above rocks db.

(fyi flat_store may sound confusing, I didn't have a better name, but it doesn't refer to FlatStorage, but just a wrapper on top of store)

ahah I see thanks!

On this point do you already know where should flat_store be used vs epoch_store or trie_store..? Does it simply depend on the type of columns to read/write?
I'm wondering if rather than an true adapter suite we are looking for a wrapper around Store (doesn't have to be a trait)?

Umm yeah, there are high level patterns in our codebase (with LOT of exceptions). It should be possible to categorize the store and store update interface needs based on where it's used, like the trie_store, epoch_store etc. etc. that I mentioned. Yes, it does depend on the location/module where it's used and the set of columns it needs to access.

This is pretty much like a wrapper around Store :)
I wasn't in favor of using traits as with real structs, we have the option to store tiny pieces of data that maybe beneficial, say like a cache.
It may explicitly be useful for our resharding case where we need to map the child shard_uid to parent shard_uid. This can seamlessly be done within the trie_store and trie_store_update structs with just one additional store read! @staffik for context

Sorry, to be clear, while StoreAdapter is definitely a trait that provides us the inter-convertibility between different types of adapters, the explicit adapters themselves, like FlatStoreAdapter are structs (they could have as well been traits)

@Trisfald Shreyan and I went through a couple of alternative designs and unfortunately given all the issues with these alternatives it seems the one used in the PR is still relatively the best. I'm a Rust perfectionist and I don't like how this is designed, but practicality is more important I guess.

This reverts commit 39bac2c.

tayfunelmas · 2024-09-25T11:40:06Z

How does the composing the updates work across adapters and base traits? For example, finalizing a block will generate a large transaction with updates to different parts of the DB. Some of these updates will be for flat store (using FlatStoreAdapter) and some will be for state etc. One issue I encountered was that some components use StoreUpdate and some use ChainStoreUpdate to update state and it is not possible to merge the updates into a single transactions. Do you touch this aspect here?

shreyan-gupta · 2024-09-25T18:03:49Z

How does the composing the updates work across adapters and base traits? For example, finalizing a block will generate a large transaction with updates to different parts of the DB. Some of these updates will be for flat store (using FlatStoreAdapter) and some will be for state etc. One issue I encountered was that some components use StoreUpdate and some use ChainStoreUpdate to update state and it is not possible to merge the updates into a single transactions. Do you touch this aspect here?

While I don't specifically know about ChainStoreUpdate and how that's structured, high level, yes, once we have a store_update object, it's simple to convert it to a mutable reference of any type of update adapter, i.e. store_update.flat_store_update() returns &mut FlatStoreUpdateAdapter.

If you take a look at StoreUpdateHolder in the PR, that's specifically to tackle the issue of merging and committing changes. For merging and committing, we need an owned instance of store_update, as opposed to pushing in new set of changes. But all these are just inner implementation details to get the most seamless experience possible. Can definitely take this up offline in case it's not clear how it works.

robin-near · 2024-09-26T16:56:12Z

core/store/src/adapter/mod.rs

+
+// Static instances of StoreUpdateHolder always hold an owned StoreUpdate instance.
+// In such case it should be possible to convert it to StoreUpdate.
+impl Into<StoreUpdate> for StoreUpdateHolder<'static> {


robin-near · 2024-09-26T18:18:39Z

core/store/src/adapter/mod.rs

+    }
+}
+
+pub trait StoreAdapter {


@Trisfald Shreyan and I went through a couple of alternative designs and unfortunately given all the issues with these alternatives it seems the one used in the PR is still relatively the best. I'm a Rust perfectionist and I don't like how this is designed, but practicality is more important I guess.

Follow up on #12123 This PR replaces `ReadOnlyChunksStore` with `ChunkStoreAdapter`.

PR in the series to move to store adapter. Follow up on #12123

shreyan-gupta added 6 commits September 20, 2024 12:44

[cleanup] Remove flat storage inline migration handler

0500671

fix

963b4fb

[store] Flat store adapter

3c8d69d

new design

e22652c

more changes

27f0e1b

more changes

8522a39

Trisfald reviewed Sep 23, 2024

View reviewed changes

shreyan-gupta added 6 commits September 23, 2024 10:48

return StoreUpdate from flat_store.store_update()

39bac2c

fmt

c4bfb45

Revert "return StoreUpdate from flat_store.store_update()"

ff74156

This reverts commit 39bac2c.

explicitly call into() for store_update merge

654b47a

remove StoreUpdateCommit trait, tmi

ed3147d

some comments

88a4bb7

shreyan-gupta changed the title ~~[store] Flat store adapter~~ [store] Introduce Flat store adapter Sep 23, 2024

Merge remote-tracking branch 'origin' into shreyan/store/flat_store

85430f6

shreyan-gupta marked this pull request as ready for review September 24, 2024 23:47

shreyan-gupta requested a review from a team as a code owner September 24, 2024 23:47

shreyan-gupta requested review from akhi3030, tayfunelmas, pugachAG, Longarithm and robin-near September 24, 2024 23:47

fmt

0ebf4df

shreyan-gupta mentioned this pull request Sep 25, 2024

[store] Introduce Trie store adapter #12146

Merged

robin-near approved these changes Sep 26, 2024

View reviewed changes

shreyan-gupta added this pull request to the merge queue Sep 26, 2024

Merged via the queue into master with commit c9def16 Sep 26, 2024
29 of 30 checks passed

shreyan-gupta deleted the shreyan/store/flat_store branch September 26, 2024 18:43

shreyan-gupta mentioned this pull request Sep 26, 2024

[store] Introduce Chunk store adapter #12158

Merged

github-merge-queue bot pushed a commit that referenced this pull request Sep 26, 2024

[store] Introduce Chunk store adapter (#12158)

0e58ed7

Follow up on #12123 This PR replaces `ReadOnlyChunksStore` with `ChunkStoreAdapter`.

github-merge-queue bot pushed a commit that referenced this pull request Sep 26, 2024

[store] Introduce Trie store adapter (#12146)

e5aa208

PR in the series to move to store adapter. Follow up on #12123

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[store] Introduce Flat store adapter #12123

[store] Introduce Flat store adapter #12123

shreyan-gupta commented Sep 21, 2024 •

edited

Loading

codecov bot commented Sep 22, 2024 •

edited

Loading

Trisfald Sep 23, 2024

shreyan-gupta Sep 23, 2024

shreyan-gupta Sep 23, 2024

shreyan-gupta Sep 23, 2024

Trisfald Sep 23, 2024 •

edited

Loading

shreyan-gupta Sep 23, 2024 •

edited

Loading

shreyan-gupta Sep 23, 2024 •

edited

Loading

robin-near Sep 26, 2024

tayfunelmas commented Sep 25, 2024

shreyan-gupta commented Sep 25, 2024

robin-near Sep 26, 2024

shreyan-gupta Sep 26, 2024

robin-near Sep 26, 2024

[store] Introduce Flat store adapter #12123

[store] Introduce Flat store adapter #12123

Conversation

shreyan-gupta commented Sep 21, 2024 • edited Loading

codecov bot commented Sep 22, 2024 • edited Loading

Codecov Report

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

Trisfald Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

shreyan-gupta Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

shreyan-gupta Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tayfunelmas commented Sep 25, 2024

shreyan-gupta commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyan-gupta commented Sep 21, 2024 •

edited

Loading

codecov bot commented Sep 22, 2024 •

edited

Loading

Trisfald Sep 23, 2024 •

edited

Loading

shreyan-gupta Sep 23, 2024 •

edited

Loading

shreyan-gupta Sep 23, 2024 •

edited

Loading