[Access & Execution] Add flag to disable bitswap bloom cache#8227
[Access & Execution] Add flag to disable bitswap bloom cache#8227zhangchiqing wants to merge 13 commits intomasterfrom
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| ComplianceConfig: compliance.DefaultConfig(), | ||
| DhtSystemEnabled: true, | ||
| BitswapReprovideEnabled: true, | ||
| BitswapBloomCacheEnabled: true, // default: use cached blockstore TODO leo: change default to false |
There was a problem hiding this comment.
The test cases also passed when I changed the default to false.
fxamacker
left a comment
fxamacker
left a comment
There was a problem hiding this comment.
The condition for creating cachedBlockStore appears to be inverted. Specifically, we should check !SkipBloomCache before creating cachedBlockStore.
Also, it might be easier to follow if we use a positive flag UseBloomCache instead of a negative flag SkipBloomCache. This would make it more consistent with the new node flag BitswapBloomCacheEnabled.
Co-authored-by: Faye Amacker <33205765+fxamacker@users.noreply.github.com>
Co-authored-by: Faye Amacker <33205765+fxamacker@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a Bitswap bloom-cache toggle: new Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/node_builder.go (1)
189-195: BitswapBloomCacheEnabled field and default look consistent (minor doc nit)The new
BitswapBloomCacheEnabledflag and its default oftrueline up with the blob service’sUseBloomCachedefault and how builders consume it. One small suggestion: observers also read this flag via their builders, so consider updating the comment (“only meaningful to Access and Execution nodes”) to include Observer nodes to avoid future confusion. Optional only.Also applies to: 307-314
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/access/node_builder/access_node_builder.gocmd/execution_builder.gocmd/node_builder.gocmd/observer/node_builder/observer_builder.gocmd/scaffold.gointegration/tests/access/cohort3/execution_state_sync_test.gonetwork/p2p/blob/blob_service.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
network/p2p/blob/blob_service.gocmd/access/node_builder/access_node_builder.gocmd/execution_builder.gointegration/tests/access/cohort3/execution_state_sync_test.gocmd/node_builder.gocmd/scaffold.gocmd/observer/node_builder/observer_builder.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
network/p2p/blob/blob_service.go
{module,engine,cmd}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All major processing components must implement the
Componentinterface from/module/component/component.goto ensure consistent lifecycle management and graceful shutdown patterns
Files:
cmd/access/node_builder/access_node_builder.gocmd/execution_builder.gocmd/node_builder.gocmd/scaffold.gocmd/observer/node_builder/observer_builder.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests should be co-located with the code they test
Follow the existing pattern of*_test.gofiles for test naming
Use fixtures for realistic test data as defined in/utils/unittest/
Files:
integration/tests/access/cohort3/execution_state_sync_test.go
integration/tests/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Integration tests should go in
/integration/tests/
Files:
integration/tests/access/cohort3/execution_state_sync_test.go
🧠 Learnings (1)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives
Applied to files:
integration/tests/access/cohort3/execution_state_sync_test.go
🧬 Code graph analysis (4)
cmd/access/node_builder/access_node_builder.go (1)
network/p2p/blob/blob_service.go (1)
WithUseBloomCache(107-111)
cmd/execution_builder.go (1)
network/p2p/blob/blob_service.go (1)
WithUseBloomCache(107-111)
cmd/scaffold.go (1)
cmd/node_builder.go (1)
BaseConfig(140-197)
cmd/observer/node_builder/observer_builder.go (1)
network/p2p/blob/blob_service.go (1)
WithUseBloomCache(107-111)
⏰ 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). (37)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./integration/)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: analyze-code (go)
- GitHub Check: Docker Build
🔇 Additional comments (5)
integration/tests/access/cohort3/execution_state_sync_test.go (1)
200-216: ✅ Well-placed structured logging for better test observability.The logging additions capture relevant details (height, block ID, chunk count, errors) and are appropriately positioned:
- Error logging occurs before retry, aiding visibility into transient failures
- Success logging occurs after successful data conversion, confirming the full flow succeeded
The structured fields using zerolog are idiomatic for Flow Go. Since these logs are within the
retryNotFoundcallback, they may repeat on retries, which is expected and helpful for debugging integration test behavior.cmd/observer/node_builder/observer_builder.go (1)
1195-1205: Observer execution data service correctly wired to Bloom cache flagAppending
blob.WithUseBloomCache(builder.BitswapBloomCacheEnabled)to the options here aligns observer behavior with access and execution nodes and keeps the toggle centralized inBaseConfig. No issues spotted with this wiring.cmd/execution_builder.go (1)
398-418: Execution blob service now respects Bloom cache configurationAppending
blob.WithUseBloomCache(node.BitswapBloomCacheEnabled)ensures execution nodes honor the shared Bloom cache flag, consistent with other builders. The option placement alongside existing bitswap and reprovide options looks correct.cmd/access/node_builder/access_node_builder.go (1)
646-666: Access execution data service correctly wired to Bloom cache flagUsing
blob.WithUseBloomCache(builder.BitswapBloomCacheEnabled)here makes the execution data blob service on access nodes respect the shared Bloom cache toggle, mirroring execution and observer builders. The option ordering and usage look sound.cmd/scaffold.go (1)
216-219: Approved. The flag binding is correct and properly usesDefaultBaseConfig()which initializesBitswapBloomCacheEnabledtotrue, matching the PR intent. The implementation is consistent with other bitswap-related flags and the help text clearly documents the experimental feature.
| type BlobServiceConfig struct { | ||
| ReprovideInterval time.Duration // the interval at which the DHT provider entries are refreshed | ||
| BitswapOptions []bitswap.Option // options to pass to the Bitswap service | ||
| UseBloomCache bool // if true, use the bloom cache (cached blockstore), otherwise use plain blockstore | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd network/p2p/blob && wc -l blob_service.goRepository: onflow/flow-go
Length of output: 77
🏁 Script executed:
cat -n network/p2p/blob/blob_service.go | head -160Repository: onflow/flow-go
Length of output: 6605
🏁 Script executed:
# Verify the exact line where blockStore is used in CachedBlockstore call
sed -n '141,151p' network/p2p/blob/blob_service.goRepository: onflow/flow-go
Length of output: 354
🏁 Script executed:
# Double-check WithRateLimit implementation to confirm it modifies bs.blockStore
sed -n '95,100p' network/p2p/blob/blob_service.goRepository: onflow/flow-go
Length of output: 303
🏁 Script executed:
# Verify WithParentBlobService also modifies bs.blockStore
sed -n '75,80p' network/p2p/blob/blob_service.goRepository: onflow/flow-go
Length of output: 334
Fix CachedBlockstore to wrap the final blockstore after applying options
When UseBloomCache is true, CachedBlockstore is called with the local blockStore variable created on line 125, instead of bs.blockStore. Since options like WithRateLimit and WithParentBlobService mutate only bs.blockStore (not the local variable), their effects are silently discarded. For example:
- If
WithRateLimitis applied, it wrapsbs.blockStore, butCachedBlockstorethen wraps the unwrapped localblockStore, and the rate limiter is lost. - If
WithParentBlobServiceis applied, it replacesbs.blockStorewith the parent's store, butCachedBlockstorewraps the new localblockStoreinstead, breaking the parent relationship.
Change line 144 to use bs.blockStore instead of blockStore:
Proposed fix
if bs.config.UseBloomCache {
cachedBlockStore, err := blockstore.CachedBlockstore(
context.Background(),
- blockStore,
+ bs.blockStore,
blockstore.DefaultCacheOpts(),
)
if err != nil {
return nil, fmt.Errorf("failed to create cached blockstore: %w", err)
}
bs.blockStore = cachedBlockStore
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In network/p2p/blob/blob_service.go around lines ~125-145, the CachedBlockstore
call wraps the local variable blockStore instead of bs.blockStore, causing any
option-wrapping (e.g. WithRateLimit, WithParentBlobService) applied to
bs.blockStore to be ignored; change the CachedBlockstore invocation to use
bs.blockStore (the final, possibly-wrapped store) so that all option mutations
are preserved and the parent/RateLimit wrappers remain effective.
Pull request was closed
fxamacker
added
Preserve
|
Found 20 test failures on Blacksmith runners: Failures
|
6 participants
Add an experimental
bitswap-bloom-cache-enabled flag(default: true) that controls whether the Bitswap bloom cache is used for Access/Execution/Observer nodes. When disabled, the blob service uses a new WithSkipBloomCache option to skip the cached blockstore and instead use a plain Pebble-backed blockstore, avoiding the CPU cost of building bloom filters at startup while still relying on Pebble’s SSTable bloom filters.Summary by CodeRabbit
New Features
Chores