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: use hash based record verification for chunks #1059

Merged
merged 11 commits into from Dec 28, 2023

Conversation

RolandSherwin
Copy link
Member

@RolandSherwin RolandSherwin commented Dec 7, 2023

Description

Summary generated by Reviewpad on 07 Dec 23 15:52 UTC

This pull request includes changes to multiple files.

  • The mod.rs file in the messages module adds support for chunk proofs and organizes message-related components.
  • Changes in the FilesCmds enum, upload_files and upload_chunk functions remove the show_holders functionality.
  • The QueryResponse enum adds new variants for GetStoreCost and GetChunkExistenceProof queries.
  • The Node implementation in the node.rs file adds support for handling GetChunkExistenceProof queries.
  • The Cargo.toml file adds the tiny-keccak dependency.
  • Changes in the query.rs file update imports and add new variants to the Query enum.
  • The chunk_proof.rs file adds the ChunkProof struct and associated methods.
  • Changes in the Files implementation simplify parameter passing and remove unused show_holders functionality.
  • Changes in various files import ChunkProof and modify functionality related to chunk storage and verification.
  • Changes in the event.rs file modify the SwarmDriver implementation for handling Kademlia events.
  • Changes in the error.rs file import types and add new error variants for chunk existence and register not found.
  • Changes in the file add new functions for retrieving chunk proofs and store costs from the network.

Please review these changes and let me know if you have any questions.

@reviewpad reviewpad bot added the Large Large sized PR label Dec 7, 2023
sender
.send(current_closest)
.map_err(|_| Error::InternalMsgChannelDropped)?;
// TODO: consider order the result and terminate when reach any of the

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
@RolandSherwin RolandSherwin force-pushed the nonce_verification branch 5 times, most recently from 7d93dac to 518a7ac Compare December 11, 2023 18:08
@RolandSherwin RolandSherwin marked this pull request as ready for review December 11, 2023 18:32
@reviewpad reviewpad bot requested a review from bochaco December 11, 2023 18:33
@RolandSherwin RolandSherwin changed the title feat: use nonce based record verification for chunks feat: use hash based record verification for chunks Dec 11, 2023
debug!("send_and_get_responses for {req:?}");
let mut list_of_futures = peers
.iter()
.map(|peer| Box::pin(self.send_request(req.clone(), *peer)))
.map(|peer| {
Box::pin(async {

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
@RolandSherwin RolandSherwin force-pushed the nonce_verification branch 10 times, most recently from 8794b70 to ebb01b0 Compare December 18, 2023 09:31
sn_client/src/api.rs Outdated Show resolved Hide resolved
@joshuef
Copy link
Contributor

joshuef commented Dec 18, 2023

added DoNotMerge as breaking change we may not want in juusst yet

Copy link

reviewpad bot commented Dec 18, 2023

Reviewpad Report

‼️ Errors

  • Unconventional commit detected: 'chore(cli)!: remove show_holders flag as we perform that by default
  • during hash based verification' (365f9f4)

@RolandSherwin RolandSherwin force-pushed the nonce_verification branch 2 times, most recently from 8d6880b to 751fbcf Compare December 19, 2023 09:40
pub struct ChunkProof([u8; 32]);

impl ChunkProof {
pub fn new(record_value: &[u8], nonce: Nonce) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

by making ChunkProof constructor taking a chunk_name instead of whole content will be enough,
and has the benefit of avoid un-necessary access to the whole content?
especialy at the previous place of

                    // make sure the chunk is stored;
                    let chunk = Chunk::new(Bytes::from(std::fs::read(&chunk_path)?));
                    let res = client.verify_chunk_stored(&chunk).await;

which has to read from disk ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This can lead to dishonest nodes just storing the keys and discarding the content. And the proof that they create would be valid.

Copy link
Member

Choose a reason for hiding this comment

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

ah, right, make sense.

@joshuef joshuef added this pull request to the merge queue Dec 28, 2023
Merged via the queue into maidsafe:main with commit c0c18f8 Dec 28, 2023
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Large Large sized PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants