Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add method to verify consistent precomputed data #561

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

akonring
Copy link
Contributor

@akonring akonring commented Apr 19, 2024

Description

This PR adds another method is_consistent_precompute to the Precompute trait interface. A client can use this method to verify that the precomputed data contains the same underlying commitment as the output of commit_only_precompute.

This is useful if a party (builder) is generating the precompute_data and commitment using commit_only_precompute and subsequently sends the precompute_data to a leader (disperser) and shares the commitment with the network. Instead of trusting the builder, the leader now merely checks (using is_consistent_precompute) that the precomputed_data agrees with the commitment before dispersing.

closes: #560


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Re-reviewed Files changed in the GitHub PR explorer

@akonring akonring force-pushed the ak/560 branch 2 times, most recently from 59bfbdf to db46192 Compare April 19, 2024 07:56
@akonring akonring changed the title Ak/560 feat: add method to verify consistent precomputed data Apr 19, 2024
nyospe
nyospe previously approved these changes Apr 19, 2024
Copy link

@nyospe nyospe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

@akonring akonring marked this pull request as ready for review April 19, 2024 11:41
Copy link
Contributor

@ggutoski ggutoski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Unfortunately you might need to repeat all this after the major rework of #556 is merged.

Comment on lines 151 to 152
payload_byte_len: u32,
num_storage_nodes: u32,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nyospe do you know payload_byte_len, num_storage_nodes at the time you want to call this? Normally this data is available via the VidCommon struct. But you only get one of those after dispersal.

I worry that you'll need to add a bunch of bookkeeping on the call side to keep track of this stuff. We tried to prevent that by making it available in the VidCommon.

Copy link
Contributor Author

@akonring akonring Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably both committer and disperser have initialized the VID scheme with the same payload_byte_len and num_storage_nodes (otherwise, how will pre-computation work). And if this is not the case then this new verification check will also reveal that, no?

let (recovery_threshold, num_storage_nodes) = (4, 6);
let mut rng = jf_utils::test_rng();
let srs = init_srs(recovery_threshold as usize, &mut rng);
let advz = Advz::new(num_storage_nodes, recovery_threshold, srs).unwrap();
let bytes_random = init_random_payload(4000, &mut rng);
(advz, bytes_random)
(advz, bytes_random, num_storage_nodes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate that you need to add num_storage_nodes here. Maybe we can revert this pending feedback from Nathan.

@akonring
Copy link
Contributor Author

Thanks for the feedback. I've rebased on top of the (refactored) main.
Let me know if more changes should be considered.
Awaiting @nyospe response here: #561 (comment) before moving forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add verification check between precompute data and vid commitment
3 participants