[Performance] : Optimize Merkle Tree Hashing by Using Byte-Level SHA256#33
[Performance] : Optimize Merkle Tree Hashing by Using Byte-Level SHA256#33Shubhamx404 wants to merge 10 commits intoAOSSIE-Org:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded a bytes-returning SHA-256 helper and refactored Merkle routines to operate on raw bytes; introduced a benchmarking routine (tracemalloc + perf_counter) and CLI flag to run benchmarks; tests updated to use new helpers and proof-loading/verification flow. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (--BENCHMARK_MODE)
participant Utils as utils.run_benchmark / Merkle routines
participant FS as FileSystem
participant Hash as SHA256
participant Prof as Profiler
CLI->>Utils: run_benchmark(file_path, chunk_size)
Utils->>Prof: tracemalloc.start & perf_counter start
Utils->>FS: read file in chunks
loop per chunk
Utils->>Hash: compute_sha256_bytes(chunk)
Hash-->>Utils: raw digest bytes
Utils->>Utils: assemble leaves & compute parents (raw bytes)
end
Utils->>Prof: stop & get peak/time
Utils-->>CLI: output root, proof timings, peak memory
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
backend
python
size/M
external-contributor
pending-coderabbit-review
and removed
size/M
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmark_custom.py`:
- Around line 42-44: The current except block that catches "except Exception as
e" and only does print(f"An error occurred: {e}") swallows failures; change it
to either catch specific expected exceptions or, after logging the error (use
logging.exception or similar for stacktrace), re-raise the exception or exit
with a non-zero status so automation sees failure; locate the "except Exception
as e" block and replace the print-only behavior with structured logging plus
either "raise" or a sys.exit(1) to ensure failures are propagated.
- Line 33: The local variable "proof" is assigned from
generate_merkle_proof(file_path, chunk_index=chunk_index, chunk_size=chunk_size)
but never used, causing Ruff F841; remove the unused assignment by either
calling generate_merkle_proof(...) without capturing its return or assign it to
a throwaway "_" (e.g., _ = generate_merkle_proof(...)) in the same spot where
"proof = generate_merkle_proof(...)" appears to silence the lint error while
keeping the call semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c44dedfc-9e6c-4647-aacd-c33adf46f5e9
📒 Files selected for processing (3)
benchmark_custom.pyopenverifiablellm/utils.pytests/test_util.py
Shubhamx404
changed the title
|
hey @Archit381 can you review this pr linked with issue #26 |
|
@Shubhamx404 Do the following changes pls
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openverifiablellm/utils.py`:
- Around line 468-472: The current fragile parsing that sets input_dump =
sys.argv[1] can misassign the file when flags like "--BENCHMARK_MODE=TRUE" come
first; replace this with proper argparse usage to define a positional input_dump
and an optional --BENCHMARK_MODE flag (so run_benchmark(input_dump) or
extract_text_from_xml(input_dump) are called reliably), or at minimum validate
that input_dump is an existing file path before using it and scan sys.argv for
the benchmark flag instead of assuming position; update the main entry logic
around input_dump, run_benchmark, and extract_text_from_xml accordingly.
- Line 427: The unpacked variable current_mem from the
tracemalloc.get_traced_memory() call is unused; rename it to _current_mem (or
_current_mem) to indicate intentional unused status. Update the assignment where
tracemalloc.get_traced_memory() is called (the current_mem, peak_mem =
tracemalloc.get_traced_memory() expression) to use the prefixed name so linters
and reviewers recognize it as intentionally unused while leaving peak_mem
unchanged.
- Around line 440-441: The chunk_index selection can exceed available chunks and
cause IndexError; instead of using size_mb > 10 to pick chunk_index=10 in the
call to generate_merkle_proof(file_path, chunk_index=chunk_index,
chunk_size=chunk_size), compute the actual chunk count from file size and
chunk_size (e.g., chunk_count = ceil(file_size_bytes / chunk_size)) and set
chunk_index = min(10, chunk_count - 1) (or 0 if chunk_count == 0) before calling
generate_merkle_proof so you always pass a valid chunk index.
- Around line 76-78: Add a unit test that asserts compute_merkle_root produces
the expected hardcoded root for a multi-chunk input: create a deterministic list
of chunks (byte strings), compute the merkle root via
compute_merkle_root(chunks) and compare its hex (or bytes) to a precomputed
expected value (from an independent SHA-256 concatenation calculation) to catch
byte-order/concatenation issues; reference the compute_merkle_root function and
use the same byte-formatting convention as compute_sha256_bytes for comparison,
and include at least a 3+ chunk test vector so the tree has multiple levels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bea160f-0730-46ea-8594-8683af360b8b
📒 Files selected for processing (1)
openverifiablellm/utils.py
|
Hey @Archit381 i implemented what you needed 
|
|
Add before vs after resource consumption benchmarks in this PR Also fix coderabbit issues |
|
@Archit381 showed memory usage before and after before 
After 
|
|
@Shubhamx404 The results don't really show any significant upside to your change |
|
@Archit381 also genrating manifest after this takes less time and memory usage is indentical too before and after reducing the chunk_size to very small values (e.g., 4 kb or 8 bytes) it creates millions of Merkle tree nodes. The old implementation slows down or crashes due to repeated bytes.fromhex() conversions, while the new byte-level chained-commitments-fix handles it efficiently and scales without stalling i checked the codes and resolve the issue .. new benchmark score and below befor and after |
|
hey @Archit381 i resolved coderabbit issues , i again run benchmark for conformation before 
After 
here is observation
|
coderabbit follow up Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openverifiablellm/utils.py`:
- Around line 457-462: The unconditional proof logging at the end of the block
(logging generate_merkle_proof, peak_mem_proof, proof_time, chunk_index) should
be removed and instead only logged inside the branch that actually generates a
proof (the branch that sets end_time, chunk_index and traces memory); update the
generate_merkle_proof related logging so it is emitted once per proof generation
and avoid referencing end_time or chunk_index when the function returned early
for an empty file—i.e., move the logger.info calls for proof_time and
peak_mem_proof into the proof-generation branch and delete the
duplicate/unconditional logging at the end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b9408b3-89bb-4909-9da4-ad5a6ec85640
📒 Files selected for processing (1)
openverifiablellm/utils.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@Archit381 i again review everyting and test for benchmark 
after 
|
|
@Archit381 can you please review this |
|
@Shubhamx404 Pull from main, fix the conflicts and check again. We are using |
|
@Archit381 okay , working on it |
@Archit381 all test passed |
|
Fix Lint test |
|
hey @Archit381 fixed lint issue |
after changes |
2 participants
Addressed Issues:
Fixes #26
This PR improves the performance and correctness of the Merkle tree implementation by removing repeated hex string conversions during hashing.
Summary
Previously, the functions
compute_merkle_root,generate_merkle_proof```, andverify_merkle_proofrelied directly oncompute_sha256()```, which returns a hexadecimal string (.hexdigest()). However, hashlib.sha256() internally operates on raw bytes. Because of this mismatch, the implementation repeatedly converted between hex strings and raw bytes while constructing the Merkle tree.For large files, this resulted in significant overhead.
Before
Solution
The Merkle tree implementation was optimized to operate on raw bytes instead of hexadecimal strings. A new helper function compute_sha256_bytes() returns the SHA256 digest using .digest(), allowing all intermediate hashes to remain in byte format. This removes repeated bytes.fromhex() conversions and reduces overhead, with hex conversion performed only once when returning the final Merkle root.
After
compute_sha256_bytes()This function returns the raw binary digest (.digest()) instead of a hexadecimal string.Workflow
implementation example
Benchmarks , benchmark workflow and Timings
pc - 8gb ram DDR4 , 4gb amd gpu , ryzen 5600h
Before changes
This was run on local software ide
this was run on window powershell
After Changes
This was run on local ide
this was run on win powershell
i have also incorporated benchmarks for merkel_roots ,
benchmark_custom.pyLocally run using
python benchmark_custom.py "File path "Additional Notes:
#Final Architecture
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
New Features
Improvements
Tests