-
Notifications
You must be signed in to change notification settings - Fork 10
Refactor sector proof to use cached subtree roots #374
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
Conversation
a8eaf12 to
445a67e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors sector proof generation to use cached subtree roots at a fixed 64-leaf level, reducing disk I/O operations by allowing hosts to build proofs without reading the entire sector data. This optimization is particularly beneficial for reducing the amount of data read from disk during proof generation.
Key Changes:
- Introduces a specialized
sectorAccumulatorfor efficient Merkle tree computation - Adds
CachedSectorSubtreesto compute and cache 64-leaf subtree roots (4 KiB chunks) - Refactors
BuildSectorProofto construct proofs using cached subtrees and minimal segment data
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| rhp/v4/merkle.go | Adds caching infrastructure with sectorAccumulator, CachedSectorSubtrees, and refactored BuildSectorProof to use cached 64-leaf subtree roots |
| rhp/v4/merkle_test.go | Adds comprehensive test coverage for BuildSectorProof with various range scenarios including edge cases and randomized inputs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
445a67e to
89dce9e
Compare
|
LGTM. I agree that it's time to absorb the RHP2 and RHP3 packages into RHPv4. |
| var wg sync.WaitGroup | ||
| for i := range roots { | ||
| wg.Add(1) | ||
| go func(i int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This'll spin up 1024 goroutines per sector right? Isn't that an excessive amount? Obviously it would come at the cost of a little more complexity but perhaps we could batch these and use runtime.GOMAXPROCS or use a pool or something?
Refactors sector proofs to use cached subtree roots to reduce disk IO when building a proof for part of a sector. (SiaFoundation/hostd#876, SiaFoundation/coreutils#364)
(Yes, I copied
sectorAccumulatorin but it's probably time we remove the RHP2 and RHP3 packages in a follow up)@lukechampine feel free to take this over if you are unhappy with it.