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 extended range proofs #102

Merged

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Jun 5, 2022

Added extended range proofs

Cargo.toml Outdated Show resolved Hide resolved
src/ristretto/bulletproofs_plus.rs Show resolved Hide resolved
src/ristretto/bulletproofs_plus.rs Show resolved Hide resolved
src/ristretto/bulletproofs_plus.rs Outdated Show resolved Hide resolved
src/ristretto/bulletproofs_plus.rs Show resolved Hide resolved
src/ristretto/bulletproofs_plus.rs Outdated Show resolved Hide resolved
src/ristretto/bulletproofs_plus.rs Outdated Show resolved Hide resolved
Ok(proof.to_bytes())
}

fn verify_batch_and_recover_masks(
Copy link
Contributor

Choose a reason for hiding this comment

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

A user is either verifying or recovering. Verifying is implicit in the action, so it's not needed in the name. Adding it in the name makes it seem like verifying is the main action here, when it is actually recovering

Suggested change
fn verify_batch_and_recover_masks(
fn batch_recover_masks(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Batch recovery of masks is more expensive than linear mask recovery for the same amount of proofs, so that is not promoted. The primary action here is batch verification (logarithmic), with the additional benefit to recover masks at a linear cost. I will update the documentation to reflect this.

proof: &Self::Proof,
statement: &RistrettoExtendedStatement,
) -> Result<Option<RistrettoExtendedMask>, RangeProofError> {
return match RistrettoRangeProof::from_bytes(proof)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you just call verify_batch_and_recover(vec![proof], vec![statement])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above comment: Batch recovery of masks is more expensive than linear mask recovery for the same amount of proofs... Also, as a proof + commitment owner, we do not always want to verify the proof, in which case we can just call recover_mask followed by verify_mask, both really cheap operations.

};
}

fn verify_mask(
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the commitment factory is a better place for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extended commitment factory can verify an extended commitment's opening, but is agnostic to mask verification, which is the primary action here.

I have added a specific test after mask recovery to also verify that the extended commitment factory can open the commitment.

stringhandler
stringhandler previously approved these changes Jun 15, 2022
Copy link
Contributor

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

I'm not moving on the minimum_value_promise: Option<u64>. If you change that to minimum_value_promise: u64, it can be mergedd

@stringhandler stringhandler dismissed their stale review June 15, 2022 07:41

Meant to be comment, not approve

@stringhandler stringhandler merged commit b7f7761 into tari-project:main Jun 15, 2022
@hansieodendaal hansieodendaal deleted the ho_extended_range_proofs branch June 15, 2022 09:39
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.

None yet

3 participants