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: reduce verification vector allocation #127

Merged
merged 1 commit into from Mar 27, 2024

Conversation

AaronFeickert
Copy link
Contributor

@AaronFeickert AaronFeickert commented Mar 22, 2024

Batch verification vectors are allocated in part using the expected aggregation factor of each statement in the batch. However, this was being done using the aggregation factors corresponding to each statement's associated generators, which may exceed the aggregation factors actually used in the statements. The result was a possible over-allocation of these vectors.

This PR changes the allocation to use the actual aggregation factors.

Closes #126.

@AaronFeickert
Copy link
Contributor Author

One way to test this PR is to check (just before the final verification multiscalar multiplication evaluation) that dynamic_scalars.len() == msm_dynamic_len && dynamic_points.len() == msm_dynamic_len after the change, but that this check fails before the change.

@sdbondi
Copy link
Member

sdbondi commented Mar 26, 2024

The reasoning and the code make sense. Is it correct to assume that in Tari's usage, the aggregation factor and number of commitments are always the same?

Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

utACK

@hansieodendaal
Copy link
Contributor

hansieodendaal commented Mar 26, 2024

The reasoning and the code make sense. Is it correct to assume that in Tari's usage, the aggregation factor and number of commitments are always the same?

@sdbondi , yes they are , see https://github.com/tari-project/bulletproofs-plus/pull/127/files#diff-a4997ec3ef089e27ef54a31d22f718c1e73a454188bab2ec2f96d55f172aa687L239

@AaronFeickert
Copy link
Contributor Author

AaronFeickert commented Mar 26, 2024

@sdbondi yes, but not quite for the reason that @hansieodendaal gave.

There's an unfortunate use of terminology: parameters specify an aggregation factor that represents the largest number of commitments that can be accommodated, but within the proof mechanics we also specify the aggregation factor to be the actual number of commitments handled by any given proof. This design was intended to be flexible in cases where the caller needs to verify proofs where the aggregation factor is not fixed, but is merely capped.

It is the case that the Tari codebase currently only generates parameters that account for trivial (non)-aggregation, so the case handled by this PR wouldn't arise. But in the more general case where the parameters can accommodate more commitments than are provided to the verifier, it would arise and result in a (small) over-allocation. This is because the verification arithmetic only needs to account for the commitments in the proof, not whatever extra generators might be hanging around.

@AaronFeickert
Copy link
Contributor Author

AaronFeickert commented Mar 26, 2024

It might be beneficial to do some renaming, such that the aggregation factor specified in parameters is called maximum_aggregation_factor or something, in order to better identify what it does.

See #130.

@hansieodendaal hansieodendaal merged commit 6c4bfe0 into tari-project:main Mar 27, 2024
8 checks passed
@AaronFeickert AaronFeickert deleted the allocation branch March 27, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verification vectors are over-allocated
4 participants