Skip to content

Conversation

cuiweixie
Copy link
Contributor

@cuiweixie cuiweixie commented Sep 22, 2025

Summary by CodeRabbit

  • Refactor
    • Streamlined internal hashing by using a shared empty-hash value and removing a redundant helper, with references updated accordingly.
    • No changes to public APIs or visible behavior; hashing for leaf and inner nodes remains unchanged.
    • Maintains compatibility with existing data and workflows; no user-facing impact.

@cuiweixie
Copy link
Contributor Author

cuiweixie commented Sep 22, 2025

#25358
@aljo242

Copy link
Contributor

coderabbitai bot commented Sep 22, 2025

📝 Walkthrough

Walkthrough

Refactors empty hash handling in store/internal/tree/hash.go by replacing a helper function with a package-level byte slice initialized from a hex string, updating call sites accordingly, and adding the required import. Other hashing logic remains unchanged.

Changes

Cohort / File(s) Summary
Hash empty value refactor
store/internal/tree/hash.go
Added import encoding/hex; introduced package-level variable emptyHash []byte from a hex string; replaced calls to emptyHash() with emptyHash; removed the old emptyHash() function; left leaf/inner node hashing logic unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly and accurately summarizes the primary change: precomputing the empty hash to save runtime cost. This directly maps to the changes in store/internal/tree/hash.go where the emptyHash() function is replaced by a precomputed emptyHash variable and call sites are updated. The phrasing is concise, specific, and relevant to the changeset.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
store/internal/tree/hash.go (2)

5-5: Drop hex import after switching to array precompute (or handle the error).

If you adopt the array approach, encoding/hex is unused; remove it. If you keep hex decoding, don’t ignore the error—validate at init and panic on malformed constant.

Apply this diff when using the array:

 import (
   "crypto/sha256"
-  "encoding/hex"
   "hash"
   "math/bits"
 )

If keeping hex:

-var emptyHash, _ = hex.DecodeString("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")
+var emptyHash = func() []byte {
+  b, err = hex.DecodeString("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")
+  if err != nil { panic(err) }
+  return b
+}()

21-31: Trim the generator comment; keep a terse, source-of-truth note.

The multi-line “generate from this code” block is noisy. Replace with a concise doc explaining the precomputation.

Apply this diff:

-// generate from this code
-//
-// 
-//  func emptyHash() []byte {
-//    h := sha256.Sum256([]byte{})
-//    return h[:]
-//  }
-//
-// 
-//  func main() {
-//    println(hex.EncodeToString(emptyHash()))
-//  }
+// emptyHashArr is sha256.Sum256(nil) precomputed once to avoid per-call hashing.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 9171179 and 75ee605.

📒 Files selected for processing (1)
  • store/internal/tree/hash.go (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary

@aljo242
Copy link
Contributor

aljo242 commented Sep 22, 2025

Use a precomputed [32]byte and copy on return (32B), which is safe and still cheap.

why cant we just do this ^^

@cuiweixie
Copy link
Contributor Author

Use a precomputed [32]byte and copy on return (32B), which is safe and still cheap.

why cant we just do this ^^

done

@aljo242
Copy link
Contributor

aljo242 commented Sep 23, 2025

I'd like to add a test to verify that this is the actual empty hash

@cuiweixie
Copy link
Contributor Author

I'd like to add a test to verify that this is the actual empty hash

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants