PeerDAS: Add raw functions like eip4844 #282

hangleang · 2025-01-17T10:36:23Z

This pull request will add raw functions into DAS trait, like eip4844 in this commit. The idea is to allow easier integration into its domain without needing to transform unit type into underlying field element type (e.g. converting Cell into field elements), which adding more complexity into the integration. The WIP integration into grandine can be found here.

NOTE: the current changes has a few bugs (potential issues have been marked as well), which give the following error when test in the integration:

source slice length (8192) does not match destination slice length (0)

the error is pointing to kzg/src/das.rs, which I'm not quite familiar with the logic here.

UPDATED: the bug has been fixed, after creating the issue :)

cc @ArtiomTr, please help review the changes and give some suggestions

ArtiomTr

Hello 👋,

kzg/src/das.rs file is for abstract das implementation, with configurable trusted setup size and cell size. So for ethereum-specific parameters it is better to put code into kzg/src/eth/... module, maybe kzg/src/eth/eip_7594?

Probably you can do something like:

// kzg/src/eth/eip_7594.rs file

pub fn recover_cells_and_kzg_proofs_raw::<B: EcBackend>(das: &impl DAS<B>, cell_indices: &[usize], cells: &[[u8; BYTES_PER_CELL]]) -> Result<CellsKzgProofs, String> {
   // ...
}

I believe, then you can use this function like so:

recover_cells_and_kzg_proofs_raw::<BlstBackend>(settings(), cell_indices, cells)

Few things to keep in mind:

our implementation saves cells in flat slice, so you probably want to group frs / flatten your input. Because of fixed-size slices, memory layout actually stays the same, so flattening should be zero-cost
c-kzg-4844 in recover_cells_and_kzg_proofs function allows to recover only cells, without proofs. Same in compute_cells_and_kzg_proofs, you can select - compute only cells, only proofs, or both. However, rust-eth-kzg doesn't provide such functionality. So we chose to implement as c-kzg does this, but they're not very convenient, as you need to pre-allocate output slices :). So choose what better works for you, just keep in mind that you can skip some computations.

If you need some more help, just ping me 😄

ArtiomTr · 2025-01-17T10:47:29Z

kzg/src/das.rs

+        let mut recovered_cells: Vec<B::Fr> = vec![];
+        let mut recovered_proofs: Vec<B::G1> = vec![];


You need to pre-allocate space for recovered_cells and recovered_proofs. You can look at the examples in test cases:

rust-kzg/kzg-bench/src/tests/eip_7594.rs

Lines 132 to 135 in cb71d62

let mut recv_cells =

vec![B::Fr::default(); eth::CELLS_PER_EXT_BLOB * eth::FIELD_ELEMENTS_PER_CELL];

let mut recv_proofs = vec![B::G1::default(); eth::CELLS_PER_EXT_BLOB];

Oh, this is already fixed :)

ohh, yeah. I already update those with the force-commit

hangleang · 2025-01-17T11:23:30Z

@ArtiomTr you are right, I'll update according to your suggestions

hangleang · 2025-01-17T16:10:05Z

Hello @ArtiomTr

I've been updated to follow your suggestions to move peerdas-specific functions into its own module, outside of das-abstract trait, please kindly review the changes :)

ArtiomTr

Looks good, just minor nitpicks 👍

ArtiomTr · 2025-01-17T16:12:28Z

kzg/src/eth/eip_7594.rs

+    let _ = das.recover_cells_and_kzg_proofs(
+        &mut recovered_cells,
+        Some(&mut recovered_proofs),
+        cell_indices,
+        &cells,
+    );


I believe there should be ?, because you're currently ignoring error, if it occurs in recover_cells_and_kzg_proofs:

Suggested change

let _ = das.recover_cells_and_kzg_proofs(

&mut recovered_cells,

Some(&mut recovered_proofs),

cell_indices,

&cells,

);

das.recover_cells_and_kzg_proofs(

&mut recovered_cells,

Some(&mut recovered_proofs),

cell_indices,

&cells,

)?;

ahh! good eyes

ArtiomTr · 2025-01-17T16:14:27Z

