feat(mater): add unixfs wrapping support #690

pete-eiger · 2025-01-21T17:35:39Z

PR: Add UnixFS Wrapping Support

Description

This PR adds the ability for users to decide whether to wrap file contents in raw blocks or UnixFS DAG nodes when converting to a CARv2. Specifically:

WrapMode enum (Raw or UnixFS) is introduced.
CLI updated: A --wrap option allows the user to choose how data is packaged in the CAR.
Balanced‐tree builder: A new stream_balanced_tree_unixfs function creates proper UnixFS leaves and parent nodes, ensuring the DAG is navigable by retrieval clients.
Raw flow preserved: If --wrap=raw is chosen, it uses stream_balanced_tree with raw leaves.

All relevant tests now pass, including a new check verifying that the UnixFS mode indeed produces the expected number of leaf blocks (matching the total chunk count) and parent nodes.

Important points for reviewers

UnixFS DAG creation: Handled by stream_balanced_tree_unixfs, which encodes each chunk as a DAG‐PB File node.
Raw mode: Leaves remain raw blocks, handled by the original stream_balanced_tree.
CLI: A new ConvertConfig uses WrapMode to drive the logic in create_filestore.
Tests: The test test_filestore_unixfs_dag_structure confirms a correct UnixFS DAG with equal numbers of chunks and leaf blocks.

Checklist

Important points for reviewers?
The code merges neatly with existing balanced‐tree logic. Tests confirm that both raw and UnixFS modes work properly.
Description of the change?
Yes, see above.
Follow‐ups?
None; this PR completes the UnixFS wrapping feature.
Has it been tested?
Yes, new tests pass locally (test_filestore_unixfs_dag_structure).
Any alternative approaches considered?
A manual parent node builder was considered, but leveraging the existing balanced‐tree approach is cleaner.
Documented new APIs?
The WrapMode enum is documented and the CLI’s --wrap flag is explained in the help text.

…nixfs-dag

aidan46 · 2025-01-22T07:56:53Z

mater/lib/src/stores/mod.rs

+/// Determines how content is encoded in the CAR file.
+#[derive(clap::ValueEnum, Clone, Debug)]
+pub enum WrapMode {
+    /// Store content directly in the CAR file without additional metadata.
+    /// This is the most space-efficient option but provides minimal metadata.
+    Raw,
+    /// Wrap content in UnixFS format which includes file metadata and DAG structure.
+    /// This is compatible with IPFS and provides richer metadata about the content.
+    UnixFS,
+}
+
+impl Default for WrapMode {
+    fn default() -> Self {
+        Self::Raw
+    }
+}


Defaults for unit-type enums can be derived too:

Suggested change

/// Determines how content is encoded in the CAR file.

#[derive(clap::ValueEnum, Clone, Debug)]

pub enum WrapMode {

/// Store content directly in the CAR file without additional metadata.

/// This is the most space-efficient option but provides minimal metadata.

Raw,

/// Wrap content in UnixFS format which includes file metadata and DAG structure.

/// This is compatible with IPFS and provides richer metadata about the content.

UnixFS,

}

impl Default for WrapMode {

fn default() -> Self {

Self::Raw

}

}

/// Determines how content is encoded in the CAR file.

#[derive(clap::ValueEnum, Clone, Debug, Default)]

pub enum WrapMode {

/// Store content directly in the CAR file without additional metadata.

/// This is the most space-efficient option but provides minimal metadata.

#[default]

Raw,

/// Wrap content in UnixFS format which includes file metadata and DAG structure.

/// This is compatible with IPFS and provides richer metadata about the content.

UnixFS,

}

There's no other wrapping. Having an enum here is overengineering

True. Also lets have a wrapped mode by default. If the user wants the raw they should provide a flag.

aidan46 · 2025-01-22T08:14:11Z

mater/lib/src/v2/reader.rs

-            written += output_file.write(&contents).await?;
+
+            if let Ok((cid, contents)) = self.read_block().await {
+                if contents.len() == 0 {


I remember when implementing this that this break condition cause the output file to miss some data. Did you check that the output file contains all the data?

cernicc

Good job on figuring this out 💪 I've added some comments and questions. The tests are currently not compiling.

Heads up. If you want to rerun the ci you have to retag the pr with the ready for review. The ci doesn't run automatically on each commit.

cernicc · 2025-01-22T10:15:48Z

maat/tests/real_world.rs

@@ -1,4 +1,4 @@
-use std::collections::BTreeSet;
+use std::{collections::BTreeSet, time::Duration};


Konrad is currently working on fixing the maat. Do you think we should remove the changes so that he won't have conflicts when merging? The thing is that there are more changes needed to fix the tests. There are not only compile time problems.

cernicc · 2025-01-22T10:19:07Z

mater/lib/src/stores/mod.rs

+/// Determines how content is encoded in the CAR file.
+#[derive(clap::ValueEnum, Clone, Debug)]
+pub enum WrapMode {
+    /// Store content directly in the CAR file without additional metadata.
+    /// This is the most space-efficient option but provides minimal metadata.
+    Raw,
+    /// Wrap content in UnixFS format which includes file metadata and DAG structure.
+    /// This is compatible with IPFS and provides richer metadata about the content.
+    UnixFS,
+}
+
+impl Default for WrapMode {
+    fn default() -> Self {
+        Self::Raw
+    }
+}


True. Also lets have a wrapped mode by default. If the user wants the raw they should provide a flag.

cernicc · 2025-01-22T10:21:38Z

mater/lib/src/stores/mod.rs

        }
    }
 }

 impl Default for Config {
    fn default() -> Self {
        Self::Balanced {
-            chunk_size: DEFAULT_BLOCK_SIZE,
-            tree_width: DEFAULT_TREE_WIDTH,
+            chunk_size: 256 * 1024,  // 256KiB


What was the reason of removing the DEFAULT_BLOCK_SIZE and DEFAULT_TREE_WIDTH? I just want to see if there is something to learn here :D

cernicc · 2025-01-22T10:22:45Z

mater/lib/src/stores/mod.rs

-            tree_width: DEFAULT_TREE_WIDTH,
+            chunk_size: 256 * 1024,  // 256KiB
+            tree_width: 174,         // Default from ipfs
+            wrap_mode: WrapMode::Raw // Default to raw like go-car


go-car creates a wrapped car by default

cernicc · 2025-01-22T10:25:02Z

mater/cli/src/convert.rs


+    let mater_config = match config.wrap_mode {
+        WrapMode::Raw => Config::default(),
+        WrapMode::UnixFS => Config::Balanced {


How were the chunk_size and tree_width here determined?

cernicc · 2025-01-22T10:48:20Z

mater/lib/src/stores/filestore.rs

-            } // otherwise, the buffer is not full, so we don't do a thing
+            }
+            // If we reach EOF but still have a partial chunk, yield it
+            if read_bytes == 0 && buf.len() > 0 {


Was this a bug?

cernicc · 2025-01-22T10:54:01Z

mater/lib/src/stores/filestore.rs

-        entries.push(entry);
-        position += writer.write_block(&node_cid, &node_bytes).await?;
+
+        if let Some(existing_position) = written_blocks.get(&node_cid) {


This change was needed because of deduplication? Was that a bug?

cernicc · 2025-01-22T11:08:21Z

mater/lib/Cargo.toml

@@ -29,6 +29,7 @@ thiserror.workspace = true
 tokio = { workspace = true, features = ["fs", "macros", "rt-multi-thread"] }
 tokio-stream.workspace = true
 tokio-util = { workspace = true, features = ["io"] }
+clap = { workspace = true, features = ["derive"] }


I think we should hide the clap behind a feature flag. This was one of the reasons why we split the cli and lib.

cernicc · 2025-01-22T11:21:55Z

mater/lib/src/v2/reader.rs

@@ -21,7 +26,7 @@ impl<R> Reader<R> {

 impl<R> Reader<R>
 where
-    R: AsyncRead + Unpin,
+    R: AsyncRead + Unpin + AsyncSeek,


Why is Seek needed?

cernicc · 2025-01-22T11:28:10Z

storage-provider/server/src/storage.rs

-            .route("/upload/:cid", put(upload))
+            .route(
+                "/upload/:cid",
+                put(upload as fn(State<Arc<StorageServerState>>, Path<String>, Request<Body>) -> _)


Why is this change needed?

jmg-duarte

Good work. The PR description would be better if it explained how the new code achieves the functionality since there's a lack of comments on the code for that specific purpose

jmg-duarte · 2025-01-22T09:45:54Z

mater/lib/Cargo.toml

@@ -29,6 +29,7 @@ thiserror.workspace = true
 tokio = { workspace = true, features = ["fs", "macros", "rt-multi-thread"] }
 tokio-stream.workspace = true
 tokio-util = { workspace = true, features = ["io"] }
+clap = { workspace = true, features = ["derive"] }


This must be removed. The mater crate has nothing to do with the CLI

jmg-duarte · 2025-01-22T09:47:43Z

mater/cli/src/main.rs

+        #[arg(long, value_enum, default_value = "raw")]
+        wrap: WrapMode,


It's either wrapped or not. This is a bool

jmg-duarte · 2025-01-22T09:49:04Z

maat/tests/real_world.rs

How is this file related to mater?

jmg-duarte · 2025-01-22T09:49:22Z

pallets/proofs/src/porep/mod.rs

How is this file related to mater?

jmg-duarte · 2025-01-22T09:51:29Z

mater/lib/src/stores/mod.rs

+            chunk_size: 256 * 1024,  // 256KiB
+            tree_width: 174,         // Default from ipfs


1 - const
2 - why are you changing the existing value?

jmg-duarte · 2025-01-22T10:36:14Z

mater/lib/src/unixfs/mod.rs

+                let total_file_size: u64 = children.iter().map(|c| c.1.raw_data_length).sum();
+                let total_encoded: u64 = children.iter().map(|c| c.1.encoded_data_length).sum();
+                let blocksizes: Vec<_> = children.iter().map(|c| c.1.raw_data_length).collect();


You're performing 3 loops when you could do just 1

jmg-duarte · 2025-01-22T10:37:25Z

mater/lib/src/unixfs/mod.rs

+                };
+                let encoded = DagPbCodec::encode_to_vec(&pb_node)?;
+                let mh = generate_multihash::<Sha256,_>(&encoded);
+                let cid = Cid::new_v1(DAG_PB_CODE, mh);


jmg-duarte · 2025-01-22T12:05:13Z

mater/lib/src/unixfs/mod.rs

+                let pb_links = children.into_iter().map(|(child_cid, link_info)| {
+                    PbLink {
+                        cid: child_cid,
+                        name: Some("".to_string()),
+                        size: Some(link_info.encoded_data_length),
+                    }
+                }).collect::<Vec<_>>();


You can also add this to the previous loop issue

jmg-duarte · 2025-01-22T12:08:04Z

mater/lib/src/stores/mod.rs

@@ -1,10 +1,9 @@
 mod blockstore;
 mod file;
-mod filestore;
+pub mod filestore;


Why are you exposing the whole module?

jmg-duarte · 2025-01-22T12:13:52Z

mater/lib/src/stores/mod.rs

+pub struct ConvertConfig {
+    /// How the content should be wrapped in the CAR file.
+    /// See [`WrapMode`] for details.
+    pub wrap_mode: WrapMode,
+    /// Whether to overwrite existing files at the output path.
+    /// If false, will error if the output file already exists.
+    pub overwrite: bool,
+}


This structure adds more mental overhead than it solves issues. They're two parameters, it's ok to use them explicitly

pete-eiger added 4 commits January 19, 2025 18:38

feat: wrap contents in unixfs dag

54a20f7

Merge branch 'develop' into feat/675/mater-convert-wrap-contents-in-u…

424c9d8

…nixfs-dag

feat: wrap contents in unixfs dag

1751166

Merge branch 'develop' into feat/675/mater-convert-wrap-contents-in-u…

4a37e30

…nixfs-dag

pete-eiger requested a review from a team January 21, 2025 17:36

pete-eiger self-assigned this Jan 21, 2025

pete-eiger added the ready for review Review is needed label Jan 21, 2025

pete-eiger linked an issue Jan 21, 2025 that may be closed by this pull request

feat: Mater convert, wrap contents in unixfs dag #675

Open

cleanup

7739b88

aidan46 reviewed Jan 22, 2025

View reviewed changes

jmg-duarte changed the title ~~Feat/675/mater convert wrap contents in unixfs dag~~ feat(mater): add unixfs wrapping support Jan 22, 2025

cernicc reviewed Jan 22, 2025

View reviewed changes

jmg-duarte requested changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(mater): add unixfs wrapping support #690

feat(mater): add unixfs wrapping support #690

pete-eiger commented Jan 21, 2025

aidan46 Jan 22, 2025

jmg-duarte Jan 22, 2025

cernicc Jan 22, 2025

aidan46 Jan 22, 2025

cernicc left a comment

cernicc Jan 22, 2025

cernicc Jan 22, 2025

cernicc Jan 22, 2025 •

edited

Loading

cernicc Jan 22, 2025

cernicc Jan 22, 2025

cernicc Jan 22, 2025

cernicc Jan 22, 2025

cernicc Jan 22, 2025

cernicc Jan 22, 2025

cernicc Jan 22, 2025

jmg-duarte left a comment

jmg-duarte Jan 22, 2025

jmg-duarte Jan 22, 2025

jmg-duarte Jan 22, 2025

jmg-duarte Jan 22, 2025

jmg-duarte Jan 22, 2025

jmg-duarte Jan 22, 2025

jmg-duarte Jan 22, 2025

jmg-duarte Jan 22, 2025

jmg-duarte Jan 22, 2025

jmg-duarte Jan 22, 2025

		@@ -1,4 +1,4 @@
		use std::collections::BTreeSet;
		use std::{collections::BTreeSet, time::Duration};

		#[arg(long, value_enum, default_value = "raw")]
		wrap: WrapMode,

		chunk_size: 256 * 1024, // 256KiB
		tree_width: 174, // Default from ipfs

feat(mater): add unixfs wrapping support #690

Are you sure you want to change the base?

feat(mater): add unixfs wrapping support #690

Conversation

pete-eiger commented Jan 21, 2025

PR: Add UnixFS Wrapping Support

Description

Important points for reviewers

Checklist

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

cernicc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cernicc Jan 22, 2025 • edited Loading

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

jmg-duarte 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cernicc Jan 22, 2025 •

edited

Loading