Fetch ConsensusParameters from the database
#1809
Merged
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
…ch-consensus-parameters-from-database
bvrooman
reviewed
Apr 6, 2024
|
|
||
| #[derive(Clone, Debug)] | ||
| pub struct SharedState { | ||
| latest_consensus_parameters_version: Arc<SharedMutex<ConsensusParametersVersion>>, |
There was a problem hiding this comment.
SharedMutex is already Arc (crates/services/src/service.rs):
/// Alias for `Arc<T>`
pub type Shared<T> = std::sync::Arc<T>;
/// A mutex that can safely be in async contexts and avoids deadlocks.
#[derive(Debug)]
pub struct SharedMutex<T>(Shared<parking_lot::Mutex<T>>);
Can this just be SharedMutex<ConsensusParametersVersion>?
| @@ -254,6 +250,7 @@ where | |||
|
|
|||
| let acceptance = match checked_tx { | |||
| Ok(tx) => { | |||
| let id = tx.transaction().cached_id().expect("`Checked` tx should have cached id"); | |||
There was a problem hiding this comment.
Is there any reason not to just use ? instead of except here?
There was a problem hiding this comment.
It is the expect because this kind of information should be available now. But because we forgot to expose it, we don't have access right now. I don't want to use ? because it will hide the problem if, at some point, this information is not available.
…ers-from-database' into feature/featch-consensus-parameters-from-database
bvrooman
previously approved these changes
Apr 8, 2024
bvrooman
reviewed
Apr 8, 2024
| pub struct SharedState { | ||
| latest_consensus_parameters_version: SharedMutex<ConsensusParametersVersion>, | ||
| consensus_parameters: | ||
| SharedMutex<HashMap<ConsensusParametersVersion, Arc<ConsensusParameters>>>, |
There was a problem hiding this comment.
I would be interested to understand better if we really need Arc<ConsensusParameters> here instead of just ConsensusParameters:
- If the
SharedState::consensus_parametersis already shared, then the contents ofSharedState::consensus_parameterscan be accessed by cloning theconsensus_parametersArcand locking the mutex - If
SharedStateownsconsensus_parameters, andconsensus_parametersowns the hashmap's value type, we know that the lifetime of the hashmap and its values are maintained bySharedState, which in turn has a lifetime equal to the long running task. We have clear ownership semantics so we probably don't need RC here
Let me know if that sounds possible or if I'm simply misunderstanding anything here.
There was a problem hiding this comment.
We can't return a reference to the ConsensusParameters because it is owned by the MutexGuard, so it has a local lifetime. Plus, if we could, it would create a situation where you can return a reference, lock the HashMap, and remove the value, making the reference invalid.
…ch-consensus-parameters-from-database
…ch-consensus-parameters-from-database
…ch-consensus-parameters-from-database
Dentosal
previously approved these changes
Apr 9, 2024
xgreenx
dismissed stale reviews from Dentosal and bvrooman
The base branch was changed.
…atabase # Conflicts: # CHANGELOG.md # crates/fuel-core/src/service/adapters.rs # crates/fuel-core/src/service/adapters/graphql_api.rs # crates/fuel-core/src/service/adapters/producer.rs # crates/fuel-core/src/service/adapters/txpool.rs # crates/fuel-core/src/service/sub_services.rs # crates/services/producer/src/block_producer.rs # crates/services/producer/src/block_producer/gas_price.rs # crates/services/producer/src/block_producer/tests.rs # crates/services/txpool/src/service.rs
Dentosal
approved these changes
Apr 11, 2024
xgreenx
added a commit
that referenced
this pull request
Apr 18, 2024
## Version v0.25.0 ### Fixed - [#1821](#1821): Can handle missing tables in snapshot. - [#1814](#1814): Bugfix: the `iter_all_by_prefix` was not working for all tables. The change adds a `Rust` level filtering. ### Added - [#1831](#1831): Included the total gas and fee used by transaction into `TransactionStatus`. - [#1821](#1821): Propagate shutdown signal to (re)genesis. Also add progress bar for (re)genesis. - [#1813](#1813): Added back support for `/health` endpoint. - [#1799](#1799): Snapshot creation is now concurrent. - [#1811](#1811): Regenesis now preserves old blocks and transactions for GraphQL API. ### Changed - [#1833](#1833): Regenesis of `SpentMessages` and `ProcessedTransactions`. - [#1830](#1830): Use versioning enum for WASM executor input and output. - [#1816](#1816): Updated the upgradable executor to fetch the state transition bytecode from the database when the version doesn't match a native one. This change enables the WASM executor in the "production" build and requires a `wasm32-unknown-unknown` target. - [#1812](#1812): Follow-up PR to simplify the logic around parallel snapshot creation. - [#1809](#1809): Fetch `ConsensusParameters` from the database - [#1808](#1808): Fetch consensus parameters from the provider. #### Breaking - [#1826](#1826): The changes make the state transition bytecode part of the `ChainConfig`. It guarantees the state transition's availability for the network's first blocks. The change has many minor improvements in different areas related to the state transition bytecode: - The state transition bytecode lies in its own file(`state_transition_bytecode.wasm`) along with the chain config file. The `ChainConfig` loads it automatically when `ChainConfig::load` is called and pushes it back when `ChainConfig::write` is called. - The `fuel-core` release bundle also contains the `fuel-core-wasm-executor.wasm` file of the corresponding executor version. - The regenesis process now considers the last block produced by the previous network. When we create a (re)genesis block of a new network, it has the `height = last_block_of_old_netowkr + 1`. It continues the old network and doesn't overlap blocks(before, we had `old_block.height == new_genesis_block.hegiht`). - Along with the new block height, the regenesis process also increases the state transition bytecode and consensus parameters versions. It guarantees that a new network doesn't use values from the previous network and allows us not to migrate `StateTransitionBytecodeVersions` and `ConsensusParametersVersions` tables. - Added a new CLI argument, `native-executor-version,` that allows overriding of the default version of the native executor. It can be useful for side rollups that have their own history of executor upgrades. - Replaced: ```rust let file = std::fs::File::open(path)?; let mut snapshot: Self = serde_json::from_reader(&file)?; ``` with a: ```rust let mut json = String::new(); std::fs::File::open(&path) .with_context(|| format!("Could not open snapshot file: {path:?}"))? .read_to_string(&mut json)?; let mut snapshot: Self = serde_json::from_str(json.as_str())?; ``` because it is 100 times faster for big JSON files. - Updated all tests to use `Config::local_node_*` instead of working with the `SnapshotReader` directly. It is the preparation of the tests for the futures bumps of the `Executor::VERSION`. When we increase the version, all tests continue to use `GenesisBlock.state_transition_bytecode = 0` while the version is different, which forces the usage of the WASM executor, while for tests, we still prefer to test native execution. The `Config::local_node_*` handles it and forces the executor to use the native version. - Reworked the `build.rs` file of the upgradable executor. The script now caches WASM bytecode to avoid recompilation. Also, fixed the issue with outdated WASM bytecode. The script reacts on any modifications of the `fuel-core-wasm-executor` and forces recompilation (it is why we need the cache), so WASM bytecode always is actual now. - [#1822](#1822): Removed support of `Create` transaction from debugger since it doesn't have any script to execute. - [#1822](#1822): Use `fuel-vm 0.49.0` with new transactions types - `Upgrade` and `Upload`. Also added `max_bytecode_subsections` field to the `ConsensusParameters` to limit the number of bytecode subsections in the state transition bytecode. - [#1816](#1816): Updated the upgradable executor to fetch the state transition bytecode from the database when the version doesn't match a native one. This change enables the WASM executor in the "production" build and requires a `wasm32-unknown-unknown` target. ### Before requesting review - [x] I have reviewed the code myself ### After merging, notify other teams - [x] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [x] [Sway compiler](https://github.com/FuelLabs/sway/) - [x] DevOps ## What's Changed * Add PR template by @Dentosal in #1806 * feat: Parallellize snapshot creation by @segfault-magnet in #1799 * Follow-up PR to simplify the logic around parallel snapshot creation by @xgreenx in #1812 * Bugfix: the `iter_all_by_prefix` was not working for all tables by @xgreenx in #1814 * Added back support for `/health` endpoint by @xgreenx in #1813 * Fetch consensus parameters from the provider by @xgreenx in #1808 * Fetch `ConsensusParameters` from the database by @xgreenx in #1809 * Handle FTI messages in executor by @Voxelot in #1787 * Use state transition bytecode from the database by @xgreenx in #1816 * feat: (re)genesis graceful shutdown by @segfault-magnet in #1821 * Use `fuel-vm 0.49.0` with new transactions types by @xgreenx in #1822 * Included the total gas and fee into `TransactionStatus` by @xgreenx in #1831 * Use versioning enum for WASM executor input and output by @xgreenx in #1830 * Support upgradability of the consensus parameters and state transition bytecode in genesis by @xgreenx in #1826 * Store old blocks and txs after regenesis by @Dentosal in #1811 * Regenesis of `SpentMessages` and `ProcessedTransactions` by @xgreenx in #1833 **Full Changelog**: v0.24.2...v0.25.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.
Related issue #1753
The change fetches the
ConsensusParametersfrom the database and caches it inside of theConsensusParametersProvider. TheConsensusParametersProviderhas a service that runs in the background and listens for a new block from the block importer. If a new block uses a new consensus parameters version, theConsensusParametersProviderwill update the internallatest_consensus_parameters_versionvariable and cache a newConsensusParameters.Before requesting review