Small adjustments #2
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The idea is to keep the elements in the queue sorted both by height and pos. We make this adjustment in order to: - make this method more similar with the original one (gen_proof_for_peak()) => should be easier to upstream the changes - avoid treating tiny corner cases - improve performance (didn't do formal benchmarks, but just by running some unit tests looks like a 2-3x improvement)
Merged
Lederstrumpf
approved these changes
May 16, 2024
svyatonik
approved these changes
May 16, 2024
acatangiu
approved these changes
May 17, 2024
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Did a more in-depth review of #1 . Overall the approach seems sane to me. Opening this PR just to propose some improvements that I noticed during the review.
The main change here is related to
gen_node_proof_for_peak()The idea is to sort the elements in the queue by height and pos. This way we can keep using the same strategy in the original crate (making these changes easier to upstream), avoid treating tiny corner cases, and it also improves the performance of generating an ancestry proof.
I suppose something similar can be done for
calculate_peak_root()as well, but I'll check this at a later time and if possible, I'll open a separate PR.