DA compression #1609

Dentosal · 2024-01-18T16:12:38Z

Related #1605. VM PR FuelLabs/fuel-vm#670.

This PR adds DA compression crate for Fuel blocks, performed upon block creation. The compressed blocks are stored into the offchain database and can be fetched using the GraphQL API.

Note for reviewers

To keep this reasonably compact, decompression support is not included in this PR, and will be done as a follow-up. As a result, the full data roundtrip testing is not part of this PR. There's no proof here that compression of full blocks is reversible.

TODO

Features

Temporal registry db support
Optimize temporal registry eviction implementation
Implement TxId ↔ TxPointer lookups
Integrate with the block committer (GraphQL interface, probably)

Tests

compressed blocks are available from non-block-producer nodes
e2e test for the full decompression cycle (moved to a follow-up)

Follow-up issues

Sync the node from L1: Synchronize the node from compressed blocks on L1 #2208
Decompression roudntrip tests DA compression: decompression roundtrip tests #2238
Figure out which cache eviction algorithm/behavior is wanted: Select eviction policy for DA compression temporal registry #2231
Figure out if we need to remove the compressed blocks from the db after a while
Merkle roots for fraud proofs DA compression: temporal registry merkle trees #2232

…ecialization

…el-core into dento/da-compression

…type)

Dentosal · 2024-09-25T14:12:08Z

There is one more question that we need to resolve:

When do we want to enable the DA compression? Do we want to enable it starting a specific block, or do we want to start it from the genesis block?

If the first one, when we need to coordinate it with the ecosystem. If the second one is, then we need to come up with a migration solution.

Don't we already post the blocks to L1? Maybe we could call those "V0" comprssed blocks, and allow the decompressor to work on those as well? I'm not sure if they are easily recognizable, though.

Is anyone in the ecosystem depending on the posted blocks? What kind of coordination is needed?

xgreenx · 2024-09-25T16:27:56Z

Don't we already post the blocks to L1? Maybe we could call those "V0" comprssed blocks, and allow the decompressor to work on those as well? I'm not sure if they are easily recognizable, though.

We do. Those blocks don't have versioning.

Is anyone in the ecosystem depending on the posted blocks? What kind of coordination is needed?

Our sentries depend on it=) And anyone in the network who wants to follow the compression. The network should agree starting which block height we are doing compression.

It can be the first blob with a new compression type, but then we have a problem with our cluster(how sentries know what block to start). Plus, we need add support for blobs into our Relayer to track this information=)

I think just setting block height in the ChainConfig will be the simplest option. But we need to discuss it cc @Voxelot

xgreenx · 2024-09-25T16:49:52Z

Cargo.lock

@@ -169,9 +169,9 @@ dependencies = [

 [[package]]


Could you revert changes in this file please?=)

Ping on that one=)

xgreenx · 2024-09-25T16:50:40Z

crates/compression/Cargo.toml

+postcard = { version = "1.0", features = ["use-std"] }
+rand = { workspace = true, optional = true }
+serde = { version = "1.0", features = ["derive"] }
+thiserror = { workspace = true }


thiserror is not no_std compatible, lets not use it=)

Rermoved in c5ac5db

xgreenx · 2024-09-25T17:40:23Z

crates/compression/src/decompress.rs

+        ctx: &DecompressCtx<D>,
+    ) -> Result<Self, DecompressError> {
+        Ok(Transaction::mint(
+            Default::default(), // TODO: what should this we do with this?


At the end of decompression, you can modify the TxPointer with the block height and transaction index.

xgreenx · 2024-09-25T17:41:53Z

crates/compression/src/eviction_policy.rs

+    {
+        // Pick first key not in the set
+        // TODO: use a proper algo, maybe LRU?
+        let mut key = db.read_latest(keyspace)?;


One nanosecond is much faster than one microsecond(in the case of the database).

Plus, if we cache the key, then we don't rely on the database implementation of the db.read_latest and db.write_latest =)

crates/compression/src/tables.rs

xgreenx · 2024-09-25T18:44:24Z

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

+    };
+}
+
+// Arguments here should match the tables! macro from crates/compression/src/tables.rs


It would be better if the implementation of the storage was independent of the tables! macro. It seems the removing of RegistryKeyspace and RegistryKeyspaceValue from implementation will solve that.

It is independent. The comment here just tells the future person modifying the code where to look for values that are used here. But now it's removed in 946a2e8 anyway.

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

With this approach, we have more macro rules, but the logic is split between different modules, and we can see the hierarchy of dependency between modules(instead of everyone depending on everyone). Plus, we don't need the intermediate `CompressCtxKeyspaces` type anymore.

crates/compression/src/compress.rs

xgreenx · 2024-09-26T16:42:01Z

Cargo.lock

@@ -169,9 +169,9 @@ dependencies = [

 [[package]]


Ping on that one=)

xgreenx · 2024-09-26T16:47:12Z

crates/compression/src/compress.rs

+                    if let Some(found) = ctx.db.registry_index_lookup(self)? {
+                        return Ok(found);
+                    }
+
+                    let key = ctx.$ident.cache_evictor.next_key();
+                    let old = ctx.$ident.changes.insert(key, self.clone());
+                    assert!(old.is_none(), "Key collision in registry substitution");


Where do we handle the case when the same type is used several times during one compression? For example transactions has 2 inputs with the same owner.

It seems that we don't handle it and we will hit the assert

We don't handle it here, and allocate two keys in that case. That's not correct and it's fixed in 40a1a3d.

We would not hit the assert anyway, since that requires that the cache evictor would return the same key twice, which will not happen.

xgreenx · 2024-09-26T16:59:54Z

crates/compression/src/compress.rs

+                        registrations.$ident.push((key, value));
+                    }
+                )*
+                Ok(registrations)


