Add decompression traits and a test case #2295

Dentosal · 2024-10-04T18:24:38Z

Add a test case for decompressing DA-compressed blocks. FIx CombinedDb::from_config to respects state_rewind_policy with tmp RocksDB.

netrome

LGTM, one question from me.

crates/fuel-core/src/graphql_api/da_compression.rs

acerone85 · 2024-10-15T18:55:00Z

crates/fuel-core/src/combined_database.rs

+            on_chain: Database::rocksdb_temp(state_rewind_policy)?,
+            off_chain: Database::rocksdb_temp(state_rewind_policy)?,
+            relayer: Database::rocksdb_temp(state_rewind_policy)?,
+            gas_price: Default::default(),


out of curiosity, why the difference for the gas_price database?

@xgreenx might know, I have no idea

Because gas price table and relayer table doesn't need to support state rewind in the case of the temporary database(right now at least). Because we don't have any logic around it.

MitchTurner

WIP. I want to give it another, deeper pass.

MitchTurner · 2024-10-15T18:55:51Z

crates/fuel-core/src/combined_database.rs

@@ -94,6 +94,19 @@ impl CombinedDatabase {
        })
    }

+    /// A test-only temporary rocksdb database with given rewind policy.
+    #[cfg(feature = "rocksdb")]


Should this also be "test-helpers" if it's "test-only"?

Done in 5251318

I reverted this in beaee56 since it seems like we're using it with when empty path is provided, and I'm not sure if we need that outside tests as well.

MitchTurner · 2024-10-15T19:07:47Z

tests/tests/da_compression.rs

+    };
+    let decompressed = decompress(compression_config, db_tx, block).await.unwrap();
+
+    assert!(decompressed.transactions.len() == 2);


Might as well check that the second tx matches the one submitted.

Some fields will differ, as the returned tx would have to be executed to generate full values

tests/tests/da_compression.rs

crates/fuel-core/src/graphql_api/da_compression.rs

MitchTurner · 2024-10-16T06:48:48Z

tests/tests/da_compression.rs

+    let db = &srv.shared.database;
+    let mut tx_inner = db.off_chain().clone().into_transaction();
+    let db_tx = DecompressDbTx {
+        db_tx: DbTx {
+            db_tx: &mut tx_inner,
+        },
+        onchain_db: db.on_chain().view_at(&0u32.into()).unwrap(),
+    };
+    let decompressed = decompress(compression_config, db_tx, block).await.unwrap();


I guess I'm a little confused by using the shared db for decompressing the info. Shouldn't we be able to decompress it with an isolated, clean db?

Maybe that's not the point of this test.

decompression requires some previous indexation to be done on the data, it can't be done from an empty database afaiu

rafal-ch

LGTM 👍

crates/fuel-core/src/graphql_api/da_compression.rs

rymnc · 2024-10-15T16:25:30Z

crates/fuel-core/src/combined_database.rs

