Skip to content

Commit

Permalink
fix: Fix commit error on GraphQL service startup (#1802)
Browse files Browse the repository at this point in the history
Related issues:
- Closes #1801

This PR removes the unnecessary DB commit that occurs during startup of
the GraphQL API service. Because the committed transaction has no
corresponding change in block height, the state transition fails and
throws an error. This error then causes startup of the Fuel node to
fail.

This can currently happen in local and production builds, but we did not
have coverage for this in test. This is because this requires loading
the off-chain database using something like
`CombinedDatabase::open(...)`, while most tests in the codebase use a
default off-chain database, i.e. `from_database`:

```
    pub async fn from_database(
        database: Database,
        config: Config,
    ) -> anyhow::Result<Self> {
        let combined_database =
            CombinedDatabase::new(database, Default::default(), Default::default());
        Self::from_combined_database(combined_database, config).await
    }
```

This PR introduces a test that performs the following steps:
1. Creates a node using a RocksDB database on the filesystem for
on-chain, off-chain, and relayer ("combined") data
2. Sends some transactions to the node
3. Shuts down the node
4. Creates a second node by loading the same database on the filesystem
for on-chain, off-chain, and relayer data
5. Waits for the second node to become healthy

Before removing the commit, this test failed with the observed failure:

```
The initialization of the service failed.: error occurred in the underlying datastore `NewHeightIsNotSet { prev_height: 3 }`
```

With the code change, this test passes.

---------

Co-authored-by: xgreenx <xgreenx9999@gmail.com>
  • Loading branch information
Brandon Vrooman and xgreenx authored Apr 4, 2024
1 parent 03195c1 commit c387de9
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 14 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ Description of the upcoming release here.

### Fixed

- [#1802](https://github.com/FuelLabs/fuel-core/pull/1802): Fixed a runtime panic that occurred when restarting a node. The panic was caused by an invalid database commit while loading an existing off-chain database. The invalid commit is removed in this PR.
- [#1803](https://github.com/FuelLabs/fuel-core/pull/1803): Produce block when da height haven't changed.
- [#1795](https://github.com/FuelLabs/fuel-core/pull/1795): Fixed the building of the `fuel-core-wasm-executor` to work outside of the `fuel-core` context. The change uses the path to the manifest file of the `fuel-core-upgradable-executor` to build the `fuel-core-wasm-executor` instead of relying on the workspace.

Expand Down
3 changes: 3 additions & 0 deletions crates/fuel-core/src/graphql_api/ports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,9 @@ pub mod worker {
/// Returns the total count after the update.
fn increase_tx_count(&mut self, new_txs_count: u64) -> StorageResult<u64>;

/// Gets the total number of transactions on the chain from metadata.
fn get_tx_count(&self) -> StorageResult<u64>;

/// Commits the underlying changes into the database.
fn commit(self) -> StorageResult<()>;
}
Expand Down
24 changes: 15 additions & 9 deletions crates/fuel-core/src/graphql_api/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use fuel_core_storage::{
Error as StorageError,
Result as StorageResult,
StorageAsMut,
StorageAsRef,
StorageMutate,
};
use fuel_core_types::{
Expand All @@ -42,6 +43,10 @@ pub mod messages;
pub mod statistic;
pub mod transactions;

/// Tracks the total number of transactions written to the chain
/// It's useful for analyzing TPS or other metrics.
const TX_COUNT: &str = "total_tx_count";

/// GraphQL database tables column ids to the corresponding [`fuel_core_storage::Mappable`] table.
#[repr(u32)]
#[derive(
Expand Down Expand Up @@ -125,22 +130,23 @@ where
}

fn increase_tx_count(&mut self, new_txs_count: u64) -> StorageResult<u64> {
/// Tracks the total number of transactions written to the chain
/// It's useful for analyzing TPS or other metrics.
const TX_COUNT: &str = "total_tx_count";

// TODO: how should tx count be initialized after regenesis?
let current_tx_count: u64 = self
.storage::<StatisticTable<u64>>()
.get(TX_COUNT)?
.unwrap_or_default()
.into_owned();
let current_tx_count: u64 = self.get_tx_count()?;
// Using saturating_add because this value doesn't significantly impact the correctness of execution.
let new_tx_count = current_tx_count.saturating_add(new_txs_count);
<_ as StorageMutate<StatisticTable<u64>>>::insert(self, TX_COUNT, &new_tx_count)?;
Ok(new_tx_count)
}

fn get_tx_count(&self) -> StorageResult<u64> {
let tx_count = self
.storage::<StatisticTable<u64>>()
.get(TX_COUNT)?
.unwrap_or_default()
.into_owned();
Ok(tx_count)
}

fn commit(self) -> StorageResult<()> {
self.commit()?;
Ok(())
Expand Down
9 changes: 5 additions & 4 deletions crates/fuel-core/src/graphql_api/worker_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,10 +323,11 @@ where
_: &StateWatcher,
_: Self::TaskParams,
) -> anyhow::Result<Self::Task> {
let mut db_tx = self.database.transaction();
let total_tx_count = db_tx.increase_tx_count(0).unwrap_or_default();
db_tx.commit()?;
graphql_metrics().total_txs_count.set(total_tx_count as i64);
{
let db_tx = self.database.transaction();
let total_tx_count = db_tx.get_tx_count().unwrap_or_default();
graphql_metrics().total_txs_count.set(total_tx_count as i64);
}

// TODO: It is possible that the node was shut down before we processed all imported blocks.
// It could lead to some missed blocks and the database's inconsistent state.
Expand Down
40 changes: 39 additions & 1 deletion tests/tests/health.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
use fuel_core::{
combined_database::CombinedDatabase,
database::Database,
service::{
Config,
FuelService,
ServiceTrait,
},
types::fuel_tx::Transaction,
};
use fuel_core_client::client::FuelClient;

Expand All @@ -26,7 +29,6 @@ async fn can_restart_node() {

// start node once
{
use fuel_core::service::ServiceTrait;
let database = Database::open_rocksdb(tmp_dir.path(), None).unwrap();
let first_startup = FuelService::from_database(database, Config::local_node())
.await
Expand All @@ -41,3 +43,39 @@ async fn can_restart_node() {
.unwrap();
}
}

#[tokio::test]
async fn can_restart_node_with_transactions() {
let capacity = 1024 * 1024;
let tmp_dir = tempfile::TempDir::new().unwrap();

{
// Given
let database = CombinedDatabase::open(tmp_dir.path(), capacity).unwrap();
let service = FuelService::from_combined_database(database, Config::local_node())
.await
.unwrap();
let client = FuelClient::from(service.bound_address);
client.health().await.unwrap();

for _ in 0..5 {
let tx = Transaction::default_test_tx();
client.submit_and_await_commit(&tx).await.unwrap();
}

service.stop_and_await().await.unwrap();
}

{
// When
let database = CombinedDatabase::open(tmp_dir.path(), capacity).unwrap();
let service = FuelService::from_combined_database(database, Config::local_node())
.await
.unwrap();
let client = FuelClient::from(service.bound_address);

// Then
client.health().await.unwrap();
service.stop_and_await().await.unwrap();
}
}

0 comments on commit c387de9

Please sign in to comment.