What do you think about calling registrations.write_to_registry(&mut db)?; here as well? In this case CompressCtx will update the latest key along with all registrations

crates/compression/src/eviction_policy.rs

xgreenx · 2024-09-26T17:07:57Z

crates/compression/src/lib.rs

+        /// Serialization for compressed transactions is already tested in fuel-vm,
+        /// but the rest of the block de/serialization is be tested here.


While it is true, it doesn't change the fact that we need to test compression and decompression here. In the fuel-tx,, we test those traits for compression, and their deriving works correctly. Here, we need to test that the logic of the compress and decompress contexts is correct. We need to be sure that we collect the right addresses, IDs, and codes and store them in the database. That we assign correct register keys, and can handle cases when we use the same register key during one compression.

Yep. The testing is clearly not sufficient. I've taken this PR back to draft stage until I manage to write those.

We can merge this PR if we do not enable block compression in the fuel-core to simplify the review process; what do you think?

crates/compression/src/ports.rs

xgreenx · 2024-09-26T17:11:04Z

crates/fuel-core/Cargo.toml

@@ -44,6 +45,7 @@ hyper = { workspace = true }
 indicatif = { workspace = true, default-features = true }
 itertools = { workspace = true }
 num_cpus = { version = "1.16.0", optional = true }
+paste = "1"


We use it in several places, I think it will be better to use { workspace = true } isntead

Sure. 1e70c77

We have two more places=)

xgreenx · 2024-09-26T17:11:47Z

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

+                // Remove the overwritten value from index, if any
+                self.db_tx
+                    .storage_as_mut::<[< DaCompressionTemporalRegistryIndex $type >]>()
+                    .remove(&value_in_index)?;
+
+                // Add the new value to the index
+                self.db_tx
+                    .storage_as_mut::<[< DaCompressionTemporalRegistryIndex $type >]>()
+                    .insert(&value_in_index, key)?;


Why do we need to remove it first and insert after? Insert will replace the value automaticly

This was broken in some refactor. Fixed in ee3e844.

) - Minimized the number of tables for metadata and reverse index since we don't need control over this information. - Use explicitly the `VersionedCompressedBlock` type as the input type for compression and decompression. It is up to the `fuel-core` decide how to represent the final compressed block(postcard or something else). - Added initialization of the `next_key`. - Use a more performant codec for each table of the type instead of a Postcard codec. --------- Co-authored-by: Hannes Karppila <2204863+Dentosal@users.noreply.github.com>

Dentosal · 2024-09-27T11:42:06Z

Design notes from a call with @xgreenx:

only one dedicated node handling compression
- if it crashes, it syncs from L1. must not be down over L1 retention time.
- compressor is part of fuel-core, have a flag to enable
- enable for only one sentry
only store data for past two weeks in the temporal registry
- block header timestamp is used for timing
- add a CLI flag for controlling retention
block committercan binary seach to figure which compressed blocks are available
remove smt root field from v0 compression
postpone some testing to a follow-up PR

Add initial work towards DA compression crate

15662c8

Dentosal added the mainnet label Jan 18, 2024

Dentosal self-assigned this Jan 18, 2024

Dentosal added 19 commits January 18, 2024 11:12

Merge branch 'master' into dento/da-compression

f17cc70

Add distilled and compressed header and tx types

3101be6

Add changelog

4a2372f

Attempt nice types interface, that wont work since Rust is missing sp…

941e5e5

…ecialization

Nevermind, type-based madness was possible and actually quite neat

e65062d

Remove malleable fields

f322d0f

WIP

20f7269

WIP

018c088