+        state_rewind_policy: StateRewindPolicy,
+    ) -> DatabaseResult<Self> {
+        Ok(Self {
+            on_chain: Database::rocksdb_temp(state_rewind_policy)?,


perhaps we should use ShallowTempDir here? we moved it from fuel-core to the benches crate but if the use case is the same we can use it

What's the reason behind this?

because we have a way to clean up the directory after shutdown.. not sure if this method deletes the db after we are done with it.

rymnc · 2024-10-16T08:19:08Z

tests/tests/da_compression.rs

+    let db = &srv.shared.database;
+    let mut tx_inner = db.off_chain().clone().into_transaction();
+    let db_tx = DecompressDbTx {
+        db_tx: DbTx {
+            db_tx: &mut tx_inner,
+        },
+        onchain_db: db.on_chain().view_at(&0u32.into()).unwrap(),
+    };
+    let decompressed = decompress(compression_config, db_tx, block).await.unwrap();


decompression requires some previous indexation to be done on the data, it can't be done from an empty database afaiu

Co-authored-by: Rafał Chabowski <[email protected]>

This PR extends the `fuel-core` too make public some functionality that could be useful to reuse in the watchtower https://github.com/FuelLabs/network-watchtower. ### Before requesting review - [x] I have reviewed the code myself

crates/fuel-core/src/combined_database.rs

xgreenx · 2024-10-29T17:41:14Z

crates/fuel-core/src/combined_database.rs

+            on_chain: Database::rocksdb_temp(state_rewind_policy)?,
+            off_chain: Database::rocksdb_temp(state_rewind_policy)?,
+            relayer: Database::rocksdb_temp(state_rewind_policy)?,
+            gas_price: Default::default(),


Because gas price table and relayer table doesn't need to support state rewind in the case of the temporary database(right now at least). Because we don't have any logic around it.

xgreenx · 2024-10-29T17:45:19Z

tests/tests/da_compression.rs

+        db_tx: DbTx {
+            db_tx: &mut tx_inner,
+        },
+        onchain_db: db.on_chain().view_at(&0u32.into()).unwrap(),


It would be nice if in the test you highlighted somehow that we use the previous state of the database because it is important detail=)

Added a comment to that effect in aca530a

Dentosal self-assigned this Oct 4, 2024

Dentosal requested review from xgreenx and MitchTurner as code owners October 4, 2024 18:24

Dentosal added the no changelog Skip the CI check of the changelog modification label Oct 5, 2024

Base automatically changed from dento/da-compression to master October 5, 2024 17:29

Re-add changes to a clean branch from master (too many conflicts)

72dd40d

Dentosal force-pushed the dento/da-compression-decompress-test branch from fb37322 to 72dd40d Compare October 15, 2024 14:35

Update changelog

bcf1b6c

Dentosal removed the no changelog Skip the CI check of the changelog modification label Oct 15, 2024

Fix benches

cf227f5

Dentosal requested a review from a team October 15, 2024 15:22

netrome previously approved these changes Oct 15, 2024

View reviewed changes

crates/fuel-core/src/graphql_api/da_compression.rs Show resolved Hide resolved

Dentosal requested a review from a team October 15, 2024 18:02

acerone85 reviewed Oct 15, 2024

View reviewed changes

MitchTurner reviewed Oct 15, 2024

View reviewed changes

MitchTurner reviewed Oct 16, 2024

View reviewed changes

rafal-ch previously approved these changes Oct 16, 2024

View reviewed changes

crates/fuel-core/src/graphql_api/da_compression.rs Outdated Show resolved Hide resolved

crates/fuel-core/src/graphql_api/da_compression.rs Outdated Show resolved Hide resolved

rymnc reviewed Oct 16, 2024

View reviewed changes

Remove dbg!

a7324f7

Co-authored-by: Rafał Chabowski <[email protected]>

Dentosal dismissed stale reviews from rafal-ch and netrome via a7324f7 October 17, 2024 10:11

Dentosal and others added 5 commits October 17, 2024 14:05

PR review/cleanup

a4c891e

Move from_state_rewind_policy behind test-helpers

5251318

Revert 5251318

beaee56

Merge branch 'master' into dento/da-compression-decompress-test

f78b718

Merge branch 'master' into dento/da-compression-decompress-test

eadecbe

Dentosal requested review from MitchTurner, acerone85, rymnc and netrome October 24, 2024 11:12

Dentosal requested a review from rafal-ch October 24, 2024 11:12

Merge branch 'master' into dento/da-compression-decompress-test

f8db782

netrome previously approved these changes Oct 25, 2024

View reviewed changes

Dentosal and others added 2 commits October 28, 2024 08:47

Merge branch 'master' into dento/da-compression-decompress-test

75ae124

Extend fuel-core for the watchtower (#2372)

8c6996a

This PR extends the `fuel-core` too make public some functionality that could be useful to reuse in the watchtower https://github.com/FuelLabs/network-watchtower. ### Before requesting review - [x] I have reviewed the code myself

xgreenx dismissed netrome’s stale review via 8c6996a October 29, 2024 17:36

xgreenx reviewed Oct 29, 2024

View reviewed changes

Dentosal added 3 commits November 2, 2024 08:03

Address PR comments

aca530a

Merge branch 'master' into dento/da-compression-decompress-test

43f791c

Fix a typo that the spellchecker complains about

3b1bd25

xgreenx approved these changes Nov 13, 2024

View reviewed changes

Merge branch 'master' into dento/da-compression-decompress-test

c08f660

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add decompression traits and a test case #2295

Add decompression traits and a test case #2295

Dentosal commented Oct 4, 2024 •

edited

Loading

netrome left a comment

acerone85 Oct 15, 2024

Dentosal Oct 17, 2024

xgreenx Oct 29, 2024

MitchTurner left a comment

MitchTurner Oct 15, 2024

Dentosal Oct 17, 2024

Dentosal Oct 17, 2024

MitchTurner Oct 15, 2024

Dentosal Oct 17, 2024

MitchTurner Oct 16, 2024

rymnc Oct 16, 2024

rafal-ch left a comment

rymnc Oct 15, 2024

Dentosal Oct 17, 2024

rymnc Oct 18, 2024

rymnc Oct 16, 2024

xgreenx Oct 29, 2024

xgreenx Oct 29, 2024

Dentosal Nov 2, 2024

Add decompression traits and a test case #2295

Are you sure you want to change the base?

Add decompression traits and a test case #2295

Conversation

Dentosal commented Oct 4, 2024 • edited Loading

netrome left a comment

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

MitchTurner left a comment

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

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

rafal-ch left a comment

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

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

Dentosal commented Oct 4, 2024 •

edited

Loading