Differentiate between Consensus and Cluster Headers storage#8222
Differentiate between Consensus and Cluster Headers storage#8222
Conversation
Weakens the chainID requirement for cluster chains when reading from storage.
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! |
module/builder/collection/builder.go
Outdated
|
|
||
| for _, blockID := range clusterBlockIDs { | ||
| header, err := b.clusterHeaders.ByBlockID(blockID) | ||
| header, err := b.clusterHeaders.ByBlockID(blockID) // TODO(4204) transaction deduplication crosses clusterHeaders epoch boundary |
There was a problem hiding this comment.
Transaction de-duplication actually does not occur across cluster and epoch boundaries.
- Each transaction is uniquely assigned to one cluster in one epoch, based on the transaction's reference block (see ingestion logic)
- Therefore, each cluster has a range of reference block heights they can accept. These ranges are equivalent to the height range of blocks within an epoch (
$[FirstBlockInEpoch.Height, LastBlockInEpoch.Height]$ . These ranges are consecutive and do not overlap.- In short, if we are considering a cluster block with reference block height
$FirstBlockInEpoch.Height$ , thenminRefHeightis actually$FirstBlockInEpoch.Height$ (we don't need to search further back).
- In short, if we are considering a cluster block with reference block height
- We already take this into account when determining the lowest possible reference block
So I think we can remove this TODO, and remove the special-case logic in storage.Headers meant to work around this. I would also suggest adding some documentation here explaining why there is no overlap between clusters and epochs.
There was a problem hiding this comment.
Thank you for clarifying this explicitly. Now that I understand better what's going on, I can say that while you are right that that's what should be happening, it is not, due to two bugs 🐞🐞. The existing behaviour did actually look across the epoch boundary (now causing clustover_switchover_test to fail), because:
- 🐞
ctx.refEpochFirstHeightis never initialized in the block builder, so defaults to 0. (Introduced in Enforce that collection reference blocks are bound to the cluster's operating epoch #4148 and not caught in Removes unused first height field from LN builder #6828)
Essentially, theminRefHeightdid not get clamped to the start of the epoch, and was alwaysLastFinalizedBlockInEpoch.Height - DefaultTransactionExpiry. - In addition, the minimum height of the range is decreased twice - once in
lowestPossibleReferenceBlockHeight()(respecting the epoch boundary if not for bug 1) and then again infindRefHeightSearchRangeForConflictingClusterBlocks(🐞 not respecting the epoch boundary).
I believe this means that this particular lookup was always checking approximately the past ~1200 reference heights (flow.DefaultTransactionExpiry * 2), regardless of which epoch those heights were in.
While the first bug also affects payload construction, I don't think it has impacted correctness (essentially transactions from the previous epoch would not immediately be considered expired, but they are already split into pools by epoch anyways as you have noted, so transactions from a different epoch will not be encountered.)
There was a problem hiding this comment.
You're right. Oops, that was introduced by me 🫣.
Here's what I think we should do:
- bring back the logic that populates
refEpochFirstHeight- Check for
ErrNotFound-- if we see that error, then setrefEpochFirstHeightto zero. This means that we have just joined the network, and that our local state cutoff (our root block) is newer thanrefEpochFirstHeight.
- Check for
There was a problem hiding this comment.
done in e397124 - does the comment on expected behaviour from setting refEpochFirstHeight = 0 match with what you expect?
There was a problem hiding this comment.
To double-check: the minimum reference height to check for a duplicated transaction is approximately finalizedHeight - 2*DefaultTransactionExpiry, because of the following situation:
Collection C1 C2
┏━━━━━━━━━━━┫
Transaction T1 T2
┊ ┊
Reference Block R1──────────R2──────────Head
<-Expiry-> <-Expiry->
The new collection C2 could barely include transaction T2; to ensure we deduplicate correctly, we need to check collection C1, which has reference block R1 (because it could just barely include transaction T1).
There was a problem hiding this comment.
@tim-barry I would go off this function and this documentation.
When we are inserting C2, the range we need to check is [C2MinRefHeight-Expiry, C2MaxRefHeight]
C2MinRefHeight is both the reference block height of C2, and the smallest reference block height of all transactions in C2. C2MaxRefHeight is the largest reference block height of all transactions in C2.
The check was unintentionally crossing an epoch boundary and retrieving headers from a previous cluster chain. In addition, `ctx.refEpochFirstHeight` was never initialized. See #8222 (comment) for details
tim-barry
requested review from
zhangchiqing
and removed request for
zhangchiqing
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/store/headers.go (1)
312-323: Missing documentation for cluster error case.For cluster headers,
BlockIDByViewwill returnErrNotAvailableForClusterConsensusvia the viewCache, but this isn't documented. The doc only mentionsErrNotFound.📝 Suggested doc update
// BlockIDByView returns the block ID that is certified at the given view. It is an optimized // version of `ByView` that skips retrieving the block. Expected errors during normal operations: // - [storage.ErrNotFound] if no certified block is known at given view. +// - [storage.ErrNotAvailableForClusterConsensus] if called on a cluster Headers instance // // NOTE: this method is not available until next spork (mainnet27) or a migration that builds the index.
🤖 Fix all issues with AI agents
In @storage/store/headers.go:
- Around line 175-180: The doc comment above the method retrieveProposalTx
incorrectly names the function "retrieveTx"; update the comment to reference the
correct method name retrieveProposalTx and ensure the description still matches
this method's behavior (returning a proposal header and proposer signature) and
lists the same expected errors; adjust any other comment occurrences around the
Headers type or this function to use retrieveProposalTx consistently.
🧹 Nitpick comments (3)
storage/store/headers.go (3)
43-46: Duplicate lock validation withoperation.InsertHeader.The lock check here duplicates the validation already performed inside
operation.InsertHeader(which returns an error if the lock isn't held). While defensive coding is reasonable, this adds maintenance overhead if lock requirements change.Consider removing the redundant check here, or document why the pre-check is intentional (e.g., to fail faster before any other operations).
84-87: Misleading parameter name:heightshould beview.The parameter is named
heightbut represents a view value. This is a no-op function returning an error, but the parameter name should match semantics for clarity and consistency withretrieveViewtype signature.♻️ Suggested fix
- retrieveView := func(r storage.Reader, height uint64) (flow.Identifier, error) { + retrieveView := func(r storage.Reader, view uint64) (flow.Identifier, error) { var id flow.Identifier return id, storage.ErrNotAvailableForClusterConsensus }
99-110: Minor: Unnecessary error variable in return.On line 109,
erris guaranteed to benilat this point since line 103 returns early on error. Returningnilexplicitly would be clearer.♻️ Suggested fix
if header.ChainID != chainID { return nil, fmt.Errorf("expected chain ID %v, got %v: %w", chainID, header.ChainID, storage.ErrWrongChain) } - return &header, err + return &header, nil
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
storage/store/headers.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
storage/store/headers.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
storage/store/headers.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
storage/store/headers.go
🧠 Learnings (5)
📚 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 **/*.go : All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Applied to files:
storage/store/headers.go
📚 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 **/*.go : Use comprehensive error wrapping for debugging; avoid `fmt.Errorf`, use `irrecoverable` package for exceptions
Applied to files:
storage/store/headers.go
📚 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:
storage/store/headers.go
📚 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 **/*.go : Implement proper interfaces before concrete types
Applied to files:
storage/store/headers.go
📚 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 {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
storage/store/headers.go
🧬 Code graph analysis (1)
storage/store/headers.go (5)
storage/headers.go (1)
Headers(15-65)state/cluster/root_block.go (1)
IsCanonicalClusterID(26-28)storage/errors.go (2)
ErrWrongChain(37-37)ErrNotAvailableForClusterConsensus(41-41)storage/locks.go (2)
LockInsertBlock(14-14)LockInsertOrFinalizeClusterBlock(22-22)storage/operation/headers.go (5)
InsertHeader(26-42)LookupBlockHeight(142-144)LookupCertifiedBlockByView(149-151)InsertClusterHeader(57-73)RetrieveHeader(79-81)
⏰ 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 (./integration/)
- GitHub Check: Lint (./)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (9)
storage/store/headers.go (9)
18-27: LGTM!The
chainIDfield addition to theHeadersstruct is appropriate for binding header storage to a specific chain.
134-161: LGTM!The
storeTxmethod correctly delegates chain ID validation to thestoreWithLockclosure, and the documentation is properly updated to reflect the newErrWrongChainerror.
163-173: LGTM!Chain ID validation is correctly performed via the retrieve closure in the cache layer.
201-207: LGTM!Documentation correctly updated to reflect chain ID validation.
209-216: LGTM!Documentation correctly updated.
218-228: LGTM!Height lookup correctly uses chain-specific index, and chain ID validation occurs during header retrieval.
230-244: LGTM!The
ByViewmethod correctly returnsErrNotAvailableForClusterConsensusfor cluster headers via the retrieveView closure.
246-260: Consider implications ofExistsnot validating chain ID.The documentation explicitly states this method doesn't distinguish between cluster and consensus headers. While this is clearly documented, callers using
Existsfollowed byByBlockIDcould get inconsistent results:
Exists(blockID)returnstrue(header exists)ByBlockID(blockID)returnsErrWrongChain(wrong chain)If this is intentional for performance reasons (avoiding header deserialization), the current documentation is sufficient. Otherwise, consider whether this asymmetry could cause subtle bugs in calling code.
273-310: LGTM!Children headers are correctly validated via
ByBlockID, which enforces chain ID matching.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/store/headers.go (1)
251-265: Useirrecoverable.NewExceptionfinstead offmt.Errorfat line 262.The method documents "No errors are expected during normal operation" (line 254), which per Flow's coding conventions indicates an exception scenario. Exceptions must be wrapped with
irrecoverable.NewExceptionf, notfmt.Errorf. This is consistent with how callers handle this method's errors—for example,receipt_validator.go:248wraps the result withirrecoverable.NewExceptionf.Regarding the cross-chain visibility: The documented behavior is intentional. Call sites (
receipt_validator.go:246,compliance/core.go:205,212,state.go:910) appropriately handle the lack of chain distinction by checking existence and treating the result as a binary fact independent of which chain the header came from.
🤖 Fix all issues with AI agents
In @storage/store/headers.go:
- Around line 99-110: In the retrieve closure (function retrieve) change the
final return to explicitly return nil for the error instead of reusing the
variable err; locate the function named retrieve (and the call to
operation.RetrieveHeader) and replace the trailing "return &header, err" with
"return &header, nil" so the success path clearly returns a nil error.
- Around line 175-180: The doc comment above retrieveProposalTx refers to the
wrong method name "retrieveTx"; update the comment to correctly reference
"retrieveProposalTx" and ensure the description matches the method (e.g.,
"retrieveProposalTx returns a proposal header..."), leaving the rest of the
comment (expected errors and behavior) unchanged so it accurately documents the
retrieveProposalTx function.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
storage/store/headers.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
storage/store/headers.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
storage/store/headers.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
storage/store/headers.go
🧠 Learnings (5)
📚 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 **/*.go : All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Applied to files:
storage/store/headers.go
📚 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 **/*.go : Use comprehensive error wrapping for debugging; avoid `fmt.Errorf`, use `irrecoverable` package for exceptions
Applied to files:
storage/store/headers.go
📚 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:
storage/store/headers.go
📚 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 **/*.go : Implement proper interfaces before concrete types
Applied to files:
storage/store/headers.go
📚 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 {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
storage/store/headers.go
🧬 Code graph analysis (1)
storage/store/headers.go (5)
storage/headers.go (1)
Headers(15-65)state/cluster/root_block.go (1)
IsCanonicalClusterID(26-28)storage/errors.go (2)
ErrWrongChain(37-37)ErrNotAvailableForClusterConsensus(41-41)storage/locks.go (2)
LockInsertBlock(14-14)LockInsertOrFinalizeClusterBlock(22-22)storage/operation/headers.go (3)
InsertHeader(26-42)InsertClusterHeader(57-73)RetrieveHeader(79-81)
⏰ 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 (engine/access)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (7)
storage/store/headers.go (7)
18-27: LGTM!The
chainIDfield is appropriately added as an immutable field set at construction time, ensuring thread-safe access without additional synchronization.
35-58: LGTM!The constructor correctly validates that non-cluster chain IDs are used, and the
storeWithLockclosure properly validates both chain ID consistency and lock ownership before delegating tooperation.InsertHeader. The lock validation at lines 43-45 provides defense-in-depth alongside the check inInsertHeader.
61-89: LGTM!The cluster constructor correctly validates cluster chain IDs, uses the appropriate cluster-specific operations (
InsertClusterHeader,LookupClusterBlockHeight), and properly returnsErrNotAvailableForClusterConsensusfor view-based lookups, which aligns with the interface contract.
134-161: LGTM!The documentation correctly documents the new
ErrWrongChainerror condition, and the implementation properly delegates chain validation to thestoreWithLockclosure configured at construction.
192-204: LGTM!The documentation clearly explains the chain-safety guarantee: the height cache is populated using a chain-specific database index, so only blocks of the respective chain can be returned. This is a valuable clarification.
337-369: No action needed. The executed block index is intentionally designed to store potentially non-canonical blocks (including fork blocks), as documented instorage/operation/executed.go. Chain validation inRollbackExecutedBlockwould be incorrect here; the lack of validation is proper by design.Likely an incorrect or invalid review comment.
330-335: Deprecation ofFindHeadersis appropriate and safe.Marking this method as deprecated with the reason "Undocumented, hence unsafe for public use" is justified. This method is not part of the
storage.Headersinterface and correctly bypasses chain validation—operation.FindHeadersiterates through all headers in the database without checkingChainID, unlike all interface methods (ByBlockID, ByHeight, ByView, etc.) which explicitly validate chain membership. The method has no usages in the codebase and provides unrestricted cross-chain header access, making it unsafe for public consumption.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
storage/store/blocks.go (1)
127-127: Minor grammar nit: "Expected errors returns" is awkward.The phrase appears on multiple lines (127, 138, 149, 162, 174, 187, 204, 224). Consider using "Expected error returns" (consistent with line 58) or "Expected errors returned" for grammatical consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
storage/cluster_blocks.gostorage/operation/heights_test.gostorage/store/blocks.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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:
storage/cluster_blocks.gostorage/operation/heights_test.gostorage/store/blocks.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
storage/cluster_blocks.gostorage/operation/heights_test.gostorage/store/blocks.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
storage/cluster_blocks.gostorage/operation/heights_test.gostorage/store/blocks.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:
storage/operation/heights_test.go
🧬 Code graph analysis (1)
storage/operation/heights_test.go (2)
storage/operation/heights.go (3)
RetrieveFinalizedHeight(46-58)IncompleteStateError(21-21)RetrieveSealedHeight(82-94)storage/errors.go (1)
ErrNotFound(17-17)
⏰ 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 (./)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./integration/)
- 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: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (6)
storage/cluster_blocks.go (1)
20-20: LGTM!The documentation updates correctly use the Go 1.19+ godoc link syntax
[storage.ErrNotFound]to create hyperlinks to the error type in generated documentation. This is a nice improvement for documentation consistency across the codebase.Also applies to: 29-29
storage/operation/heights_test.go (2)
17-27: LGTM!The test correctly validates the sentinel error behavior for non-bootstrapped databases. The dual assertion (
ErrorIsforIncompleteStateErrorandNotErrorIsforErrNotFound) properly verifies that the error wrapping masks the low-levelErrNotFoundas intended.
29-39: LGTM!Symmetric test for sealed height retrieval. Together with
TestNonBootstrappedRetrieveFinalizedHeight, this provides complete coverage for theIncompleteStateErrorsentinel behavior on non-bootstrapped databases.storage/store/blocks.go (3)
42-42: Documentation correctly reflects newErrWrongChainbehavior for ID-based lookups.The addition of
storage.ErrWrongChainto the documented error returns is appropriate for methods that accept arbitrary block IDs from callers (BatchStore,retrieve,retrieveProposal,ByID,ProposalByID). These methods can legitimately encounter blocks from a different chain than the one the storage instance is bound to.Also applies to: 60-60, 93-93, 127-129, 138-140
149-157: Verify:ErrWrongChaincould propagate fromByIDbut is not documented.
ByViewcallsByID(blockID)on line 156, which documentsErrWrongChainas an expected error. If the view index ever contains a block ID from a different chain (e.g., due to data corruption), this error would propagate upward.The current documentation assumes the view index is always consistent with the bound chain, which should hold in normal operation. If this is the intended contract, consider adding a brief comment clarifying this assumption, or documenting that
ErrWrongChainis a possible (though unexpected) return.
204-225: Inconsistency with AI summary regardingErrWrongChaindocumentation.The AI summary states
ErrWrongChainwas documented forByCollectionIDandBlockIDByCollectionID, but the actual documentation only listsErrNotFound.If this is intentional (collection lookups use chain-specific indices), the current documentation is correct. Please confirm this matches the expected behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
storage/store/headers.go (2)
99-110: Minor: Redundant error return on line 109.The
errvariable is guaranteed to benilafter the check on line 102-104. Returningerron line 109 is technically correct but slightly misleading.✨ Suggested improvement
- return &header, err + return &header, nil
330-335: Deprecation notice added to FindHeaders.Good addition of the deprecation notice. Consider also adding a
// Deprecated:comment that Go tooling recognizes.✨ Suggested improvement for tooling recognition
-// Deprecated: Undocumented, hence unsafe for public use. +// Deprecated: FindHeaders is undocumented, hence unsafe for public use. +// This method does not filter by chainID and may return headers from any chain.engine/collection/epochmgr/factories/cluster_state.go (1)
74-78: Verify the error return on the success path.Line 78 returns
errwhich is the error fromNewMutableStateon line 74. IfNewMutableStatesucceeds,errisnil, so this is correct. However, the pattern is inconsistent with earlier returns that explicitly returnnilfor the error on success paths (not shown in this diff, but typical Go style).For consistency, consider returning
nilexplicitly on the success path:✨ Suggested improvement for explicit nil return
- return mutableState, clusterHeaders, clusterPayloads, clusterBlocks, consensusHeaders, err + return mutableState, clusterHeaders, clusterPayloads, clusterBlocks, consensusHeaders, nilThis makes it clearer that this is a success path. However, returning
erris not incorrect since it will benilafter the error check on line 75-77.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
engine/collection/epochmgr/engine_test.goengine/collection/epochmgr/factories/cluster_state.gostorage/store/headers.go
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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:
engine/collection/epochmgr/factories/cluster_state.gostorage/store/headers.goengine/collection/epochmgr/engine_test.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:
engine/collection/epochmgr/factories/cluster_state.goengine/collection/epochmgr/engine_test.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
engine/collection/epochmgr/factories/cluster_state.goengine/collection/epochmgr/engine_test.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
engine/collection/epochmgr/factories/cluster_state.goengine/collection/epochmgr/engine_test.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
storage/store/headers.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
storage/store/headers.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:
engine/collection/epochmgr/engine_test.go
🧠 Learnings (5)
📚 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:
engine/collection/epochmgr/factories/cluster_state.gostorage/store/headers.go
📚 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 **/*.go : All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Applied to files:
storage/store/headers.go
📚 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 **/*.go : Use comprehensive error wrapping for debugging; avoid `fmt.Errorf`, use `irrecoverable` package for exceptions
Applied to files:
storage/store/headers.go
📚 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 **/*.go : Implement proper interfaces before concrete types
Applied to files:
storage/store/headers.go
📚 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 {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
storage/store/headers.go
🧬 Code graph analysis (1)
engine/collection/epochmgr/engine_test.go (2)
model/flow/chain.go (1)
ChainID(14-14)state/protocol/mock/params.go (1)
NewParams(171-181)
⏰ 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 (./integration/)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (9)
engine/collection/epochmgr/engine_test.go (4)
70-73: LGTM! Cluster state params mock properly wired.The mock correctly sets up
ChainID()on cluster params and wires it throughcomponents.state.On("Params"). The.Maybe()calls are appropriate since not all tests may exercise these paths.
111-140: LGTM! Factory mock correctly updated for new arity.The
Createmock now acceptsmock.Anythingfor theconsensusChainIDparameter, and all returned provider functions are updated to take(epoch realprotocol.CommittedEpoch, consensusChainID flow.ChainID)as parameters. The implementation correctly ignores theconsensusChainIDin tests since the mock components don't need chain-specific behavior.
167-169: LGTM! Protocol state params mock properly configured.The test setup correctly creates a
protocol.Paramsmock and configuresChainID()to return a test chain ID. This aligns with the engine's new requirement to retrieve the consensus chain ID from protocol state params.
278-282: LGTM! Unauthorized node mock correctly updated.The
unauthorizedMatcheris properly applied withmock.Anythingfor theconsensusChainIDparameter, maintaining consistency with the updatedCreatesignature.storage/store/headers.go (3)
31-59: LGTM! Consensus headers constructor with proper validation.The constructor correctly:
- Validates that the chainID is not a cluster ID using
cluster.IsCanonicalClusterID- Returns an
irrecoverableexception for misconfiguration- Validates chainID consistency in
storeWithLock- Requires proper lock acquisition before insertion
61-89: LGTM! Cluster headers constructor with appropriate limitations.The constructor correctly:
- Validates that the chainID is a canonical cluster ID
- Uses cluster-specific operations (
InsertClusterHeader,LookupClusterBlockHeight)- Returns
storage.ErrNotAvailableForClusterConsensusfor view lookups (as cluster consensus doesn't support this)- Requires
LockInsertOrFinalizeClusterBlockinstead ofLockInsertBlock
251-253: Documentation accurately reflects the method's cross-chain behavior.The
Existsmethod's documentation correctly notes that it does not distinguish between cluster and consensus headers because it checks block existence by ID without loading the header type. Examination of call sites across the codebase confirms proper usage: consensus nodes call this method for consensus headers, collection nodes for cluster headers, and the underlying store architecture prevents cross-chain confusion. All callers properly handle errors.engine/collection/epochmgr/factories/cluster_state.go (2)
37-55: LGTM! Factory correctly creates chain-specific headers.The factory now properly separates concerns:
clusterHeadersfor cluster block headers (usingNewClusterHeaders)consensusHeadersfor reference block lookups (usingNewHeaders)The comment on line 52 clearly explains the purpose of
consensusHeadersfor reference blocks.
63-66: LGTM! OpenState updated with cluster-specific storage.The call to
clusterkv.OpenStatenow correctly receivesclusterHeadersandclusterPayloadsinstead of the previous generic headers/payloads, ensuring cluster state uses the appropriate chain-bound storage.
AlexHentschel
left a comment
AlexHentschel
left a comment
There was a problem hiding this comment.
👏 Very nice and clear Thanks for grinding through the large change surface.
cmd/util/cmd/export-json-transactions/transactions/range_test.go
Outdated
Show resolved
Hide resolved
Prefer constructor if present Co-authored-by: Alexander Hentschel <alex.hentschel@flowfoundation.org>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/util/cmd/read-badger/cmd/epoch_protocol_state.go (1)
28-32: Variable shadows imported package name.The variable
metricson line 28 shadows the importedmetricspackage. This can cause confusion and potential issues if someone tries to use the package elsewhere in the function.♻️ Suggested fix
- metrics := metrics.NewNoopCollector() - setups := store.NewEpochSetups(metrics, db) - epochCommits := store.NewEpochCommits(metrics, db) - epochProtocolStateEntries := store.NewEpochProtocolStateEntries(metrics, setups, epochCommits, db, + collector := metrics.NewNoopCollector() + setups := store.NewEpochSetups(collector, db) + epochCommits := store.NewEpochCommits(collector, db) + epochProtocolStateEntries := store.NewEpochProtocolStateEntries(collector, setups, epochCommits, db, store.DefaultEpochProtocolStateCacheSize, store.DefaultProtocolStateIndexCacheSize)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
admin/commands/storage/read_range_cluster_blocks.gocmd/util/cmd/read-badger/cmd/blocks.gocmd/util/cmd/read-badger/cmd/collections.gocmd/util/cmd/read-badger/cmd/epoch_commit.gocmd/util/cmd/read-badger/cmd/epoch_protocol_state.gocmd/util/cmd/read-badger/cmd/guarantees.gocmd/util/cmd/read-badger/cmd/protocol_kvstore.gocmd/util/cmd/read-badger/cmd/seals.gocmd/util/cmd/read-badger/cmd/transactions.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/util/cmd/read-badger/cmd/guarantees.go
- cmd/util/cmd/read-badger/cmd/protocol_kvstore.go
- cmd/util/cmd/read-badger/cmd/blocks.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
cmd/util/cmd/read-badger/cmd/epoch_protocol_state.goadmin/commands/storage/read_range_cluster_blocks.gocmd/util/cmd/read-badger/cmd/transactions.gocmd/util/cmd/read-badger/cmd/collections.gocmd/util/cmd/read-badger/cmd/seals.gocmd/util/cmd/read-badger/cmd/epoch_commit.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/util/cmd/read-badger/cmd/epoch_protocol_state.gocmd/util/cmd/read-badger/cmd/transactions.gocmd/util/cmd/read-badger/cmd/collections.gocmd/util/cmd/read-badger/cmd/seals.gocmd/util/cmd/read-badger/cmd/epoch_commit.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 {module,engine,cmd}/**/*.go : All major processing components must implement the `Component` interface from `/module/component/component.go` to ensure consistent lifecycle management and graceful shutdown patterns
Applied to files:
admin/commands/storage/read_range_cluster_blocks.go
🧬 Code graph analysis (3)
cmd/util/cmd/read-badger/cmd/epoch_protocol_state.go (1)
storage/store/epoch_protocol_state.go (2)
DefaultEpochProtocolStateCacheSize(19-19)DefaultProtocolStateIndexCacheSize(23-23)
admin/commands/storage/read_range_cluster_blocks.go (3)
storage/store/cluster_payloads.go (1)
ClusterPayloads(16-19)storage/store/headers.go (1)
NewClusterHeaders(66-89)model/flow/chain.go (1)
ChainID(14-14)
cmd/util/cmd/read-badger/cmd/transactions.go (1)
storage/inmemory/transactions_reader.go (1)
NewTransactions(14-23)
⏰ 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 (./integration/)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (10)
admin/commands/storage/read_range_cluster_blocks.go (2)
53-59: LGTM! Runtime creation of cluster headers aligns with the PR objectives.The approach of creating
clusterHeadersat request time is correct since thechainIDis only available from the request parameters. The error fromNewClusterHeadersis properly handled, andNewClusterHeadersinternally validates that the provided chainID is a valid cluster chain ID (returning an error for non-cluster chain IDs).
29-34: Constructor simplification looks good.Removing the
headersparameter from the constructor is appropriate since cluster headers are now created at runtime with the request-specific chainID. This aligns with the PR's goal of binding ChainID to each Headers instance at construction time.cmd/util/cmd/read-badger/cmd/collections.go (3)
33-34: Clean migration to store-based abstractions.The refactor from
storages-based access to the newstore.NewTransactionsandstore.NewCollectionsconstructors is consistent with the broader PR changes. Error handling for all subsequent collection operations is properly implemented.
47-61: Method calls correctly updated.The
collections.LightByIDandcollections.ByIDcalls properly replace the previousstorages.Collections.*pattern. Error handling is preserved.
72-78: Transaction-based lookup updated correctly.The
collections.LightByTransactionIDcall maintains the same error handling pattern as before.cmd/util/cmd/read-badger/cmd/transactions.go (1)
28-37: Consistent refactor to store-based transactions.The change from
storages-based access tostore.NewTransactionsfollows the same pattern ascollections.go. Thetransactions.ByIDcall and error handling are correctly implemented.Note: The
storage/inmemory/transactions_reader.gosnippet shows a differentNewTransactionsconstructor (for in-memory testing with transaction bodies), which is in a separate package and serves a different purpose—no conflict here.cmd/util/cmd/read-badger/cmd/seals.go (1)
30-30: LGTM!The migration to the store-based initialization with
store.NewSealsis clean and correct. Usingmetrics.NewNoopCollector()is appropriate for this read-only CLI utility.cmd/util/cmd/read-badger/cmd/epoch_protocol_state.go (1)
41-44: LGTM!The updated access pattern using the newly constructed
epochProtocolStateEntriesis correct, and error handling is properly preserved.cmd/util/cmd/read-badger/cmd/epoch_commit.go (2)
30-30: LGTM!The migration to
store.NewEpochCommitsfollows the established pattern consistently.
39-42: LGTM!The updated access pattern is correct and error handling is properly maintained.
Co-authored-by: Alexander Hentschel <alex.hentschel@flowfoundation.org>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
storage/store/headers.go (2)
329-334: FindHeaders is unused and should be removed.
FindHeadersis already marked deprecated and unused throughout the codebase. While the concern about missing chain ID validation is technically valid, the deprecation is the appropriate response. Given the PR's focus on chain separation and the fact that other retrieval methods enforce chain ID checks (seeretrieveTxlines 105-106), this method should be removed entirely rather than refactored. The deprecation notice makes removal the clear path forward.
340-368: Add chain validation for the input header parameter.The method should validate that the input
headerparameter belongs to the expected chain (h.chainID) before using it to update the executed block. Byzantine inputs must be validated; see the defensive programming pattern used in theStoreoperation (lines 40-41).Additionally, consider validating the retrieved header against the chain ID as a defense against database corruption, particularly given the security-sensitive nature of execution state.
🤖 Fix all issues with AI agents
In @engine/collection/epochmgr/factories/cluster_state.go:
- Line 76: Fix the typo in the error message returned where the code currently
uses fmt.Errorf("could create mutable cluster state: %w", err); update this to
fmt.Errorf("could not create mutable cluster state: %w", err) (look for this
exact format call in the cluster state factory code, e.g., in cluster_state.go
or the function that returns the six-tuple including the error) so the error
reads correctly.
In @storage/store/init.go:
- Around line 3-7: The file imports currently lack the "fmt" package while code
later calls fmt.Errorf (use of fmt.Errorf in init or wherever error is
constructed), causing a compile error; fix by adding "fmt" to the import block
alongside the existing imports (e.g., add "fmt" to the import list in init.go)
so fmt.Errorf resolves correctly and the package compiles.
🧹 Nitpick comments (2)
state/protocol/badger/state.go (1)
1005-1014: Redundant irrecoverable wrapping and minor typo.
GetLatestFinalizedHeaderalready returns errors wrapped withirrecoverable.NewExceptionf, so wrapping again at line 1011 withirrecoverable.NewExceptionis redundant. Consider returning the error directly or adding meaningful context if re-wrapping.Line 1006: typo "present./" should be "present."
Suggested fix
-// benign [storage.ErrNotFound] and failed reads of quantities that the protocol mandates to be present./ +// benign [storage.ErrNotFound] and failed reads of quantities that the protocol mandates to be present. // // No error returns expected during normal operations. func GetChainID(db storage.DB) (flow.ChainID, error) { h, err := GetLatestFinalizedHeader(db) if err != nil { - return "", irrecoverable.NewException(err) + return "", fmt.Errorf("could not get chain ID from finalized header: %w", err) } return h.ChainID, nil }storage/store/headers.go (1)
98-109: Minor: Returnnilexplicitly instead oferrfor clarity.After the error check at lines 101-103,
erris guaranteed to benil. Returningnilexplicitly would be clearer.♻️ Suggested improvement
retrieve := func(r storage.Reader, blockID flow.Identifier) (*flow.Header, error) { var header flow.Header err := operation.RetrieveHeader(r, blockID, &header) if err != nil { return nil, err } // raise an error when the retrieved header is for a different chain than expected if header.ChainID != chainID { return nil, fmt.Errorf("expected chain ID %v, got %v: %w", chainID, header.ChainID, storage.ErrWrongChain) } - return &header, err + return &header, nil }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
engine/collection/epochmgr/engine_test.goengine/collection/epochmgr/factories/cluster_state.goengine/verification/verifier/verifiers.gostate/protocol/badger/state.gostorage/store/headers.gostorage/store/init.go
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/verification/verifier/verifiers.go
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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:
engine/collection/epochmgr/engine_test.gostate/protocol/badger/state.gostorage/store/init.gostorage/store/headers.goengine/collection/epochmgr/factories/cluster_state.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:
engine/collection/epochmgr/engine_test.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:
engine/collection/epochmgr/engine_test.goengine/collection/epochmgr/factories/cluster_state.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
engine/collection/epochmgr/engine_test.goengine/collection/epochmgr/factories/cluster_state.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
engine/collection/epochmgr/engine_test.goengine/collection/epochmgr/factories/cluster_state.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
storage/store/init.gostorage/store/headers.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
storage/store/init.gostorage/store/headers.go
🧠 Learnings (6)
📚 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:
state/protocol/badger/state.gostorage/store/init.gostorage/store/headers.goengine/collection/epochmgr/factories/cluster_state.go
📚 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 **/*.go : Use comprehensive error wrapping for debugging; avoid `fmt.Errorf`, use `irrecoverable` package for exceptions
Applied to files:
state/protocol/badger/state.gostorage/store/init.gostorage/store/headers.goengine/collection/epochmgr/factories/cluster_state.go
📚 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 **/*.go : NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Applied to files:
storage/store/init.goengine/collection/epochmgr/factories/cluster_state.go
📚 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 **/*.go : All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Applied to files:
storage/store/headers.go
📚 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 **/*.go : Implement proper interfaces before concrete types
Applied to files:
storage/store/headers.go
📚 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 {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
storage/store/headers.go
🧬 Code graph analysis (4)
engine/collection/epochmgr/engine_test.go (2)
model/flow/chain.go (1)
ChainID(14-14)state/protocol/mock/params.go (1)
NewParams(171-181)
storage/store/init.go (3)
storage/badger/all.go (1)
InitAll(14-56)model/flow/chain.go (1)
ChainID(14-14)storage/store/headers.go (1)
NewHeaders(35-59)
storage/store/headers.go (6)
model/flow/chain.go (1)
ChainID(14-14)storage/headers.go (1)
Headers(15-65)state/cluster/root_block.go (1)
IsCanonicalClusterID(26-28)storage/errors.go (2)
ErrWrongChain(37-37)ErrNotAvailableForClusterConsensus(41-41)storage/locks.go (2)
LockInsertBlock(14-14)LockInsertOrFinalizeClusterBlock(22-22)storage/operation/headers.go (5)
InsertHeader(26-42)LookupBlockHeight(142-144)LookupCertifiedBlockByView(149-151)InsertClusterHeader(57-73)RetrieveHeader(79-81)
engine/collection/epochmgr/factories/cluster_state.go (5)
model/flow/chain.go (1)
ChainID(14-14)state/cluster/badger/mutator.go (2)
MutableState(23-30)NewMutableState(34-44)storage/store/headers.go (3)
Headers(19-27)NewClusterHeaders(66-88)NewHeaders(35-59)storage/store/cluster_payloads.go (2)
ClusterPayloads(16-19)NewClusterPayloads(23-37)state/protocol/badger/state.go (3)
IsBootstrapped(982-992)State(34-70)Bootstrap(94-203)
⏰ 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 (./)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./integration/)
- 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: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (17)
engine/collection/epochmgr/factories/cluster_state.go (1)
46-55: LGTM! Clean separation of cluster and consensus headers.The factory now properly creates dedicated storage abstractions for cluster headers (via
NewClusterHeaders) and consensus headers (viaNewHeaders), correctly wiring the chain IDs to each. This aligns with the PR objective of differentiating between cluster and consensus header storage.engine/collection/epochmgr/engine_test.go (4)
70-73: LGTM! Cluster state params correctly mocked with ChainID.The mock setup properly wires
ChainID()to return a cluster-specific chain ID, and the params are attached to the cluster state mock. This allows tests to simulate the chain ID retrieval that the real implementation uses.
111-140: LGTM! Factory mock correctly updated to accept the new chainID parameter.The
MockFactoryCreatemethod properly:
- Adds
mock.Anythingmatcher for theconsensusChainIDparameter (line 111)- Updates all return function signatures to include the
consensusChainID flow.ChainIDparameter (lines 118-139)This aligns with the signature change in
EpochComponentsFactory.Create.
167-169: LGTM! Protocol state params correctly mocked with consensus ChainID.The setup properly creates a protocol params mock with
ChainID()returning a distinct consensus chain identifier, which is then wired tosuite.state.On("Params"). This enables the engine to obtain the consensus chain ID when creating epoch components.
279-281: LGTM! Unauthorized node mock updated for new signature.The matcher correctly adds
mock.Anythingfor theconsensusChainIDparameter to align with the updated factory interface.state/protocol/badger/state.go (3)
13-13: LGTM!Import addition is necessary for the new helper functions and aligns with the coding guidelines requiring use of the
irrecoverablepackage for exceptions.
981-992: LGTM!The switch from
storage.ErrNotFoundtooperation.IncompleteStateErroraligns with the PR's objective to differentiate between benign not-found errors and incomplete/corrupted protocol state. The updated comment accurately reflects the expected behavior.
1016-1047: LGTM!The implementation correctly reads directly from the database, bypassing high-level storage abstractions. Error handling properly uses
irrecoverable.NewExceptionfwith%wverb to preserve the error chain, allowing callers to check foroperation.IncompleteStateErrorusingerrors.Is(). Based on learnings, this aligns with the requirement to use theirrecoverablepackage for exceptions.storage/store/init.go (2)
31-39: Good documentation and error handling for the updated signature.The comment clearly explains the chainID parameter purpose and the "No errors expected during normal operations" contract. The error wrapping provides useful context for debugging.
60-79: LGTM!The remainder of the function correctly constructs the
Allstruct and returns nil error on success. The structure aligns with the pattern instorage/badger/all.go.storage/store/headers.go (7)
11-16: LGTM!The new imports for
irrecoverableandclusterpackages are appropriate.irrecoverableis used for programmer errors per the coding guidelines, andcluster.IsCanonicalClusterIDis used for chain ID validation.
18-27: Clean struct extension with chainID field.Adding
chainIDto theHeadersstruct enables consistent chain validation across all operations.
31-59: Well-designed constructor with proper validation.The implementation correctly:
- Uses
irrecoverable.NewExceptionffor programmer errors (calling wrong constructor)- Validates
header.ChainIDmatches the boundchainIDon insert- Enforces the appropriate lock (
LockInsertBlock)- Documents the "no errors expected" contract
This aligns with the PR objectives of binding Headers to a specific chain ID.
61-88: Cluster-specific constructor correctly mirrors the consensus path.Good implementation:
- Validates chainID is a canonical cluster ID
- Uses
LockInsertOrFinalizeClusterBlockandoperation.InsertClusterHeader- Uses cluster-specific height lookup via
LookupClusterBlockHeight- Returns
ErrNotAvailableForClusterConsensusfor view-based retrieval
90-131: Good encapsulation of shared logic innewHeaders.The internal helper cleanly consolidates:
- Cache initialization with appropriate metrics
- Chain ID validation on retrieval
- Binding of chain-specific store/retrieve functions
The function signature with type-parameterized callbacks (
storeWithLockFunc,retrieveFunc) provides good flexibility for cluster vs consensus paths.
191-203: Helpful inline documentation explaining chain-specific index behavior.The comment at lines 195-197 clearly explains why this method only returns IDs for the expected chain—because the height cache uses a chain-specific database index. This aids future maintainability.
250-264: Remove:Existsintentionally does not distinguish chains.The interface in
storage/headers.go(lines 40-42) explicitly documents that this method does not distinguish between cluster and consensus headers. This is intentional by design—Existsis a simple existence check, while other interface methods likeByBlockIDandByHeightenforce chain separation by returningErrWrongChainwhen needed. No action required.
Co-authored-by: Alexander Hentschel <alex.hentschel@flowfoundation.org>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
storage/store/headers.go (2)
98-109: Returnnilexplicitly instead of reusingerr.At line 108,
erris guaranteed to benilafter the successful check at line 101-103. While this is technically correct, returningerris misleading. Returnnilexplicitly for clarity.Proposed fix
retrieve := func(r storage.Reader, blockID flow.Identifier) (*flow.Header, error) { var header flow.Header err := operation.RetrieveHeader(r, blockID, &header) if err != nil { return nil, err } // raise an error when the retrieved header is for a different chain than expected if header.ChainID != chainID { return nil, fmt.Errorf("expected chain ID %v, got %v: %w", chainID, header.ChainID, storage.ErrWrongChain) } - return &header, err + return &header, nil }
342-347:FindHeadersmarked as deprecated.The deprecation notice is appropriate. Consider adding a TODO or issue reference indicating when this method will be removed, or what should be used instead.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/util/cmd/export-json-transactions/transactions/range_test.gostate/cluster/state_test.gostorage/blocks.gostorage/cluster_blocks.gostorage/headers.gostorage/store/blocks.gostorage/store/cluster_blocks.gostorage/store/headers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- storage/headers.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.mdFollow 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:
storage/cluster_blocks.gostorage/store/blocks.gostate/cluster/state_test.gocmd/util/cmd/export-json-transactions/transactions/range_test.gostorage/store/cluster_blocks.gostorage/blocks.gostorage/store/headers.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
storage/cluster_blocks.gostorage/store/blocks.gostorage/store/cluster_blocks.gostorage/blocks.gostorage/store/headers.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
storage/cluster_blocks.gostorage/store/blocks.gostorage/store/cluster_blocks.gostorage/blocks.gostorage/store/headers.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:
state/cluster/state_test.gocmd/util/cmd/export-json-transactions/transactions/range_test.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/util/cmd/export-json-transactions/transactions/range_test.go
🧠 Learnings (5)
📚 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 {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
storage/store/blocks.gostorage/blocks.gostorage/store/headers.go
📚 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:
storage/store/blocks.gocmd/util/cmd/export-json-transactions/transactions/range_test.gostorage/store/headers.go
📚 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 **/*.go : Implement proper interfaces before concrete types
Applied to files:
storage/store/blocks.gostorage/store/headers.go
📚 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 **/*.go : Use comprehensive error wrapping for debugging; avoid `fmt.Errorf`, use `irrecoverable` package for exceptions
Applied to files:
storage/store/blocks.gostorage/store/headers.go
📚 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 **/*.go : All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Applied to files:
storage/store/headers.go
🧬 Code graph analysis (5)
storage/store/blocks.go (1)
storage/blocks.go (1)
Blocks(17-112)
state/cluster/state_test.go (3)
model/flow/chain.go (1)
AllChainIDs(46-58)state/cluster/root_block.go (3)
IsCanonicalClusterID(26-28)CanonicalClusterID(18-20)ClusterChainPrefix(14-14)utils/unittest/fixtures.go (1)
IdentifierListFixture(1144-1150)
cmd/util/cmd/export-json-transactions/transactions/range_test.go (2)
cmd/util/cmd/common/storage.go (1)
InitStorages(62-65)utils/unittest/fixtures/util.go (1)
NoError(6-10)
storage/store/cluster_blocks.go (2)
storage/cluster_blocks.go (1)
ClusterBlocks(13-34)storage/operation/cluster.go (1)
LookupClusterBlockHeight(57-59)
storage/store/headers.go (6)
model/flow/chain.go (1)
ChainID(14-14)state/cluster/root_block.go (1)
IsCanonicalClusterID(26-28)storage/errors.go (3)
ErrWrongChain(37-37)ErrNotAvailableForClusterConsensus(41-41)ErrNotFound(17-17)storage/locks.go (2)
LockInsertBlock(14-14)LockInsertOrFinalizeClusterBlock(22-22)storage/operation/headers.go (4)
InsertHeader(26-42)LookupBlockHeight(142-144)LookupCertifiedBlockByView(149-151)RetrieveHeader(79-81)storage/operation/cluster.go (1)
LookupClusterBlockHeight(57-59)
⏰ 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 (./)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./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: Integration Tests Others (integration)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (21)
state/cluster/state_test.go (2)
1-13: LGTM!The external test package (
cluster_test) is appropriate for testing the public API. Imports are well-organized and all used.
15-27: LGTM! Good test coverage with varying identifier counts.The test properly addresses the prior review feedback by varying the identifier list size from 0 to 99, including the empty list edge case. The combination of negative tests (consensus chain IDs rejected) and positive tests (canonical cluster IDs accepted) provides good coverage of the
IsCanonicalClusterIDfunction.cmd/util/cmd/export-json-transactions/transactions/range_test.go (2)
63-64: Appropriate ChainID for consensus block storage.Using
flow.Emulatoras the ChainID is correct here sinceb1andb2are consensus chain blocks created viaunittest.BlockFixture(), andflow.Emulatoris a main consensus chain ID. The error handling addition properly validates storage initialization.This addresses the concern raised in the past review about ChainID correctness - using the consensus chain ID (rather than a cluster chain ID) is appropriate for storing these consensus blocks.
77-87: Correct fix: avoid shadowing the outererrvariable.The change from
err :=toerr =properly reuses theerrvariable declared on line 63, ensuring the error fromWithLockis checked by therequire.NoErroron line 87. Using:=would have shadowed the outer variable, which could lead to confusion.storage/blocks.go (3)
26-36: Documentation updates for ID-based methods are correct.The addition of
ErrWrongChaintoBatchStoreandByIDis appropriate—these ID-based operations can receive identifiers for blocks that exist on a different chain (e.g., cluster vs. consensus). The standardized error documentation format improves consistency.
43-45: Consistent withByIDdocumentation pattern.
51-98: Correct omission ofErrWrongChainfrom height/view/collection-based methods.These methods correctly do not document
ErrWrongChain:
- Height-based lookups (
ByHeight,ProposalByHeight) use chain-specific indices, so they can only return blocks from the bound chain.- View-based lookups (
ByView,ProposalByView) are similarly chain-specific.- Collection-based lookups are consensus-chain-specific.
Note: The AI summary claims these methods have
ErrWrongChaindocumentation added, but the actual code does not include it—this is the correct behavior.storage/cluster_blocks.go (2)
20-21: Documentation correctly reflects new error semantics.The added
storage.ErrWrongChainerror documentation forProposalByIDaligns with the implementation instorage/store/cluster_blocks.gowhere chain ID is checked during header retrieval.
30-30: Correct:ProposalByHeightdoes not documentErrWrongChain.This is accurate since
ProposalByHeightuses a chain-specific height index (LookupClusterBlockHeightwithchainID), so cross-chain lookups cannot occur at the height lookup level.storage/store/blocks.go (4)
67-70: Correct use of irrecoverable exception for state corruption.If a header is successfully retrieved but the payload cannot be found, this indicates data inconsistency. Wrapping with
irrecoverable.NewExceptionfis appropriate here. Based on learnings, this aligns with the convention to use theirrecoverablepackage for exceptions indicating state corruption.
100-103: Correct use of irrecoverable exception for missing proposer signature.After successfully retrieving the block, a missing proposer signature indicates state corruption. The use of
irrecoverable.NewExceptionfis appropriate here.
169-175: Correct error handling for indexed proposal retrieval.When a block is indexed by view but cannot be retrieved, this is a symptom of state corruption. Wrapping the error with
irrecoverable.NewExceptionfis the correct approach.
43-43: Documentation aligns with interface.The
ErrWrongChaindocumentation forBatchStorematches the interface definition instorage/blocks.go.storage/store/cluster_blocks.go (1)
40-90: Well-structured implementation ofProposalByID.The implementation correctly:
- Retrieves header first (allows ErrNotFound/ErrWrongChain to propagate)
- Uses irrecoverable exceptions for subsequent failures (payload, signature) indicating state corruption
- Handles both root and non-root block/proposal construction
storage/store/headers.go (7)
31-59: Well-designed constructor with proper validation.The constructor correctly:
- Validates that the chainID is NOT a cluster chain (using
cluster.IsCanonicalClusterID)- Returns an
irrecoverable.NewExceptionferror instead of panicking (addresses past review feedback)- Validates header.ChainID matches the bound chainID before storage
- Checks for the appropriate lock (
LockInsertBlock)- Uses chain-specific height and view retrieval functions
61-88: Correct cluster-specific constructor implementation.The cluster headers constructor:
- Validates that the chainID IS a cluster chain
- Uses
LockInsertOrFinalizeClusterBlocklock- Uses
InsertClusterHeaderandLookupClusterBlockHeightoperations- Correctly returns
ErrNotAvailableForClusterConsensusfor view lookups
184-188: Correct use of irrecoverable exception for missing proposer signature.After successfully retrieving the header, a missing proposer signature indicates state corruption. The error wrapping and comment are appropriate.
231-236: Correct error handling for height-indexed header retrieval.When the height index lookup succeeds but header retrieval fails, this indicates state corruption. The irrecoverable exception wrapping is appropriate, and the comment clearly explains the reasoning.
252-257: Correct error handling for view-indexed header retrieval.Similar to
ByHeight, when the view cache returns a block ID but the header cannot be retrieved, this indicates state corruption. The irrecoverable exception wrapping is appropriate.
260-276: Well-designedExistsmethod handles chain boundaries correctly.The implementation correctly:
- Short-circuits with cache check (blocks on wrong chain are never cached)
- Falls back to
retrieveTxwhich validates ChainID- Treats both
ErrNotFoundandErrWrongChainas "does not exist on this chain"This addresses the past review comment about
Existsneeding to be chain-aware.
317-324: Error handling for child retrieval is appropriate as-is.
ErrWrongChainis explicitly documented as an expected error return in theByParentIDinterface (see documentation comment). When children from the parent-child index are retrieved viaByBlockID, returningErrWrongChainis expected behavior, not state corruption—this occurs when indexed children exist on a different chain than the current Headers instance's chain.Unlike
ByHeightandByView, which use indexed lookups that guarantee block existence, the parent-child index can legitimately contain block IDs from multiple chains since the index itself is not chain-specific. Cross-chain child references are a normal scenario, not an indicator of index corruption.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Co-authored-by: Alexander Hentschel <alex.hentschel@flowfoundation.org>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/store/blocks.go (1)
227-233: WrapByIDerror as irrecoverable for state corruption detection.
ByCollectionIDfollows the same pattern asByHeightandByView: an index lookup followed by a block retrieval. However, unlike those methods, it does not wrap theByIDerror withirrecoverable.NewExceptionf. BothByHeightandByViewwrap their second retrieval with the explicit comment "failure to retrieve a block that has been indexed indicates state corruption."If
BlockIDByCollectionIDsucceeds and returns a finalized block ID, failure to retrieve that block by ID indicates the same state corruption condition and should be handled identically. Apply the same error wrapping pattern:return nil, irrecoverable.NewExceptionf("could not retrieve indexed block for collection %x: %w", collID[:], err)
♻️ Duplicate comments (2)
module/builder/collection/builder.go (1)
308-310: Useirrecoverable.NewExceptionffor unexpected errors.Line 309 uses
fmt.Errorffor an unexpected error path, but per coding guidelines, unexpected errors (exceptions) should useirrecoverable.NewExceptionf. This is also inconsistent with line 340 which correctly usesirrecoverable.NewExceptionffor a similar unexpected error case.Proposed fix
} else { - return nil, fmt.Errorf("unexpected failure to retrieve first height of operating epoch: %w", err) + return nil, irrecoverable.NewExceptionf("unexpected failure to retrieve first height of operating epoch: %w", err) }Based on learnings, use comprehensive error wrapping for debugging; avoid
fmt.Errorf, useirrecoverablepackage for exceptions.storage/store/cluster_blocks.go (1)
103-108: Correct irrecoverable wrapping for indexed retrieval.The implementation correctly treats any error from
ProposalByIDafter a successful height lookup as state corruption. The height index is chain-specific, soErrWrongChainshould never occur here, andErrNotFoundwould indicate an inconsistent index.
🧹 Nitpick comments (1)
state/protocol/badger/state.go (1)
1008-1014: Double-wrapping inconsistency with documented error type.The documentation states this function may return
[operation.IncompleteStateError], butGetLatestFinalizedHeaderalready wraps errors withirrecoverable.NewExceptionf, and thenGetChainIDwraps again withirrecoverable.NewException. This double-wrapping is redundant.Since
GetLatestFinalizedHeaderalready returns an irrecoverable exception on any error, you can simplify the error return here:♻️ Suggested simplification
func GetChainID(db storage.DB) (flow.ChainID, error) { h, err := GetLatestFinalizedHeader(db) if err != nil { - return "", irrecoverable.NewException(err) + return "", err // already wrapped as irrecoverable exception by GetLatestFinalizedHeader } return h.ChainID, nil }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
engine/collection/epochmgr/factories/cluster_state.goengine/testutil/nodes.goengine/verification/verifier/verifiers.gomodule/builder/collection/builder.gostate/protocol/badger/state.gostorage/headers.gostorage/store/blocks.gostorage/store/blocks_test.gostorage/store/cluster_blocks.gostorage/store/headers.gostorage/store/headers_test.go
✅ Files skipped from review due to trivial changes (1)
- storage/headers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/verification/verifier/verifiers.go
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in
@docs/agents/CodingConventions.mdFollow 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:
storage/store/blocks.gostorage/store/cluster_blocks.gostorage/store/blocks_test.gomodule/builder/collection/builder.goengine/testutil/nodes.gostate/protocol/badger/state.gostorage/store/headers_test.goengine/collection/epochmgr/factories/cluster_state.gostorage/store/headers.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
storage/store/blocks.gostorage/store/cluster_blocks.gostorage/store/blocks_test.gostorage/store/headers_test.gostorage/store/headers.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
storage/store/blocks.gostorage/store/cluster_blocks.gostorage/store/blocks_test.gostorage/store/headers_test.gostorage/store/headers.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:
storage/store/blocks_test.gostorage/store/headers_test.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:
module/builder/collection/builder.goengine/testutil/nodes.goengine/collection/epochmgr/factories/cluster_state.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
engine/testutil/nodes.goengine/collection/epochmgr/factories/cluster_state.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
engine/testutil/nodes.goengine/collection/epochmgr/factories/cluster_state.go
🧠 Learnings (6)
📚 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 **/*.go : Use comprehensive error wrapping for debugging; avoid `fmt.Errorf`, use `irrecoverable` package for exceptions
Applied to files:
storage/store/blocks.gostorage/store/cluster_blocks.gomodule/builder/collection/builder.gostate/protocol/badger/state.goengine/collection/epochmgr/factories/cluster_state.gostorage/store/headers.go
📚 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 {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
storage/store/blocks.gomodule/builder/collection/builder.gostorage/store/headers.go
📚 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:
storage/store/blocks.gostorage/store/blocks_test.gostate/protocol/badger/state.gostorage/store/headers_test.goengine/collection/epochmgr/factories/cluster_state.gostorage/store/headers.go
📚 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 **/*.go : Implement proper interfaces before concrete types
Applied to files:
storage/store/blocks.gostorage/store/headers.go
📚 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 **/*.go : All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Applied to files:
storage/store/cluster_blocks.gomodule/builder/collection/builder.gostorage/store/headers.go
📚 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 **/*.go : NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Applied to files:
module/builder/collection/builder.goengine/collection/epochmgr/factories/cluster_state.go
🧬 Code graph analysis (5)
storage/store/blocks.go (2)
storage/blocks.go (1)
Blocks(17-112)storage/mock/blocks.go (1)
Blocks(15-17)
storage/store/cluster_blocks.go (2)
storage/cluster_blocks.go (1)
ClusterBlocks(13-34)storage/operation/cluster.go (1)
LookupClusterBlockHeight(57-59)
storage/store/blocks_test.go (5)
storage/store/init.go (1)
InitAll(37-82)storage/badger/all.go (1)
InitAll(14-56)storage/blocks.go (1)
Blocks(17-112)storage/errors.go (2)
ErrNotFound(17-17)ErrWrongChain(37-37)storage/locks.go (1)
LockInsertBlock(14-14)
engine/testutil/nodes.go (4)
storage/store/init.go (1)
InitAll(37-82)storage/badger/all.go (1)
InitAll(14-56)model/flow/chain.go (1)
ChainID(14-14)storage/store/headers.go (1)
NewHeaders(35-59)
state/protocol/badger/state.go (2)
storage/operation/heights.go (2)
RetrieveFinalizedHeight(46-58)IncompleteStateError(21-21)storage/operation/headers.go (2)
LookupBlockHeight(142-144)RetrieveHeader(79-81)
⏰ 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 (./)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: analyze-code (go)
- GitHub Check: Docker Build
🔇 Additional comments (33)
engine/collection/epochmgr/factories/cluster_state.go (4)
37-44: LGTM!The updated function signature correctly introduces
consensusChainIDparameter and returns separate header instances for cluster and consensus chains. This aligns well with the PR objective to differentiate between cluster and consensus header storage.
46-55: LGTM!The storage abstraction creation correctly separates cluster-specific storage (
clusterHeaders,clusterPayloads,clusterBlocks) from consensus storage (consensusHeaders). The dependency chain is properly established withclusterBlocksreceiving its header and payload dependencies. Error messages provide good context for debugging.
57-72: LGTM!The bootstrap/open state logic correctly passes the new
clusterHeadersandclusterPayloadstoOpenState, maintaining proper separation of cluster-specific storage. Error handling is consistent with descriptive messages.
74-78: LGTM!The mutable state creation correctly receives both
clusterHeadersandconsensusHeaders, enabling proper validation of reference blocks against the main consensus chain. This supports the PR objective to differentiate between cluster and consensus header storage.module/builder/collection/builder.go (2)
47-56: LGTM! Clear documentation for epoch height caching.The added documentation thoroughly explains why
epochFirstHeightuses a pointer type and the runtime initialization pattern. This addresses the concern that the builder is instantiated before the epoch starts.
639-669: LGTM! Excellent documentation and correct epoch-aware bounds.The extensive comments clearly explain:
- The mathematical derivation of the search range bounds
- The transformation from exclusive to inclusive bounds
- The epoch boundary consideration
- The underflow protection logic
The implementation correctly bounds the lower search range by
epochFirstHeightwhile preventing underflow through the rewritten comparison.state/protocol/badger/state.go (2)
13-13: LGTM!The
irrecoverableimport is correctly added to support the new error handling inGetChainIDandGetLatestFinalizedHeader.
980-992: LGTM!The
IsBootstrappedfunction correctly:
- Documents that no errors are expected during normal operation (addressing past review feedback)
- Checks for
operation.IncompleteStateErrorwhich is returned byRetrieveFinalizedHeightwhen the finalized height is not foundThis aligns with the PR objective to avoid confusion between benign
storage.ErrNotFoundand missing protocol-mandated data.engine/testutil/nodes.go (2)
239-240: LGTM!The
InitAllcall correctly passes the chainID from the root snapshot and properly handles the new error return value. This aligns with the PR's objective of binding Headers instances to a ChainID.
562-563: LGTM!The
NewHeaderscall correctly passes the chainID and handles the error return, consistent with the updated API signature.storage/store/blocks_test.go (3)
23-25: LGTM!The
InitAllcall is correctly updated to use the new signature withflow.Emulatorand proper error handling. The pattern is consistently applied across all test functions.
29-33: Good addition of pre-insert validation.Testing that
ErrNotFoundis returned before the block exists improves test coverage for the expected error semantics.
186-224: Excellent test coverage forErrWrongChainbehavior.This test properly validates the differentiation between consensus and cluster headers:
ByID/ProposalByIDcorrectly returnErrWrongChainwhen the block exists but belongs to a different chainByHeight/ProposalByHeightcorrectly returnErrNotFoundsince the height index is chain-specificThis addresses the PR objective of returning documented sentinel errors for chain mismatches.
storage/store/blocks.go (4)
67-70: Correct use of irrecoverable for state corruption.When a header exists but the payload cannot be retrieved, this indicates database inconsistency. Using
irrecoverable.NewExceptionfappropriately signals this as a fatal condition rather than an expected error.
100-103: LGTM!Wrapping signature retrieval errors as irrecoverable is correct since the block was already successfully retrieved, so missing signature data indicates state corruption.
157-162: LGTM!When the view index lookup succeeds, failure to retrieve the indexed block indicates state corruption. The irrecoverable wrapping correctly captures that the index is inconsistent with actual stored data.
192-197: LGTM!Height-indexed block retrieval failures are correctly treated as state corruption since the height index is maintained for finalized blocks only.
storage/store/cluster_blocks.go (1)
40-53: LGTM!The error handling correctly distinguishes between:
ErrNotFound/ErrWrongChainfrom header retrieval (lines 41-44) - propagated as expected errors- Payload/signature retrieval failures (lines 46-53) - wrapped as irrecoverable since header existence implies these must exist
storage/store/headers_test.go (5)
53-60: Good addition of retrieval by ID and ProposalByBlockID tests.This adds coverage for
ByBlockIDandProposalByBlockIDhappy paths, addressing the past review request for testing these methods.
96-104: Excellent test forErrNotAvailableForClusterConsensussentinel.Testing both an existing view and a non-existent view confirms that the error relates to the method being unavailable for cluster consensus, not the specific data.
184-194: Good test case forErrWrongChaininByParentID.This addresses the past review request to verify that
ByParentIDreturnsErrWrongChainwhen the parent block exists but belongs to a different chain.
263-291: Comprehensive test forErrWrongChainon store operations.The test validates that attempting to store blocks with various mismatched chain IDs (cluster chains, other main chains, invalid chains) all correctly return
ErrWrongChain. This thoroughly tests the PR's objective of rejecting headers from the wrong chain.
293-367: Excellent comprehensive test for cross-chain retrieval errors.This test thoroughly validates the PR's sentinel error behavior across multiple scenarios:
- Consensus Headers correctly retrieves consensus blocks
- Cluster Headers returns
ErrNotFoundfor height (chain-specific index) andErrWrongChainfor ID lookup of consensus blocks- Different cluster chains correctly return
ErrWrongChainfor each other's blocks- Consensus Headers correctly returns
ErrWrongChainfor cluster blocksThis provides strong coverage for the differentiated header storage behavior.
storage/store/headers.go (10)
18-27: LGTM - Headers struct updated with chainID field.The addition of the
chainIDfield enables the chain-specific behavior required by this PR. The struct is properly implementingstorage.Headersinterface as confirmed by the compile-time check on line 29.
31-59: LGTM - NewHeaders constructor properly validates and configures main consensus headers.The implementation correctly:
- Returns an error via
irrecoverable.NewExceptionffor invalid chain IDs (addressed from prior review)- Validates
header.ChainIDmatches on store operations- Uses the correct lock (
LockInsertBlock) and height/view lookup operations for main consensus
61-88: LGTM - NewClusterHeaders constructor correctly implements cluster-specific behavior.The implementation properly:
- Returns an error via
irrecoverable.NewExceptionffor non-cluster chain IDs- Uses the correct lock (
LockInsertOrFinalizeClusterBlock) andInsertClusterHeaderoperation- Uses the cluster-specific height index with
LookupClusterBlockHeight- Returns
ErrNotAvailableForClusterConsensusfor view lookups as documented
90-131: LGTM - Shared initialization logic correctly enforces chainID validation.The
retrievefunction (lines 98-109) properly validates that the retrieved header'sChainIDmatches the expected chain, returningstorage.ErrWrongChainon mismatch. This ensures all cache retrievals enforce chain separation.
223-237: LGTM - ByHeight correctly handles state corruption scenarios.The error handling at lines 233-235 properly uses
irrecoverable.NewExceptionfwhen an indexed block cannot be retrieved, as this indicates state corruption rather than a normal error condition.
260-276: LGTM - Exists method correctly treats ErrWrongChain as non-existence.Per the interface contract that a
Headersinstance operates only on its bound chain, a header from a different chain correctly returnsfalse(not found on this chain) rather than propagatingErrWrongChain. This aligns with the prior review discussion on robust API conventions.
297-325: LGTM - ByParentID properly validates parent chain before querying children.The method correctly:
- Validates the parent exists on the correct chain first (lines 298-302)
- Returns
ErrWrongChainif the parent belongs to a different chain- Treats failure to retrieve indexed children as state corruption (lines 319-320)
327-339: LGTM - BlockIDByView properly documents cluster limitation.The documentation correctly specifies that
ErrNotAvailableForClusterConsensusmay be returned when called on a cluster Headers instance, aligning with the interface contract.
341-346: No action needed — method is deprecated and unused.The
FindHeadersmethod is marked deprecated with "unsafe for public use" warning, and comprehensive codebase search confirms it has no callers. While the technical concern about chainID filtering is valid, it's already addressed through the deprecation notice. Since the method is dead code, the deprecation status is appropriate.
348-380: RollbackExecutedBlock bypasses chainID validation - add explicit safety check or use retrieveTx.This method retrieves headers directly via
operation.RetrieveHeader(line 360), bypassing chainID validation thath.retrieveTxprovides through the cache and indexed lookups. While the method is documented as "Intended to be used by Execution Nodes only" and currently only used in an admin recovery utility, there is no code-level enforcement preventing it from being called on a clusterHeadersinstance created viaNewClusterHeaders. A cluster instance could theoretically bypass the intended EN-only constraint.Recommend either:
- Add a check to ensure this method is only callable on main consensus
Headersinstances (validate that h.chainID is not a cluster chain ID)- Use
h.retrieveTxinstead ofoperation.RetrieveHeaderfor consistency with other retrieval methods, which ensures chainID validation- At minimum, validate that the retrieved header's chainID matches h.chainID before using it
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@state/protocol/badger/state.go`:
- Around line 1036-1044: The current error handling in LookupBlockHeight and
RetrieveHeader discards the original err by returning
operation.IncompleteStateError directly; change the returns in the functions
calling LookupBlockHeight and RetrieveHeader to wrap the original err so the
original error is preserved while still indicating semantic IncompleteStateError
(e.g., use fmt.Errorf to wrap operation.IncompleteStateError with %w or include
the original err as a wrapped cause). Update the two return statements around
operation.LookupBlockHeight(...) and operation.RetrieveHeader(...) to return a
wrapped error that includes operation.IncompleteStateError and the original err
(so tools and callers can inspect both), and keep behavior consistent with
RetrieveFinalizedHeight's mapping of storage.ErrNotFound to
operation.IncompleteStateError.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
state/protocol/badger/state.gostorage/store/blocks.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in
@docs/agents/CodingConventions.mdFollow 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:
storage/store/blocks.gostate/protocol/badger/state.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
storage/store/blocks.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
storage/store/blocks.go
🧠 Learnings (4)
📚 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 **/*.go : Use comprehensive error wrapping for debugging; avoid `fmt.Errorf`, use `irrecoverable` package for exceptions
Applied to files:
storage/store/blocks.go
📚 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 {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
storage/store/blocks.gostate/protocol/badger/state.go
📚 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:
storage/store/blocks.gostate/protocol/badger/state.go
📚 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 **/*.go : Implement proper interfaces before concrete types
Applied to files:
storage/store/blocks.go
⏰ 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). (35)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: analyze-code (go)
- GitHub Check: Docker Build
🔇 Additional comments (8)
state/protocol/badger/state.go (2)
979-991: LGTM!The updated error handling correctly uses
operation.IncompleteStateErrorinstead ofstorage.ErrNotFound, which aligns with the PR's goal of distinguishing protocol-mandated data absence from benign not-found scenarios. The documentation also properly states that errors indicate bugs or state corruption.
993-1013: LGTM!The function is well-documented, correctly delegates to
GetLatestFinalizedHeader, and properly wraps errors with context. The documentation clearly explains the error semantics and when this function should be called.storage/store/blocks.go (6)
67-70: Correct use of irrecoverable for state corruption.The reasoning is sound: if the header was successfully retrieved, the payload must exist since they are stored atomically via
BatchStore. A missing payload indicates state corruption.
100-103: Appropriate irrecoverable wrapping for signature retrieval.If the block body was successfully retrieved, the proposer signature must exist since they are stored together. This correctly signals state corruption.
152-162: State corruption handling in ByView is well-implemented.The logic correctly wraps retrieval errors as irrecoverable: if
BlockIDByViewsucceeds (block is indexed), butByIDfails, this indicates inconsistent state. BothErrNotFoundandErrWrongChainfromByIDwould represent corruption here, so wrapping all errors is correct. The docstring appropriately only listsErrNotFound(from the index lookup) as the expected error.
185-197: Consistent pattern applied to ByHeight.Same reasoning applies: successful height-index lookup followed by failed block retrieval indicates state corruption. Documentation correctly omits
ErrWrongChainsince the height index is chain-specific.
225-237: ByCollectionID follows the established pattern correctly.The docstring lists only
ErrNotFoundas expected, which is appropriate. If the collection-to-block index exists butByIDfails (includingErrWrongChain), this indicates corruption in the index rather than a user-recoverable error.
1-283: Interface documentation is already up-to-date. Thestorage.Blocksinterface instorage/blocks.goalready documents theErrWrongChainerror semantics for the appropriate methods (BatchStore,ByID,ProposalByID), and it matches the implementation documentation. No updates are needed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| err = operation.LookupBlockHeight(r, finalized, &id) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not retrieve blockID of finalized block at height %d: %w", finalized, operation.IncompleteStateError) | ||
| } | ||
| var header flow.Header | ||
| err = operation.RetrieveHeader(r, id, &header) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not retrieve latest finalized block %x: %w", id, operation.IncompleteStateError) | ||
| } |
There was a problem hiding this comment.
Original error is discarded, losing debugging information.
Lines 1038 and 1043 replace the actual error err with operation.IncompleteStateError, discarding the original error details. This makes debugging harder when failures occur. The error wrapping should preserve the original error while also indicating the semantic meaning (incomplete state).
Additionally, there's inconsistency with how RetrieveFinalizedHeight handles errors (it internally converts storage.ErrNotFound to IncompleteStateError), while here the original error from LookupBlockHeight and RetrieveHeader is simply discarded.
Proposed fix to preserve original error
var id flow.Identifier
err = operation.LookupBlockHeight(r, finalized, &id)
if err != nil {
- return nil, fmt.Errorf("could not retrieve blockID of finalized block at height %d: %w", finalized, operation.IncompleteStateError)
+ if errors.Is(err, storage.ErrNotFound) {
+ return nil, fmt.Errorf("could not retrieve blockID of finalized block at height %d: %w", finalized, operation.IncompleteStateError)
+ }
+ return nil, fmt.Errorf("could not retrieve blockID of finalized block at height %d: %w", finalized, err)
}
var header flow.Header
err = operation.RetrieveHeader(r, id, &header)
if err != nil {
- return nil, fmt.Errorf("could not retrieve latest finalized block %x: %w", id, operation.IncompleteStateError)
+ if errors.Is(err, storage.ErrNotFound) {
+ return nil, fmt.Errorf("could not retrieve latest finalized block %x: %w", id, operation.IncompleteStateError)
+ }
+ return nil, fmt.Errorf("could not retrieve latest finalized block %x: %w", id, err)
}Alternatively, if the intent is to always return IncompleteStateError for any failure in this function (since all data is protocol-mandated), consider wrapping the original error within the IncompleteStateError to preserve debugging context:
return nil, fmt.Errorf("could not retrieve blockID of finalized block at height %d: %w (original: %v)", finalized, operation.IncompleteStateError, err)🤖 Prompt for AI Agents
In `@state/protocol/badger/state.go` around lines 1036 - 1044, The current error
handling in LookupBlockHeight and RetrieveHeader discards the original err by
returning operation.IncompleteStateError directly; change the returns in the
functions calling LookupBlockHeight and RetrieveHeader to wrap the original err
so the original error is preserved while still indicating semantic
IncompleteStateError (e.g., use fmt.Errorf to wrap
operation.IncompleteStateError with %w or include the original err as a wrapped
cause). Update the two return statements around operation.LookupBlockHeight(...)
and operation.RetrieveHeader(...) to return a wrapped error that includes
operation.IncompleteStateError and the original err (so tools and callers can
inspect both), and keep behavior consistent with RetrieveFinalizedHeight's
mapping of storage.ErrNotFound to operation.IncompleteStateError.
5 participants
A ChainID must now be provided to Headers storage instance on creation.
That storage instance will then only be able to successfully store or retrieve headers corresponding to the correct ChainID. In addition, the height-based index will also be specific to that ChainID.
Also adds 3 new error sentinels:
storage.ErrWrongChainfor storing or retrieving headers with an unexpected/mismatched ChainIDstorage.ErrNotAvailableForClusterConsensusfor methods not implemented by cluster headers storage (ByView)operation.IncompleteStateErrorfor failure to load protocol-mandated info from the database - should be treated as an exception, except when determining whether or not to bootstrap a node at startupand fixes a bug where cluster blocks from the previous epoch+chain would be unnecessarily checked for duplicate transactions.
Closes: #4204
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.