kzg/src/eth/eip_7594.rs

+    let _ = das.compute_cells_and_kzg_proofs(
+        Some(&mut recovered_cells),
+        Some(&mut recovered_proofs),
+        &blob,
+    );


Same issue as above, with recover_cells_and_kzg_proofs

ArtiomTr · 2025-01-17T16:18:03Z

kzg/src/eth/eip_7594.rs

+    let nested_cells = cfg_iter!(cells)
+        .map(|bytes| bytes_to_cell::<B>(bytes))
+        .collect::<Result<Vec<Vec<_>>, _>>()?;
+    let cells = nested_cells.into_iter().flatten().collect::<Vec<_>>();


I'm not sure, but looks like it would be better to do flatten first, and then conversion - that way you can avoid heap allocation (collecting each cell to vector), and also flattening will be zero-cost, as memory layout is not changed:

Suggested change

let nested_cells = cfg_iter!(cells)

.map(|bytes| bytes_to_cell::<B>(bytes))

.collect::<Result<Vec<Vec<_>>, _>>()?;

let cells = nested_cells.into_iter().flatten().collect::<Vec<_>>();

let cells = cfg_iter!(cells.flatten()).map(B::Fr::from_bytes).collect::<Result<Vec<_>, String>>()?;

flatten is not available in the slice array, which should be the better way?

Oh sorry, I mean as_flattened

but, even if we do so, the bytes size will not fit into the field element, we need to chunk it first anyway

Yeah, you're right, too rushed with this suggestion. It should be:

Suggested change

let nested_cells = cfg_iter!(cells)

.map(|bytes| bytes_to_cell::<B>(bytes))

.collect::<Result<Vec<Vec<_>>, _>>()?;

let cells = nested_cells.into_iter().flatten().collect::<Vec<_>>();

let cells = cfg_iter!(cells.as_flattened().chunks(BYTES_PER_FIELD_ELEMENT)).map(B::Fr::from_bytes).collect::<Result<Vec<_>, _>>()?;

The point was that we can avoid unnecessary heap allocations, and keep only one

I could not use cfg_iter on chunks, so remove on cells for now, will improve it later

Ah right, chunks is already an iterator. There is par_chunks in rayon, but I believe it wouldn't give much performance improvement

ArtiomTr · 2025-01-17T16:19:34Z

kzg/src/eth/eip_7594.rs

+    let nested_cells = cfg_iter!(cells)
+        .map(|bytes| bytes_to_cell::<B>(bytes))
+        .collect::<Result<Vec<Vec<_>>, _>>()?;
+    let cells = nested_cells.into_iter().flatten().collect::<Vec<_>>();


Same as in recover_cells_and_kzg_proofs_raw, heap allocations and flattening could be simplified

ArtiomTr · 2025-01-17T16:22:57Z

kzg/src/eth/eip_7594.rs

+    let chunk_size = FIELD_ELEMENTS_PER_CELL;
+    if bytes.len() % chunk_size != 0 {
+        return Err(format!("Invalid byte length. got {}", bytes.len(),));
+    }


I think you can just check here, if slice size is equal to FIELD_ELEMENTS_PER_EXT_BLOB

ArtiomTr · 2025-01-17T16:23:57Z

kzg/src/eth/eip_7594.rs

+                if end > BYTES_PER_CELL {
+                    return Err(format!(
+                        "Invalid cell byte length. Expected {} got {}",
+                        BYTES_PER_CELL, end,
+                    ));
+                }


Looks like this check is unnecessary, as chunks function already returns slices, with size <= chunk_size.

ArtiomTr

Perfect 👌

* fix clippy * add comments after debug * add placeholder byte into `recovered_cells` and `recovered_proofs` * move peerdas-specific functions outside of das-abstract trait * avoid multiple heap allocation * add cfg_chunks macro

hangleang · 2025-01-18T08:55:37Z

Hello, just added cfg_chunks macro, also because I would like to trigger CI as well, saw one error case in the PR while all success at my fork

hangleang · 2025-01-23T16:36:30Z

@ArtiomTr Can we merge this PR so I can switch to use rust-kzg for peerdas?

ArtiomTr · 2025-01-23T16:39:04Z

I approve, but I don't have merge rights to this repo 😄

I believe @sauliusgrigaitis can help

hangleang · 2025-01-23T16:41:03Z

Okay, I'll ask him for help

hangleang · 2025-01-23T16:42:07Z

Thanks @sauliusgrigaitis 🙏

hangleang force-pushed the das-integration branch 2 times, most recently from 68e0697 to 25a176c Compare January 17, 2025 11:03

hangleang marked this pull request as ready for review January 17, 2025 11:08

ArtiomTr suggested changes Jan 17, 2025

View reviewed changes

hangleang force-pushed the das-integration branch from 25a176c to 7075d37 Compare January 17, 2025 16:06

hangleang force-pushed the das-integration branch from 7075d37 to c9615a3 Compare January 17, 2025 16:13

ArtiomTr suggested changes Jan 17, 2025

View reviewed changes

hangleang force-pushed the das-integration branch from c9615a3 to 8180599 Compare January 17, 2025 16:57

ArtiomTr approved these changes Jan 17, 2025

View reviewed changes

PeerDAS: Add raw functions like eip4844

24df982

* fix clippy * add comments after debug * add placeholder byte into `recovered_cells` and `recovered_proofs` * move peerdas-specific functions outside of das-abstract trait * avoid multiple heap allocation * add cfg_chunks macro

hangleang force-pushed the das-integration branch from 8180599 to 24df982 Compare January 18, 2025 08:52

ArtiomTr approved these changes Jan 20, 2025

View reviewed changes

sauliusgrigaitis merged commit 0070c96 into grandinetech:main Jan 23, 2025
31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PeerDAS: Add raw functions like eip4844 #282

PeerDAS: Add raw functions like eip4844 #282

hangleang commented Jan 17, 2025 •

edited

Loading

ArtiomTr left a comment

ArtiomTr Jan 17, 2025

ArtiomTr Jan 17, 2025

hangleang Jan 17, 2025

hangleang commented Jan 17, 2025

hangleang commented Jan 17, 2025

ArtiomTr left a comment

ArtiomTr Jan 17, 2025

hangleang Jan 17, 2025

ArtiomTr Jan 17, 2025

ArtiomTr Jan 17, 2025

hangleang Jan 17, 2025

ArtiomTr Jan 17, 2025

hangleang Jan 17, 2025

ArtiomTr Jan 17, 2025

hangleang Jan 17, 2025

ArtiomTr Jan 17, 2025

ArtiomTr Jan 17, 2025

ArtiomTr Jan 17, 2025

ArtiomTr Jan 17, 2025

ArtiomTr left a comment

hangleang commented Jan 18, 2025

hangleang commented Jan 23, 2025

ArtiomTr commented Jan 23, 2025 •

edited

Loading

hangleang commented Jan 23, 2025

hangleang commented Jan 23, 2025

		let mut recovered_cells: Vec<B::Fr> = vec![];
		let mut recovered_proofs: Vec<B::G1> = vec![];

	let mut recv_cells =
	vec![B::Fr::default(); eth::CELLS_PER_EXT_BLOB * eth::FIELD_ELEMENTS_PER_CELL];

	let mut recv_proofs = vec![B::G1::default(); eth::CELLS_PER_EXT_BLOB];

PeerDAS: Add raw functions like eip4844 #282

PeerDAS: Add raw functions like eip4844 #282

Conversation

hangleang commented Jan 17, 2025 • edited Loading

ArtiomTr 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

hangleang commented Jan 17, 2025

hangleang commented Jan 17, 2025

ArtiomTr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtiomTr left a comment

Choose a reason for hiding this comment

hangleang commented Jan 18, 2025

hangleang commented Jan 23, 2025

ArtiomTr commented Jan 23, 2025 • edited Loading

hangleang commented Jan 23, 2025

hangleang commented Jan 23, 2025

hangleang commented Jan 17, 2025 •

edited

Loading

ArtiomTr commented Jan 23, 2025 •

edited

Loading