WIP

75ab4a0

Derive Compact: initial work

7fb55be

WIP: compaction

3e4f0e0

Merge branch 'master' into dento/da-compression

b795cf7

Impl compaction

a910bce

Correctly compact sequence types

e6b1726

WIP: roundtrip and size tests

7fbb4c8

Default key, performance fixes, misc stuff

8ae7407

Combine db traits together

1919aa8

Simplify the derive code

ec4f257

Fix key add bug

08616b3

Dentosal mentioned this pull request Feb 1, 2024

DA compression for fuel-tx types FuelLabs/fuel-vm#670

Merged

Migrate the derive macro and compaction machinery to fuel-vm repo

ecde26c

Dentosal changed the title ~~DA compression as a crate~~ DA compression Feb 1, 2024

Dentosal and others added 5 commits February 12, 2024 19:34

Work towards properly architected (de)compression services

fae559b

Fix compression and it's tests

0ce4650

Merge branch 'dento/da-compression' of https://github.com/FuelLabs/fu…

e1fe97b

…el-core into dento/da-compression

Merge branch 'master' into dento/da-compression

5ecc9e6

Add decompression and a roundtrip test

ff8e753

Dentosal added 3 commits September 25, 2024 15:27

Use PartialBlockHeader in compressed block header (instead of custom …

e036b82

…type)

Use enum for versioning

9a75b4b

Move commented-out height check code to a note in the doc comment

dcc32ba

Dentosal mentioned this pull request Sep 25, 2024

Remove PrepareCtx, making the compression one-pass but complicating e… #2251

Closed

Merge branch 'master' into dento/da-compression

9eba468

xgreenx reviewed Sep 25, 2024

View reviewed changes

Dentosal and others added 8 commits September 26, 2024 03:08

Merge branch 'master' into dento/da-compression

0957584

Use separate db table for each temporal registry keyspace

946a2e8

Remove RegistryKeyspaceValue type

7fe125f

Change infallible TryFrom into From impl

8d5fdf2

cargo sort

137aee2

Remove thiserror dependency (no_std support)

c5ac5db

cargo sort

2a4c190

Make cache evictor keyspace-agnostic

c04b7dc

xgreenx mentioned this pull request Sep 26, 2024

Minor improvements to the compression crate #2254

Merged

xgreenx and others added 4 commits September 26, 2024 15:44

Minor improvements to the compression crate (#2254)

34bfcd9

Reduce DB access in evictor by caching the latest key

eda9e77

Convert panic into an error

331531d

xgreenx force-pushed the dento/da-compression branch from c750bb0 to 331531d Compare September 26, 2024 16:36

xgreenx reviewed Sep 26, 2024

View reviewed changes

Dentosal and others added 3 commits September 27, 2024 06:19

Replace the old index values correctly

ee3e844

Rename EvictorDb methods

b5edb0d

Dentosal marked this pull request as draft September 27, 2024 04:01

Dentosal added 2 commits September 27, 2024 07:09

Check if the change is already in the current compression batch

40a1a3d

Make paste a workspace dependency

1e70c77

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DA compression #1609

DA compression #1609

Dentosal commented Jan 18, 2024 •

edited

Loading

Dentosal commented Sep 25, 2024

xgreenx commented Sep 25, 2024

xgreenx Sep 25, 2024

xgreenx Sep 26, 2024

xgreenx Sep 25, 2024

Dentosal Sep 26, 2024

xgreenx Sep 25, 2024

xgreenx Sep 25, 2024

xgreenx Sep 25, 2024

Dentosal Sep 26, 2024

xgreenx Sep 26, 2024

xgreenx Sep 26, 2024

Dentosal Sep 27, 2024

xgreenx Sep 26, 2024

xgreenx Sep 26, 2024

Dentosal Sep 27, 2024

xgreenx Sep 27, 2024

xgreenx Sep 26, 2024

Dentosal Sep 27, 2024

xgreenx Sep 27, 2024

xgreenx Sep 26, 2024

Dentosal Sep 27, 2024

Dentosal commented Sep 27, 2024 •

edited

Loading

		/// Serialization for compressed transactions is already tested in fuel-vm,
		/// but the rest of the block de/serialization is be tested here.

DA compression #1609

Are you sure you want to change the base?

DA compression #1609

Conversation

Dentosal commented Jan 18, 2024 • edited Loading

Note for reviewers

TODO

Features

Tests

Follow-up issues

Dentosal commented Sep 25, 2024

xgreenx commented Sep 25, 2024

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

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

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 Sep 27, 2024 • edited Loading

Dentosal commented Jan 18, 2024 •

edited

Loading

Dentosal commented Sep 27, 2024 •

edited

Loading