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

fix: avoid cloning range proofs during verification #6166

Merged

Conversation

AaronFeickert
Copy link
Collaborator

@AaronFeickert AaronFeickert commented Feb 23, 2024

Description

Removes unnecessary cloning of range proofs during batch verification.

Motivation and Context

Currently, range proofs exist in transaction outputs as byte vectors. When parsing outputs to collect these proofs for batch verification, the byte vectors are cloned. This is done because the verification API requires that the vector of proofs contain a vector reference to each proof, but the BulletRangeProof type wrapper provides a slice instead as part of its ByteArray implementation. Because proofs are several hundred bytes and batches can be large, this cloning is wasteful.

This PR adds BulletRangeProof::as_vec, which returns the underlying range proof as a vector reference. Doing so lets us avoid the clone while collecting the range proofs.

How Has This Been Tested?

Existing tests pass.

What process can a PR reviewer use to test or verify this change?

Confirm that the change removes the cloning.

@AaronFeickert AaronFeickert requested a review from a team as a code owner February 23, 2024 18:03
@ghpbot-tari-project ghpbot-tari-project added P-reviews_required Process - Requires a review from a lead maintainer to be merged P-acks_required Process - Requires more ACKs or utACKs labels Feb 23, 2024
Copy link

Test Results (CI)

1 260 tests   1 260 ✅  12m 3s ⏱️
   39 suites      0 💤
    1 files        0 ❌

Results for commit 129fbe5.

@AaronFeickert
Copy link
Collaborator Author

While this approach seems to do the trick, it feels non-idiomatic to me. Is there a better design that wouldn't require the new as_vec, like some kind of refactoring?

Copy link

Test Results (Integration tests)

29 tests  +29   29 ✅ +29   12m 27s ⏱️ + 12m 27s
11 suites +11    0 💤 ± 0 
 2 files   + 2    0 ❌ ± 0 

Results for commit 129fbe5. ± Comparison against base commit f5860a8.

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Feb 26, 2024
@SWvheerden SWvheerden merged commit 19a824d into tari-project:development Feb 26, 2024
15 of 16 checks passed
@AaronFeickert AaronFeickert deleted the range-proof-cloning branch February 26, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants