-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Implementation of the state rewind feature for the RocksDB #1996
Conversation
…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
…-latest-view # Conflicts: # crates/fuel-core/src/database.rs
…torical-view-implementation # Conflicts: # crates/fuel-core/src/service/genesis/importer/import_task.rs # crates/fuel-core/src/state.rs # crates/fuel-core/src/state/in_memory/memory_store.rs # crates/fuel-core/src/state/rocks_db.rs
…-latest-view # Conflicts: # crates/fuel-core/src/database.rs
…torical-view-implementation
# Conflicts: # crates/fuel-core/src/database.rs
@@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). | |||
## [Unreleased] | |||
|
|||
### Added | |||
- [#1996](https://github.com/FuelLabs/fuel-core/pull/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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this might also have breaking changes, at least some pub
fields were made pub(crate)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they were added since the last release, then it should be fine.
@@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). | |||
## [Unreleased] | |||
|
|||
### Added | |||
- [#1996](https://github.com/FuelLabs/fuel-core/pull/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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they were added since the last release, then it should be fine.
let shutdown_listener = ShutdownListener::spawn(); | ||
let target_block_height = command.target_block_height.into(); | ||
|
||
while !shutdown_listener.is_cancelled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "While not is_cancelled
" is kinda a confusing double negative. Maybe add a helper that is just like
fn is_still_live(&self) -> bool {
!self.is_cancelled()
}
@@ -13,8 +13,8 @@ | |||
# - `cargo install cargo-insta` | |||
# - `npm install prettier prettier-plugin-toml` | |||
|
|||
npx prettier --check "**/Cargo.toml" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we rename ci_checks.sh
since this isn't "checking" anymore?
@@ -34,10 +40,14 @@ impl DatabaseHeight for DaBlockHeight { | |||
fn advance_height(&self) -> Option<Self> { | |||
self.0.checked_add(1).map(Into::into) | |||
} | |||
|
|||
fn rollback_height(&self) -> Option<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. When would we want to use this for DaBlockHeight
? For example, we default the relayer to not have rollback.
state_rewind_policy: StateRewindPolicy, | ||
) -> DatabaseResult<Self> { | ||
let path = db.path().join("history"); | ||
let history = RocksDb::default_open(path, None)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see some risks to this proliferating pattern of creating many distinct rocksdb instances:
- We lose ACID properties, i.e. issues like the offchain and onchain db getting out of sync can now also happen with the historical changes getting out of sync. What protections are in place to safeguard us from a situation where the changes are committed to the historical database but not to the primary database? I'm concerned that we will introduce new complicated edge cases that are hard to resolve if we don't properly encapsulate all the changes into atomic database transactions, leading to brittleness under adverse conditions.
- Resource contention overhead such as too many open files and more complexity in configuring rocksdb cache sizes. For example, should all rocksdb instances have equal cache limits? It seems like to me, we should allocate more cache to the main databases vs the historical databases during regular block production or syncing etc. However, if an RPC node gets lots of heavy traffic for historical views compared to the amount of work it needs to do to sync, then more cache should be allocated for historical changes. It's hard to predict which db instances would need more caching as it will depend on the usecase. If we didn't have separate DB's then rocksdb would automatically make the most of the cache capacity allocated based on whatever usage scenario is thrown at it.
While I understand that the onchain and offchain updates are not atomic as they are driven by separate processes, I'm wondering if we could make the historical changes part of the same database that they are tracking the changes of. Given that the only way to recover from the offchain and onchain db's falling out of sync is to replay historical data, shouldn't we try to make the historical data as rock solid as possible given that it's part of the same CombinedDatabase::commit_changes function which should be atomic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We lose ACID properties, i.e. issues like the offchain and onchain db getting out of sync can now also happen with the historical changes getting out of sync. What protections are in place to safeguard us from a situation where the changes are committed to the historical database but not to the primary database? I'm concerned that we will introduce new complicated edge cases that are hard to resolve if we don't properly encapsulate all the changes into atomic database transactions, leading to brittleness under adverse conditions.
This situation is handled by committing to the historical database first and only afterward to the wrapped database. We can always roll back the history
database to the same height as the wrapped database.
Below an example of how we process this case while committing new block height:
Resource contention overhead such as too many open files and more complexity in configuring rocksdb cache sizes. For example, should all rocksdb instances have equal cache limits? It seems like to me, we should allocate more cache to the main databases vs the historical databases during regular block production or syncing etc. However, if an RPC node gets lots of heavy traffic for historical views compared to the amount of work it needs to do to sync, then more cache should be allocated for historical changes. It's hard to predict which db instances would need more caching as it will depend on the usecase. If we didn't have separate DB's then rocksdb would automatically make the most of the cache capacity allocated based on whatever usage scenario is thrown at it.
I agree with these concerns. We need to improve the configurability of these parameters. I think it should be possible to store historical data in the same database. I've split it into two databases to have better control over the database configuration and be able to prune historical data just by deleting the folder without additional tools.
I could try to use only one database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think merging into one db can be a follow-up? Or should we handle this before landing the feature since it'll be disruptive to the database schema with two back to back big db changes?
&self, | ||
height: &Description::Height, | ||
) -> StorageResult<ViewAtHeight<Description>> { | ||
let latest_view = self.latest_view()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do some kind of consistency check here? There could be a race condition where the latest view is not in sync with the historical database when the snapshots are created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_: &Description::Height, | ||
) -> StorageResult<KeyValueView<Self::Column>> { | ||
// TODO: https://github.com/FuelLabs/fuel-core/issues/1995 | ||
unimplemented!("The historical view is not implemented for `MemoryStore`") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return an error here instead? We should be extra cautious about adding any panics
## 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
Closes #451
Overview
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. The default size of historical/rewind window is 7 days.
Also 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.
Implementation details
The change adds a new
HistoricalRocksDB
type that is the wrapper around regularRocksDB
. This type has inside another RocksDB instance that is used to duplicate all tables plus has one more column to store the reverse modifications at each block height. The reverse modification is the opposite to the operation that was done during transition from block height X to X + 1. The screenshot below should describe the idea:The key of duplicated tables is extended with block height, and the value is the reverse operation to reach the state of entry at the previous height. Having the history of reverse operations, we can iterate back from the latest version of the entry to the previous one.
Using the main property of the RocksDB(sorting keys by default), lookup operations are fast and we don't need to iterate all modifications. It is just enough to find the nearest reverse operation to the target height.
Checklist
Before requesting review
MemoryStore
#1995Database
level #1993