Prepare the database to use different types than Database for atomic view
#1991
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…iteratable-view # Conflicts: # crates/fuel-core/src/database.rs # crates/fuel-core/src/service/adapters/producer.rs
# Conflicts: # crates/fuel-core/src/database.rs # crates/fuel-core/src/service/adapters/producer.rs
MitchTurner
reviewed
Jun 27, 2024
There was a problem hiding this comment.
I don't see any big problems. Seems like generally the same structure in terms of architectural hierarchy, which is mostly what I'm concerned with wrt architecture.
I'm not sure I'm a fan of the term IterableView, since I'd expect it to give an iter to consume.
Also had some minor concerns with the interfaces for the different view types. I think there is room for improvement there.
| @@ -45,7 +48,7 @@ impl VerifierAdapter { | |||
| } | |||
| } | |||
|
|
|||
| impl fuel_core_poa::ports::Database for Database { | |||
| impl fuel_core_poa::ports::Database for OnChainIterableView { | |||
| fn block_header(&self, height: &BlockHeight) -> StorageResult<BlockHeader> { | |||
| Ok(self.get_block(height)?.header().clone()) | |||
There was a problem hiding this comment.
I guess I'm confused why we are calling the database we use here Iterable. Since this doesn't really iterate.
There was a problem hiding this comment.
Do you have any ideas about naming IterableView and KeyValueView to highlight their main differences? I only can think about just View and BasicView, but it supper generic name
There was a problem hiding this comment.
Well, IterableStore::iter_store() returns a BoxedIter<KVItem>. So that implies sometimes its purpose is iterating.
BasicView isn't bad and we can depend on IterableStore where we need to with an impl IterableStore for BasicView maybe?
There was a problem hiding this comment.
The idea of BasicView is that it doesn't implement IterableStore. Only View(IterableView) will implement IterableStore.
There was a problem hiding this comment.
I lost the line of conversation=D The IterableView is an extension of the KeyValueView. It is the same as KeyValueView but also allows iteration of the storage.
You don't like the name IterableView. I'm proposing to change it to View and change the name of KeyValueView to BasicView. Where is the third view here?=D
There was a problem hiding this comment.
That was the missing piece for my understanding. I get it now.
In that case, I'm in favor of KeyValueView and IterableKeyValueView. It's verbose but very clear.
MitchTurner
approved these changes
Jun 27, 2024
Merged
xgreenx
added a commit
that referenced
this pull request
Jul 5, 2024
## Version v0.31.0 ### Added - [#2014](#2014): Added a separate thread for the block importer. - [#2013](#2013): Added a separate thread to process P2P database lookups. - [#2004](#2004): Added new CLI argument `continue-services-on-error` to control internal flow of services. - [#2004](#2004): Added handling of incorrect shutdown of the off-chain GraphQL worker by using state rewind feature. - [#2007](#2007): Improved metrics: - Added database metrics per column. - Added statistic about commit time of each database. - Refactored how metrics are registered: Now, we use only one register shared between all metrics. This global register is used to encode all metrics. - [#1996](#1996): Added support for rollback command when state rewind feature is enabled. The command allows the rollback of the state of the blockchain several blocks behind until the end of the historical window. The default historical window it 7 days. - [#1996](#1996): Added support for the state rewind feature. The feature allows the execution of the blocks in the past and the same execution results to be received. Together with forkless upgrades, execution of any block from the past is possible if historical data exist for the target block height. - [#1994](#1994): Added the actual implementation for the `AtomicView::latest_view`. - [#1972](#1972): Implement `AlgorithmUpdater` for `GasPriceService` - [#1948](#1948): Add new `AlgorithmV1` and `AlgorithmUpdaterV1` for the gas price. Include tools for analysis - [#1676](#1676): Added new CLI arguments: - `graphql-max-depth` - `graphql-max-complexity` - `graphql-max-recursive-depth` ### Changed - [#2015](#2015): Small fixes for the database: - Fixed the name for historical columns - Metrics was working incorrectly for historical columns. - Added recommended setting for the RocksDB - The source of recommendation is official documentation https://github.com/facebook/rocksdb/wiki/Setup-Options-and-Basic-Tuning#other-general-options. - Removed repairing since it could corrupt the database if fails - Several users reported about the corrupted state of the database after having a "Too many descriptors" error where in logs, repairing of the database also failed with this error creating a `lost` folder. - [#2010](#2010): Updated the block importer to allow more blocks to be in the queue. It improves synchronization speed and mitigate the impact of other services on synchronization speed. - [#2006](#2006): Process block importer events first under P2P pressure. - [#2002](#2002): Adapted the block producer to react to checked transactions that were using another version of consensus parameters during validation in the TxPool. After an upgrade of the consensus parameters of the network, TxPool could store invalid `Checked` transactions. This change fixes that by tracking the version that was used to validate the transactions. - [#1999](#1999): Minimize the number of panics in the codebase. - [#1990](#1990): Use latest view for mutate GraphQL queries after modification of the node. - [#1992](#1992): Parse multiple relayer contracts, `RELAYER-V2-LISTENING-CONTRACTS` env variable using a `,` delimiter. - [#1980](#1980): Add `Transaction` to relayer 's event filter #### Breaking - [#2012](#2012): Bumped the `fuel-vm` to `0.55.0` release. More about the change [here](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.55.0). - [#2001](#2001): Prevent GraphQL query body to be huge and cause OOM. The default body size is `1MB`. The limit can be changed by the `graphql-request-body-bytes-limit` CLI argument. - [#1991](#1991): Prepare the database to use different types than `Database` for atomic view. - [#1989](#1989): Extract `HistoricalView` trait from the `AtomicView`. - [#1676](#1676): New `fuel-core-client` is incompatible with the old `fuel-core` because of two requested new fields. - [#1676](#1676): Changed default value for `api-request-timeout` to be `30s`. - [#1676](#1676): Now, GraphQL API has complexity and depth limitations on the queries. The default complexity limit is `20000`. It is ~50 blocks per request with transaction IDs and ~2-5 full blocks. ### Fixed - [#2000](#2000): Use correct query name in metrics for aliased queries. ## What's Changed * Generate and publish code coverage reports in the CI by @Dentosal in #1947 * Gas Price Algorithm by @MitchTurner in #1948 * Use companies fork of the `publish-crates` action by @xgreenx in #1986 * Weekly `cargo update` by @github-actions in #1985 * Implement gas price updater for service by @MitchTurner in #1972 * Extract `HistoricalView` trait from the `AtomicView` by @xgreenx in #1989 * Use fresh `ReadView` for mutate queries by @xgreenx in #1990 * Prevent api spam with GQL complexity limits by @Voxelot in #1676 * Enable parsing multiple relayer listening contract addresses from environment variables by @Jurshsmith in #1992 * Prepare the database to use different types than `Database` for atomic view by @xgreenx in #1991 * Added the actual implementation for the `AtomicView::latest_view` by @xgreenx in #1994 * Weekly `cargo update` by @github-actions in #1998 * Minimize the number of panics in the codebase by @xgreenx in #1999 * feat: include Transaction events in topic0 filter for download_logs by @DefiCake in #1980 * Use correct query name for metrics by @xgreenx in #2000 * Prevent GraphQL query body to be huge and cause OOM by @xgreenx in #2001 * Adapted the block producer to react on the outdated transactions from the TxPool by @xgreenx in #2002 * Process block importer events first under P2P pressure by @xgreenx in #2006 * Implementation of the state rewind feature for the RocksDB by @xgreenx in #1996 * Upgraded `fuel-vm` to `0.55.0` by @xgreenx in #2012 * Improved metrics for the database by @xgreenx in #2007 * Updated block importer to allow more blocks to be queue by @xgreenx in #2010 * Added handling of incorrect shutdown of the off-chain GraphQL worker by @xgreenx in #2004 * Moved P2P database lookups into a separate thread by @xgreenx in #2013 * Use dedicated thread for the block importer by @xgreenx in #2014 * Small fixes for the database by @xgreenx in #2015 ## New Contributors * @Jurshsmith made their first contribution in #1992 * @DefiCake made their first contribution in #1980 **Full Changelog**: v0.30.0...v0.31.0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change overview
The change implements the types for
AtomicViewandHistoricalViewtraits. It is only preparation for corresponding features without actual implementation.In the past, we cloned the
Databasetype and used it as a view without any atomic properties. In a new implementation,Databaseis only a view provider now, but the view itself implements all read functionality.We have two views. Separation for
IterableViewandKeyValueViewis needed because we can't iterate over the historical storage. We only have access by key to the values. It would be nice to only have one view that supports all functionality, but we don't need iteration functionality for the historical view(at least for now), and it is expensive to support(requires more storage or enormous computation resources).Implementation details
Database,IterableViewandKeyValueViewall are aliases to theGenericDatabase. TheGenericDatabaseimplements all storage traits from thefuel-storagecrate, deriving implementation fromStructuredStorage. We need it reuse the storage trait implementation for all three types. They only require the inner storage to implementKeyValueInspectto derive implementation.KeyValueViewWrapperandIterableViewWrapperare wrappers aroundArc<dyn Trait>that implementKeyValueInspecttraits.Checklist
Before requesting review