Skip to content

refactor(consensus/proposal): encapsulate ordering rules in store#3675

Merged
RafaelGranza merged 4 commits into
mainfrom
granza/proposal-store-encapsulate-ordering
May 26, 2026
Merged

refactor(consensus/proposal): encapsulate ordering rules in store#3675
RafaelGranza merged 4 commits into
mainfrom
granza/proposal-store-encapsulate-ordering

Conversation

@RafaelGranza
Copy link
Copy Markdown
Contributor

@RafaelGranza RafaelGranza commented May 24, 2026

Summary (ISSUE)

ProposalStore was leaking coordination rules (ordering, what's safe to write/delete and when) across call sites. This refactor pushes those rules into the type itself so new callers can't violate them.

What changed

  • Storage layout: flat sync.Map[Hash]*BR to nested sync.Map[Height]*sync.Map[Hash]*BR.
  • Finalized cursor: atomic.Uint64 advanced monotonically via CAS in DeleteUpToHeight.
  • Store: no-op if height <= finalized; recheck-then-self-clean after publish to handle races with a concurrent sweep.
  • Get: late height <= finalized check before returning, so readers never observe entries past the cursor even during a race.
  • IsFinalized(height): utility for callers that want to distinguish cache miss from finalized-height query.
  • DeleteUpToHeight: now O(num_heights). Drops outer entries; GC handles the inner sync.Maps.

The public API (Get(hash), Store(hash, br), DeleteUpToHeight(h)) is unchanged for callers; no signature changes leak into the tendermint.Application interface.

Design note

There was a real trade-off in how to balance cost between Get and Delete:

Layout Get Delete
Flat sync.Map[Hash]*BR O(1) O(N) where N = total entries
Nested sync.Map[Height]*sync.Map[Hash]*BR (this PR) O(num_heights) O(num_heights)

Picked the nested layout because num_heights is small in practice (1 to 3 in steady state, up to ~5 with stragglers), so the O(num_heights) Get is sub-100ns and invisible end-to-end (network and VM latency dominate). The benchmarks below confirm the Get hit stays in the tens of nanoseconds.

The flip side is meaningful: Delete cost is decoupled from total entry count. That's where the big speedup comes from.

Benchmark comparison

Apple M-series, go test -bench -benchmem. Spread across 5 live heights to reflect realistic steady-state consensus.

Workload New (bucketed) Legacy (flat) Delta
StoreThenGet (seq) 586ns 558ns ~noise
ParallelGet 24ns 12ns ~2x slower, sub-30ns
ParallelMixed (10% writes) 51ns 31ns ~1.6x slower, sub-100ns
DeleteUpToHeight (16 heights x 64 hashes) 1.6µs 101µs ~62x faster

The benchmark file (proposal_store_bench_test.go) is included as requested for future comparisons.

@RafaelGranza RafaelGranza self-assigned this May 24, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 24, 2026

Codecov Report

❌ Patch coverage is 84.21053% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.34%. Comparing base (e27e9f5) to head (617a7eb).

Files with missing lines Patch % Lines
consensus/proposal/proposal_store.go 83.78% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3675      +/-   ##
==========================================
- Coverage   76.38%   76.34%   -0.05%     
==========================================
  Files         396      396              
  Lines       36579    36596      +17     
==========================================
- Hits        27941    27938       -3     
- Misses       6664     6682      +18     
- Partials     1974     1976       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread consensus/proposal/proposal_store.go Outdated
Comment thread consensus/proposal/proposal_store.go Outdated
Comment thread consensus/proposal/proposal_store.go
Comment thread consensus/proposal/proposal_store.go Outdated
Comment thread consensus/proposal/proposal_store.go Outdated
Comment thread consensus/proposal/proposal_store.go
Comment thread consensus/proposal/proposal_store.go Outdated
Comment thread consensus/proposal/proposal_store.go Outdated
Comment thread consensus/proposal/proposal_store_bench_test.go Outdated
Comment thread consensus/proposal/proposal_store.go Outdated
Comment thread consensus/proposal/proposal_store.go Outdated
@rodrodros
Copy link
Copy Markdown
Contributor

@RafaelGranza the benchmark only shows performance data, but there is no memory usage benchmark, could you please add them as well

Comment thread consensus/proposal/proposal_store.go Outdated
Comment thread consensus/proposal/proposal_store.go Outdated
Comment thread consensus/proposal/proposal_store.go Outdated
Comment thread consensus/proposal/proposal_store.go Outdated
Comment thread consensus/proposal/proposal_store.go Outdated
@RafaelGranza RafaelGranza force-pushed the granza/proposal-store-encapsulate-ordering branch from 81ee300 to 3652e55 Compare May 25, 2026 20:09
@RafaelGranza RafaelGranza requested a review from rodrodros May 25, 2026 20:44
@ongyimeng
Copy link
Copy Markdown
Contributor

Nice refactor, the abstraction looks cleaner with these rules inside ProposalStore. I also like the atomic cursor + sync.Map approach, the concurrency design is quite elegant.

Copy link
Copy Markdown
Contributor

@rodrodros rodrodros left a comment

Choose a reason for hiding this comment

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

LGTM! Merge at will

@RafaelGranza RafaelGranza force-pushed the granza/proposal-store-encapsulate-ordering branch from 6112d00 to 33a4a22 Compare May 26, 2026 13:23
@RafaelGranza RafaelGranza enabled auto-merge (squash) May 26, 2026 13:24
@RafaelGranza RafaelGranza merged commit abba44b into main May 26, 2026
27 checks passed
@RafaelGranza RafaelGranza deleted the granza/proposal-store-encapsulate-ordering branch May 26, 2026 14:03
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.

3 participants