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

optimize validate_removals() #17560

Merged
merged 3 commits into from
Feb 16, 2024
Merged

optimize validate_removals() #17560

merged 3 commits into from
Feb 16, 2024

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Feb 14, 2024

Purpose:

transition validate_removals() to use the fast function to compute a merkle tree root. The MerkleSet class is an inefficient implementation, and very expensive to use. The block validation as well as validate_additions() were transitioned to use the fast function when I first introduced it, but must have missed this place.

history

in #10146 Mariano introduced an assert removals_res.proofs is not None which looks wrong, because the function that the proof is passed to handles the case where it's None. That's specifically the case this PR focuses on updating.

The fact that nothing has been broken this whole time (as far as I can tell) may suggest that perhaps the feature of omitting the proof when all removals are returned, is unnecessary.

This patch operates under the assumption that it is useful. Improves it and removes the assert blocking it.

Current Behavior:

validate_removals() is expensive.
The response to a removals request must include proofs, even when every single removal is included.

New Behavior:

validate_removals() is less expensive.
The response to a removals request may omit proofs if every single removal is included.

Testing Notes:

Tests have been added.

@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Feb 14, 2024
@arvidn arvidn marked this pull request as ready for review February 15, 2024 14:22
@arvidn arvidn requested a review from a team as a code owner February 15, 2024 14:22
@arvidn arvidn added the ready_to_merge Submitter and reviewers think this is ready label Feb 16, 2024
@Starttoaster Starttoaster merged commit a9ee02b into main Feb 16, 2024
268 checks passed
@Starttoaster Starttoaster deleted the validate-removals branch February 16, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants