Skip to content

refactor: extract subtree root computation - #336

Open
loglapa wants to merge 2 commits into
celestiaorg:mainfrom
loglapa:refactor-proof-compute-root-261
Open

refactor: extract subtree root computation#336
loglapa wants to merge 2 commits into
celestiaorg:mainfrom
loglapa:refactor-proof-compute-root-261

Conversation

@loglapa

@loglapa loglapa commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Closes #261

Summary

  • extract subtree-root reconstruction from its recursive closure
  • keep proof-node and subtree-root consumption explicit through shared cursors
  • preserve validation, error wrapping, and hashing behavior

Testing

  • go test ./... -run 'TestVerifySubtreeRootInclusion|TestToLeafRanges|TestNextLeafRange' -count=1
  • go test ./...
  • go build ./...
  • go vet ./...
  • golangci-lint run --timeout 10m
  • git diff --check

Signed-off-by: Vladislav Lapin <v.lapin@scalablesolutions.io>
@loglapa
loglapa marked this pull request as ready for review August 20, 2026 10:41
@loglapa
loglapa requested a review from a team as a code owner August 20, 2026 10:41
@loglapa
loglapa requested review from vgonkivs and removed request for a team August 20, 2026 10:41
@loglapa

loglapa commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

The fork workflows are still awaiting maintainer approval (Go, lint, buf, and proto generation). I attempted to approve each workflow run, but GitHub restricts that action to repository admins (403 Must have admin rights). A maintainer approval is the only remaining step before CI can run.

@vgonkivs vgonkivs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Changes visually look correct to me. One thing before we procced: can we compare the two approaches with a benchmark, since the closure was originally kept inline as a possible optimization. Could you add benchstat before/after numbers for VerifySubtreeRootInclusion (old closure vs. extracted method) to the PR? I expect parity - the recursion is dominated by HashNode/SHA-256 - but let's have the numbers

@loglapa

loglapa commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Added a reproducible BenchmarkVerifySubtreeRootInclusion in d3f396c and compared the parent implementation (inline closure) against this branch (extracted method) with 15 samples per version on Apple M5 / darwin-arm64:

                              inline closure    extracted method    vs base
sec/op                        1.797µ ± 2%       1.807µ ± 1%        ~ (p=0.129 n=15)
B/op                          1.234Ki ± 0%      1.234Ki ± 0%       ~ (p=1.000 n=15)
allocs/op                     20.00 ± 0%        20.00 ± 0%         ~ (p=1.000 n=15)

Command: go test -run "^$" -bench "^BenchmarkVerifySubtreeRootInclusion$" -benchmem -count=15 -benchtime=500ms, compared with official benchstat. There is no statistically significant runtime difference and allocations are identical. go test ./..., go vet ./..., and git diff --check pass. A full race run still finds the pre-existing Test_Root_RaceCondition race; the same targeted race test fails on the parent implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

extract computeRoot to a separate method

2 participants