Added content-defined chunking#209
Conversation
# Conflicts: # primitives/src/lib.rs # provider-node/Cargo.toml # provider-node/src/api.rs # provider-node/tests/s3_integration.rs
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| expected: query.data_root.clone(), | ||
| actual: "invalid hex".to_string(), | ||
| })?; | ||
| let data_root = H256::from_slice(&root_bytes); |
There was a problem hiding this comment.
H256::from_slice panics if root_bytes.len() != 32. hex_decode above validates hex characters but not length, so GET /content?data_root=abcd (2 bytes) panics → 500. Add a root_bytes.len() == 32 check returning Error::InvalidHash. See a similar pattern at other places too - maybe a shared parse_h256() helper would fix all of them?
| let chunk_size = storage_primitives::DEFAULT_CHUNK_SIZE as u64; | ||
| let start_chunk = query.offset / chunk_size; | ||
| let end_chunk = (query.offset + query.length).div_ceil(chunk_size); |
There was a problem hiding this comment.
This is the most important one. Now that S3/FS uploads are always CDC, /read's offset / DEFAULT_CHUNK_SIZE math is wrong for essentially every new root, and get_chunk_at_index still returns a chunk - so /read silently serves misaligned bytes with a 200 instead of erroring. Please add a guard that makes /read reject non-fixed roots before merge (the variable-size implementation can stay deferred, but silent corruption shouldn't ship).
| let chunks = state.storage.collect_chunks(data_root); | ||
| if chunks.is_empty() && data_root != H256::zero() { |
There was a problem hiding this comment.
get_content reassembles the entire object into one in-memory Vec. Unlike /read (paginated by offset/length), this is whole-file, so memory scales with max object size × concurrent requests. Fine for now, but worth a tracking note to stream the body (axum::body::Body from a stream) before large objects are in play.
There was a problem hiding this comment.
I agree we should stream it. It's fine if we add it here or in a follow-up PR but we need it
|
|
||
| **Unchanged.** The MMR is still leaf-per-chunk and chunk-size-agnostic — each leaf is `blake2_256(chunk_bytes)`. The Merkle proof model is unchanged. The S3/FS GET handlers still reassemble via `collect_chunks(data_root)`, which walks the tree by hash and doesn't assume any chunk size. Content addressing and per-bucket isolation are unchanged. | ||
|
|
||
| The `GET /read?data_root=&offset=&length=` byte-range endpoint **still assumes fixed-size** (its arithmetic computes `chunk_index = offset / DEFAULT_CHUNK_SIZE`). It is therefore unsafe to call on CDC-chunked roots. For whole-file historical fetch, use the new `GET /content?data_root=` endpoint, which walks the manifest. Updating `/read` for variable-size byte-range reads is deferred until a caller actually needs it. |
There was a problem hiding this comment.
Since this PR makes uploads always-CDC, /read is now wrong for the common case, not a rare one - please reframe from "deferred" to "guard now," and add a line that roots don't record their chunking strategy (so all offset math is globally unsafe until they do).
| | Insert/delete dedup | ❌ Cascades downstream | ✅ Only chunks straddling the edit change | | ||
| | In-place overwrite dedup | ✅ Works | ✅ Works | | ||
| | Append dedup | ✅ Works (only trailing chunk changes) | ✅ Works | | ||
| | Best for | Binary blobs with fixed-offset fields; embedded use where every byte counts | Text, structured data, anything with shifting edits | |
There was a problem hiding this comment.
Worth noting files < 64 KiB are always a single chunk and get no edit-dedup - the "best for text/structured data" framing oversells the small-file case.
|
|
||
| The provider stores file bytes as content-addressed chunks: each chunk's key is `blake2_256(bytes)`, so two PUTs that produce a byte-identical chunk get stored once. That dedup is automatic — *if* the chunker actually produces identical chunks across edits. | ||
|
|
||
| With a fixed-size chunker, it doesn't. Inserting a single byte at the start of a file shifts every downstream byte by one, so every chunk has different bytes, so every chunk has a different hash. v2's PUT effectively re-uploads the entire file. |
There was a problem hiding this comment.
I didn't understand what v2 meant at first. After reading I realized it was the new blob. It might be better to say "the new blob"
| | Append dedup | ✅ Works (only trailing chunk changes) | ✅ Works | | ||
| | Best for | Binary blobs with fixed-offset fields; embedded use where every byte counts | Text, structured data, anything with shifting edits | | ||
|
|
||
| `ChunkingStrategy::Fixed(usize)` remains the SDK default; CDC is opt-in via `ChunkingStrategy::ContentDefined`. The provider's HTTP upload endpoints (S3 / FS) always use CDC since their callers don't pick a strategy. |
There was a problem hiding this comment.
Then shouldn't CDC be the default for the SDK as well?
| } | ||
|
|
||
| #[test] | ||
| fn cdc_size_distribution_within_bounds() { |
There was a problem hiding this comment.
Fixed-size chunking would also pass this test. Can we add another one testing that the sizes of contiguous chunks are different?
| use scale_info::TypeInfo; | ||
| use sp_core::H256; | ||
|
|
||
| #[cfg(feature = "std")] |
There was a problem hiding this comment.
Why do we need std here? alloc is not std
| [dev-dependencies] | ||
| tokio = { workspace = true, features = ["macros", "rt-multi-thread", "test-util", "time"] } | ||
| serde_json = { workspace = true } | ||
| rand = "0.8" |
There was a problem hiding this comment.
Get it from the workspace
| let chunks = state.storage.collect_chunks(data_root); | ||
| if chunks.is_empty() && data_root != H256::zero() { |
There was a problem hiding this comment.
I agree we should stream it. It's fine if we add it here or in a follow-up PR but we need it
franciscoaguirre
left a comment
franciscoaguirre
left a comment
There was a problem hiding this comment.
Client-side encryption makes CDC useless. Right now it's optional so we can fix it in a follow-up but we should make it as efficient/easy to encrypt than to not.
|
|
||
| **Changed.** Chunks are now variable-size (between 64 KiB and 1 MiB). The provider's S3 (`PUT /s3/.../object`) and FS (`PUT /fs/.../file`) handlers chunk via `chunk_cdc_borrowed`. The Rust client SDK's `ChunkingStrategy::ContentDefined` arm uses the same chunker (previously a TODO that fell back to fixed-256K). | ||
|
|
||
| **Unchanged.** The MMR is still leaf-per-chunk and chunk-size-agnostic — each leaf is `blake2_256(chunk_bytes)`. The Merkle proof model is unchanged. The S3/FS GET handlers still reassemble via `collect_chunks(data_root)`, which walks the tree by hash and doesn't assume any chunk size. Content addressing and per-bucket isolation are unchanged. |
There was a problem hiding this comment.
blake2_256(chunk_bytes) are the leaves of each blob's chunk tree, not the MMR.
The MMR leaves are blake2_256(MmrLeaf { data_root, data_size, total_size }.encode()):
web3-storage/primitives/src/lib.rs
Line 268 in 4620532
| **Unchanged.** The MMR is still leaf-per-chunk and chunk-size-agnostic — each leaf is `blake2_256(chunk_bytes)`. The Merkle proof model is unchanged. The S3/FS GET handlers still reassemble via `collect_chunks(data_root)`, which walks the tree by hash and doesn't assume any chunk size. Content addressing and per-bucket isolation are unchanged. | |
| **Unchanged.** Chunk-size changes are confined to the per-blob Merkle tree, whose leaves are `blake2_256(chunk_bytes)` and whose root is `data_root`. That tree is chunk-size agnostic. The MMR above it that has one leaf per committed `data_root` never sees the chunks so it's unaffected too. |
|
|
||
| The `GET /read?data_root=&offset=&length=` byte-range endpoint assumes fixed-size chunks — its arithmetic is `chunk_index = offset / DEFAULT_CHUNK_SIZE`. Now that S3 and FS uploads always use CDC, that's wrong for the **common** case rather than a rare one, and `get_chunk_at_index` would still hand back *a* chunk, so the endpoint would silently serve misaligned bytes with a 200. | ||
|
|
||
| To prevent that, `/read` now **guards** every call: it walks the leaves under `data_root` and rejects with `422 variable_chunk_root` if any non-trailing leaf isn't exactly `DEFAULT_CHUNK_SIZE`. Implementation: `data_root_is_fixed_size` in `provider-node/src/api.rs`; integration coverage in `test_read_rejects_cdc_root`. For whole-file historical fetch, use the `GET /content?data_root=` endpoint, which walks the tree by hash (streaming chunk-by-chunk, so provider memory is one chunk per request, not the whole object) and works for any chunking strategy. |
There was a problem hiding this comment.
I would only keep /read around if it is useful to read a slice of chunks instead of all of them. If we want the whole file /content seems to win every time
|
|
||
| To prevent that, `/read` now **guards** every call: it walks the leaves under `data_root` and rejects with `422 variable_chunk_root` if any non-trailing leaf isn't exactly `DEFAULT_CHUNK_SIZE`. Implementation: `data_root_is_fixed_size` in `provider-node/src/api.rs`; integration coverage in `test_read_rejects_cdc_root`. For whole-file historical fetch, use the `GET /content?data_root=` endpoint, which walks the tree by hash (streaming chunk-by-chunk, so provider memory is one chunk per request, not the whole object) and works for any chunking strategy. | ||
|
|
||
| Worth flagging: **roots don't record their chunking strategy.** A `data_root` is just a Merkle root over a list of `blake2_256(chunk)` leaves; nothing in the tree distinguishes a fixed-256K-chunked root from a CDC root from anything else. Any future endpoint that does offset/length arithmetic is therefore globally unsafe until we either (a) tag roots with their strategy (e.g. an extra storage map alongside the bucket's S3/FS index), (b) make the read side fully strategy-agnostic by walking a manifest with explicit per-chunk offsets, or (c) only emit one canonical chunking. Right now the guard substitutes for that by content-inspecting the leaves at request time: it rejects only when it can positively prove the root is variable-size (any non-trailing leaf differs from `DEFAULT_CHUNK_SIZE`). Empty / unknown roots pass through — downstream `get_chunk_at_index` returns empty results cleanly for those, so this preserves the pre-guard behaviour for nonexistent roots. |
There was a problem hiding this comment.
Tagging the chunking strategy doesn't seem like it gives us much, especially since a tree traversal can get that information. The manifest though would allow /content to also read a slice of the file. After that I would just delete /read (or rename /content to /read) depending on what name we prefer
|
|
||
| Worth flagging: **roots don't record their chunking strategy.** A `data_root` is just a Merkle root over a list of `blake2_256(chunk)` leaves; nothing in the tree distinguishes a fixed-256K-chunked root from a CDC root from anything else. Any future endpoint that does offset/length arithmetic is therefore globally unsafe until we either (a) tag roots with their strategy (e.g. an extra storage map alongside the bucket's S3/FS index), (b) make the read side fully strategy-agnostic by walking a manifest with explicit per-chunk offsets, or (c) only emit one canonical chunking. Right now the guard substitutes for that by content-inspecting the leaves at request time: it rejects only when it can positively prove the root is variable-size (any non-trailing leaf differs from `DEFAULT_CHUNK_SIZE`). Empty / unknown roots pass through — downstream `get_chunk_at_index` returns empty results cleanly for those, so this preserves the pre-guard behaviour for nonexistent roots. | ||
|
|
||
| A real variable-size byte-range implementation of `/read` is deferred until a caller needs it. |
There was a problem hiding this comment.
Yeah I guess we could do it in a follow-up
|
|
||
| `ChunkingStrategy::default()` is now `ContentDefined` — the SDK CDC-chunks on upload by default; `Fixed(usize)` stays available for the binary/embedded cases above. The provider's HTTP upload endpoints (S3 / FS) always use CDC since their callers don't pick a strategy. | ||
|
|
||
| The default could flip because the SDK's read path no longer depends on `/read`. `StorageUserClient::download` (and `spot_check`) now fetch chunks by index through `GET /chunk_proof?data_root=&chunk_index=`, which is index-based — it never touches the fixed-size offset math that the `/read` guard rejects — and returns each chunk with its Merkle proof. The client verifies every chunk two ways: `blake2_256(bytes) == leaf_hash`, then `verify_merkle_proof(leaf_hash, index, proof, data_root)`. It iterates `0..` until the provider 404s (end of file). This is strictly stronger than the old `/read` path, which returned proofs but never checked them against `data_root` — so a lying provider was previously undetectable on download. `download(offset, length)` walks from chunk 0 and slices the result; there's no client-side random access by byte offset under CDC (chunk sizes are content-defined), which is fine since real callers read from offset 0. |
|
|
||
| With a fixed-size chunker, it doesn't. Consider an already-uploaded file (the "original blob") and a small edit that produces a new blob to upload. If the edit inserts a single byte at the start of the file, every downstream byte shifts by one — so every chunk has different bytes, so every chunk has a different hash. The PUT for the new blob effectively re-uploads the entire file. | ||
|
|
||
| Content-defined chunking solves this by choosing chunk boundaries from the *content* (via a rolling hash), not from fixed byte offsets. When you insert bytes in the middle of a file, the boundaries downstream of the edit fall at the same content positions as before; the chunks between them are byte-identical to the original blob's chunks and dedup through content addressing. |
There was a problem hiding this comment.
This is not the case if we do client-side encryption, no? Say we have a blob in plaintext, P, and the edited blob in plaintext, P'. When we encrypt them we get C and C'. Sure, P and P' do share a lot of the same bytes but C and C' would be totally different, therefore making CDC useless.
To make CDC useful the client would need to chunk the plaintext itself, generating P_0, P_1, ..., P_N and P'_0, P'_1, ..., P'_M. Then all of the chunks would need to be encrypted deterministically so they can actually be reused.
We could do this by using the same ChaCha20 nonce for all chunks (instead of a random one) and deriving that nonce from the user's account's private key (so it's different per user).
This would also mean chunking needs to be done in the client, kind of like how we do it on layer0. It would also need to happen in layer1, unlike today where we do it in the storage provider.
4 participants
Added CDC to allow efficient mutability