[3/3] - Replace Legacy History Commitment Implementation #686
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
Yay! For automated security checks. It found a potentially dangerous memory aliasing in a for loop. Also, I'm suppressing the complaints about errors which the crypto.KeccakState implementations of Read and Write cannot throw.
The legacy in inclusion-proofs packages are only used from tests at this point. And, those tests are largely only used for benchmarking the optimized implementation against the legacy one. There is one method from the prefix-proofs package which is used in the layer2-state-provider for checking that the prefix proofs generated from the optimized implementation are verifiable.
This change is mostly just suffling around code blocks, renaming variables, and adding comments. But, it also adds a cursor and keeps track of which node in the tree is actively being calculated. This allows the historyCommitter to search for the values of specific nodes by position in the tree while calculating the root. The only reason for committing the code at this state is to benchmark before and after adding the inclusion_proof calculation.
This allows us to remove the whole implementation from inclusion_proof.go and the associated tests. Also, fun fact, that existing implementation was not only much slower, but it was incorrect. I'll add some differential fuzzing to show that these inclusion proofs match the results from the legacy inclusionproofs package.
The test was taking 26 seconds and always passing. Now, the whole package's tests take < 1 second. :(
The previous code restructuring wasn't really "pure" and makes the PR look much more complicated than it is.
Previously, fillers were being created for the entire depth of the tree, but, they are only read up to the depth of a tree of size virtual.
This version of the code should be easier to maintain and only performs minutely slower because of having to copy the cursor in each recursive stack frame. This commit also rearranges the conditional logic on the left and right sides of the tree to make it simpler to follow the recursion.
The history package doesn't need to let clients work directly with a historyCommitter instance. It is really an implementation detail of the history package.
Now that the virtual Merkle tree is fully-supported, it is okay if the l2 state provider sends only the leaves it actually has values for. The padding is not necessary.
It's not necessary. The traversal is simple to unwind using the shared cursor at all levels of recursion. This gives back the ~10% performance boost previously taken away with commit bafadd1.
Specifically: 1. Added a comment about History.Height 2. Fixed some comment typos 3. Extracted a virtualFrom helper-method 4. Adopted slices.Clone 5. Cleaned up a problem with the merge from main
gligneul
approved these changes
Oct 15, 2024
The tiny bit of expressiveness it provides is not really worth the confusion it could cause if someone uses the type to cast a 0 value than it's worth.
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.
Uses the new history commitment impl across codebase.
Resolves https://linear.app/offchain-labs/issue/NIT-2789/use-potuzs-optimized-history-commitments-across-bold