AV-50 (AV-77) - Fetch missing cells from DHT #13

aterentic-ethernal · 2022-11-22T13:38:49Z

This PR supports fetch of DHT cells on light client. Main changes are:

verify_equality now returns pair of verified and missing row indexes
rows function is added to data mod, to be able to construct data row out of a fetched cells

codecov-commenter · 2022-11-24T09:51:42Z

Codecov Report

Base: 58.19% // Head: 59.54% // Increases project coverage by +1.35% 🎉

Coverage data is based on head (0c7bea5) compared to base (b0721ef).
Patch coverage: 94.88% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #13      +/-   ##
==========================================
+ Coverage   58.19%   59.54%   +1.35%     
==========================================
  Files          39       39              
  Lines        5236     5411     +175     
==========================================
+ Hits         3047     3222     +175     
  Misses       2189     2189

Impacted Files	Coverage Δ
kate/recovery/src/index.rs	`56.98% <20.00%> (-0.79%)`	⬇️
kate/recovery/src/data.rs	`63.17% <96.00%> (+9.55%)`	⬆️
kate/recovery/src/commitments.rs	`86.33% <96.80%> (+8.69%)`	⬆️
kate/recovery/src/matrix.rs	`82.83% <100.00%> (+3.55%)`	⬆️
kate/src/com.rs	`96.62% <100.00%> (-0.14%)`	⬇️
primitives/nomad/merkle/src/error.rs	`4.19% <0.00%> (-0.15%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

MiguelDD1 · 2022-11-25T11:46:51Z

kate/recovery/src/commitments.rs

 	if commitments.len() % config::COMMITMENT_SIZE > 0 {
 		return Err(Error::InvalidData(DataError::BadCommitmentsData));
 	}

-	let app_rows = com::app_specific_rows(index, dimension, app_id);
+	if <u32>::try_from(commitments.len() / config::COMMITMENT_SIZE)? != dimensions.extended_rows() {


Always cast to the biggest type:

Suggested change

if <u32>::try_from(commitments.len() / config::COMMITMENT_SIZE)? != dimensions.extended_rows() {

if (commitments.len() / config::COMMITMENT_SIZE) != dimensions.extended_rows() as usize {

Isn't that platform dependent and wouldn't as panic if 32-bit is not supported (should we even consider this case?). If we assume that smallest possible usize is u32 then 👍

Yes, it is platform dependent. It could be u32 (like WASM) or u64 (like current micros).
See https://doc.rust-lang.org/std/primitive.usize.html

Changed to
if (commitments.len() / config::COMMITMENT_SIZE) != dimensions.extended_rows().try_into()?
to cast to bigger type and avoid panics. Wdyt?

MiguelDD1 · 2022-11-25T11:47:45Z

kate/recovery/src/commitments.rs


-	if !all_rows_present {
-		return Err(Error::InvalidData(DataError::RowAndCommitmentsMismatch));
+	if <u32>::try_from(rows.len())? != dimensions.extended_rows() {


ditto

Suggested change

if <u32>::try_from(rows.len())? != dimensions.extended_rows() {

if rows.len() != dimensions.extended_rows() as usize {

Same here, used try_into for conversion.

MiguelDD1 · 2022-11-25T11:59:23Z

kate/recovery/src/commitments.rs

+		.collect::<Result<Vec<(u32, bool)>, Error>>()?;
+
+	let verified = verifications
+		.iter()
+		.filter(|(_, is_equal)| *is_equal)
+		.map(|&(i, _)| i)
+		.collect::<Vec<u32>>();


verifications does not need to be consolidated into a Vec. BTW you could merge the latest .map().filter().map() into a single filter_map() call.

Suggested change

.collect::<Result<Vec<(u32, bool)>, Error>>()?;

let verified = verifications

.iter()

.filter(|(_, is_equal)| *is_equal)

.map(|&(i, _)| i)

.collect::<Vec<u32>>();

.filter(|(_, is_equal)| *is_equal)

.map(|&(i, _)| i)

.collect::<Vec<u32>>();

This was interesting to change, removed almost 50% of code, after finding transpose fn on result :) So now it looks like:

let verified = commitments .chunks_exact(config::COMMITMENT_SIZE) .zip(rows.iter()) .zip(0u32..) .filter(|(.., index)| app_rows.contains(index)) .filter_map(|((commitment, row), index)| { try_into_scalars(row.as_ref()?) .map(|scalars| Evaluations::from_vec_and_domain(scalars, domain).interpolate()) .and_then(|polynomial| prover_key.commit(&polynomial).map_err(From::from)) .map(|result| (result.to_bytes() == commitment).then_some(index)) .transpose() }) .collect::<Result<Vec<u32>, Error>>()?; app_rows.retain(|row| !verified.contains(row)); Ok((verified, app_rows))

wdyt?

MiguelDD1 · 2022-11-25T12:03:16Z

kate/recovery/src/commitments.rs

+	let missing = app_rows
+		.into_iter()
+		.filter(|&row| !verified.contains(&row))
+		.collect::<Vec<u32>>();


Remove new allocations:

Suggested change

let missing = app_rows

.into_iter()

.filter(|&row| !verified.contains(&row))

.collect::<Vec<u32>>();

app_rows.retains(|&row| !verified.contains(&row));

@MiguelDD1 This might be an issue in regards to retain. Don't know if the subsequent change in the retain implementation changed anything, but it might be prudent to consider the perf impact mentioned in the issue. Wdyt?

I have updated code @sh3ll3x3c , it seems that issue is solved, and vec should be smaller than 512, so lets revert if we notice anything here.

MiguelDD1 · 2022-11-25T12:08:10Z

kate/recovery/src/data.rs

+pub fn rows(cells: &[Cell]) -> Vec<(u32, Vec<u8>)> {
+	let mut sorted_cells = cells.to_vec();


If 1st step is cells.to_vec(), let's simplify the interface:

Suggested change

pub fn rows(cells: &[Cell]) -> Vec<(u32, Vec<u8>)> {

let mut sorted_cells = cells.to_vec();

pub fn rows(mut cells: Vec<Cell>) -> Vec<(u32, Vec<u8>)> {

But this mutates argument just for the sake of sorting (and we cannot enforce sorted vector through type I guess), its kinda awkward to have interface like that. Wdyt?

@aterentic-ethernal See this

@MiguelDD1 doesn't this change make the interface more ambiguous in terms whether the data is actually changing or not?

I'm not sure about this one, by marking it as mut we allow caller to clone if needed (or to avoid cloning otherwise), but it introduces implicit coupling. If we remove sorting as some point, we need to change interface of fn. Also, its not clear why cells are mutated. I know its not that important here, but wanted to here more arguments.

MiguelDD1 · 2022-11-25T12:18:10Z

kate/recovery/src/data.rs

+	sorted_cells.sort_by(|a, b| {
+		let by_row = a.position.row.cmp(&b.position.row);
+		let by_col = a.position.col.cmp(&b.position.col);
+		by_row.then(by_col)
+	});


Is not the default implementation if you add #[derive(PartialOrd, Ord)] to Position?
(see Ord Derivable:

Suggested change

sorted_cells.sort_by(|a, b| {

let by_row = a.position.row.cmp(&b.position.row);

let by_col = a.position.col.cmp(&b.position.col);

by_row.then(by_col)

});

sorted_cells.sort_by_key(|cell| cell.position);

Well it depends whats affects sort more, rows or columns. In this case its row index. Having default implementation would maybe make that implicit?

I see, you're right.
What do you think about next simplification?

sorted_cells.sort_by(|a, b| (a.position.row, a.position.col).cmp(&(b.position.row,b.position.col))

This is cool, its intuitive to assume that first element of pair has precedence. Ive updated the code.

MiguelDD1 · 2022-11-25T12:19:47Z

kate/recovery/src/index.rs

+/// Size is nuber of the data cells in the matrix.
+/// Index is list of pairs (app_id, start_index),
+/// where start index is index of first cell for that application.


Move comments (Size and Index) into each member.

aterentic-ethernal force-pushed the AV-50 branch 2 times, most recently from 263c77a to bf69b72 Compare November 23, 2022 14:14

aterentic-ethernal changed the title ~~Av 50~~ AV-50 (AV-77) - Fetch missing cells from DHT Nov 23, 2022

aterentic-ethernal marked this pull request as ready for review November 23, 2022 14:43

aterentic-ethernal requested review from a team November 24, 2022 09:35

aterentic-ethernal force-pushed the AV-50 branch from 088f532 to 3127ee7 Compare November 24, 2022 09:37

aterentic-ethernal force-pushed the AV-50 branch 2 times, most recently from 483be54 to 0c7bea5 Compare November 25, 2022 07:28

sh3ll3x3c approved these changes Nov 25, 2022

View reviewed changes

MiguelDD1 reviewed Nov 25, 2022

View reviewed changes

aterentic-ethernal added 7 commits November 28, 2022 08:28

Return commitment verification per row instead per matrix

23bbdb3

Return verified and missing app data rows on commitments check

e92519e

Add extended rows positions

9a04a8c

Construct row data from given cells

b54757f

Add base test for verify equality

a95858d

Treat invalid rows as missing.

9e719c4

Simplify verification and type conversions.

179877d

aterentic-ethernal force-pushed the AV-50 branch from 40b34cc to 179877d Compare November 28, 2022 07:28

Update versions

c234b8f

sh3ll3x3c approved these changes Nov 28, 2022

View reviewed changes

MiguelDD1 approved these changes Nov 29, 2022

View reviewed changes

aterentic-ethernal merged commit f536c60 into main Nov 29, 2022

aterentic-ethernal deleted the AV-50 branch November 29, 2022 09:48

	if <u32>::try_from(commitments.len() / config::COMMITMENT_SIZE)? != dimensions.extended_rows() {
	if (commitments.len() / config::COMMITMENT_SIZE) != dimensions.extended_rows() as usize {

	if <u32>::try_from(rows.len())? != dimensions.extended_rows() {
	if rows.len() != dimensions.extended_rows() as usize {

		pub fn rows(cells: &[Cell]) -> Vec<(u32, Vec<u8>)> {
		let mut sorted_cells = cells.to_vec();

	pub fn rows(cells: &[Cell]) -> Vec<(u32, Vec<u8>)> {
	let mut sorted_cells = cells.to_vec();
	pub fn rows(mut cells: Vec<Cell>) -> Vec<(u32, Vec<u8>)> {

AV-50 (AV-77) - Fetch missing cells from DHT #13

AV-50 (AV-77) - Fetch missing cells from DHT #13

Uh oh!

Conversation

aterentic-ethernal commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiguelDD1 Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiguelDD1 Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aterentic-ethernal commented Nov 22, 2022 •

edited

Loading

codecov-commenter commented Nov 24, 2022 •

edited

Loading

MiguelDD1 Nov 25, 2022 •

edited

Loading

MiguelDD1 Nov 25, 2022 •

edited

Loading