Update SyncResponse to be verifiable #8323
Conversation
Used in the cluster sync engine to certify our latest finalized height in a syncResponse. Implementation may still change.
Also, the correct QC is now included in both consensus/cluster `SyncResponse`s, and tests now check that SyncResponses contain a QC.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughReplaces height-only sync responses with finalized block Header plus its CertifyingQC, threads protocol.State into request handlers to obtain finalized headers/QCs, adds Snapshot.QuorumCertificate(), and removes local convenience submission methods from collection synchronization engines; tests and mocks updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Node
participant RequestHandler
participant ProtocolState as State
participant FinalizedState
participant Network
Node->>RequestHandler: SyncRequest
RequestHandler->>State: Final()
State-->>RequestHandler: FinalizedState
RequestHandler->>FinalizedState: Head()
FinalizedState-->>RequestHandler: Header
RequestHandler->>FinalizedState: QuorumCertificate()
FinalizedState-->>RequestHandler: CertifyingQC
RequestHandler->>Network: SendResponse { Nonce, Header, CertifyingQC } to Node
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/collection/synchronization/engine.go (1)
261-268: Add defensive nil check forres.Headerbefore dereferencing.The
Headerfield inSyncResponseis defined as a pointer (*Header) and can be nil. The code directly accessesres.Header.Heightwithout validation. While tests create valid responses, production inputs must be treated as potentially byzantine—a malformed message could reach this point (validation is incomplete per the PR description), causing a panic.Add an explicit nil check:
if res.Header == nil { e.log.Error().Msg("sync response has nil header") return }
🤖 Fix all issues with AI agents
In @engine/common/synchronization/engine.go:
- Around line 296-300: In onSyncResponse, avoid a nil pointer dereference by
checking that res and res.Header are non-nil before accessing res.Header.Height;
if res or res.Header is nil, log a warning including originID (via
e.log.Warn/Error().Str("origin_id", originID.String())) and return early instead
of calling e.core.HandleHeight(final, res.Header.Height); this keeps the
existing use of finalizedHeaderCache.Get() and e.core.HandleHeight intact but
prevents crashes from malformed/malicious SyncResponse messages.
In @model/flow/synchronization.go:
- Around line 14-18: Update the SyncResponse docstring to accurately describe
its current fields: explain that SyncResponse contains a Nonce for request
matching, a pointer to the latest Header returned by the responding node
(Header) and the accompanying CertifyingQC (QuorumCertificate) used to verify
the header; mention that field validation is deferred to
validateSyncResponseForALSP. Locate the comment above the SyncResponse type and
replace the outdated "latest finalized block height" description with this
updated summary referencing SyncResponse, Header, CertifyingQC, and
validateSyncResponseForALSP.
In @state/cluster/badger/snapshot.go:
- Around line 71-91: Fix Snapshot.QuorumCertificate by removing the premature
return that makes the storage.ErrNotFound handling unreachable: after calling
operation.RetrieveBlockChildren, if err != nil check if errors.Is(err,
storage.ErrNotFound) and treat that as “no children” (initialize pendingIDs
empty) otherwise return the wrapped error; then before indexing pendingIDs[0]
verify len(pendingIDs) > 0 and return (nil, nil) or an appropriate no-QC result
when empty to avoid panic; refer to Snapshot.QuorumCertificate,
operation.RetrieveBlockChildren, storage.ErrNotFound and mimic the
error-handling pattern used in pending().
🧹 Nitpick comments (2)
engine/common/synchronization/request_handler.go (2)
158-162: Add error wrapping for debugging context.The error from
Head()is returned without context, which makes debugging harder. The collection counterpart (engine/collection/synchronization/request_handler.goline 154) wraps this error properly. As per coding guidelines, use comprehensive error wrapping.♻️ Suggested fix
finalizedState := r.state.Final() finalizedHeader, err := finalizedState.Head() if err != nil { - return err + return fmt.Errorf("could not get last finalized header: %w", err) }
183-191: Good snapshot consistency; consider error wrapping.Using the same
finalizedStatesnapshot for bothHead()andQuorumCertificate()ensures the QC corresponds to the returned header. The SyncResponse construction correctly populates the new fields.However, the error from
QuorumCertificate()should also be wrapped for consistency with the collection implementation.♻️ Optional: wrap QC error
qcForFinalizedHeader, err := finalizedState.QuorumCertificate() if err != nil { - return err + return fmt.Errorf("could not get QC for last finalized header: %w", err) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
engine/collection/synchronization/engine.goengine/collection/synchronization/engine_test.goengine/collection/synchronization/request_handler.goengine/common/synchronization/engine.goengine/common/synchronization/engine_suite_test.goengine/common/synchronization/engine_test.goengine/common/synchronization/request_handler.goengine/common/synchronization/request_handler_engine.gomodel/flow/synchronization.gostate/cluster/badger/mutator_test.gostate/cluster/badger/snapshot.gostate/cluster/invalid/snapshot.gostate/cluster/mock/snapshot.gostate/cluster/snapshot.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
model/flow/synchronization.goengine/common/synchronization/request_handler.gostate/cluster/snapshot.gostate/cluster/badger/snapshot.goengine/common/synchronization/request_handler_engine.gostate/cluster/invalid/snapshot.goengine/common/synchronization/engine.goengine/collection/synchronization/engine.gostate/cluster/badger/mutator_test.goengine/collection/synchronization/engine_test.gostate/cluster/mock/snapshot.goengine/common/synchronization/engine_suite_test.goengine/common/synchronization/engine_test.goengine/collection/synchronization/request_handler.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/common/synchronization/request_handler.goengine/common/synchronization/request_handler_engine.goengine/common/synchronization/engine.goengine/collection/synchronization/engine.goengine/collection/synchronization/engine_test.goengine/common/synchronization/engine_suite_test.goengine/common/synchronization/engine_test.goengine/collection/synchronization/request_handler.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/common/synchronization/request_handler.goengine/common/synchronization/request_handler_engine.goengine/common/synchronization/engine.goengine/collection/synchronization/engine.goengine/collection/synchronization/engine_test.goengine/common/synchronization/engine_suite_test.goengine/common/synchronization/engine_test.goengine/collection/synchronization/request_handler.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
engine/common/synchronization/request_handler.goengine/common/synchronization/request_handler_engine.goengine/common/synchronization/engine.goengine/collection/synchronization/engine.goengine/collection/synchronization/engine_test.goengine/common/synchronization/engine_suite_test.goengine/common/synchronization/engine_test.goengine/collection/synchronization/request_handler.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/badger/mutator_test.goengine/collection/synchronization/engine_test.goengine/common/synchronization/engine_suite_test.goengine/common/synchronization/engine_test.go
🧠 Learnings (2)
📚 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/common/synchronization/request_handler.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 **/*_test.go : Use fixtures for realistic test data as defined in `/utils/unittest/`
Applied to files:
engine/collection/synchronization/engine_test.goengine/common/synchronization/engine_test.go
🧬 Code graph analysis (10)
model/flow/synchronization.go (2)
utils/unittest/fixtures/header.go (1)
Header(10-10)utils/unittest/fixtures/quorum_certificate.go (1)
QuorumCertificate(9-9)
engine/common/synchronization/request_handler.go (1)
model/flow/synchronization.go (1)
SyncResponse(14-18)
state/cluster/snapshot.go (1)
utils/unittest/fixtures/quorum_certificate.go (1)
QuorumCertificate(9-9)
state/cluster/invalid/snapshot.go (4)
state/cluster/badger/snapshot.go (1)
Snapshot(20-23)state/cluster/mock/snapshot.go (1)
Snapshot(11-13)state/cluster/snapshot.go (1)
Snapshot(10-42)state/protocol/badger/snapshot.go (1)
Snapshot(29-32)
engine/common/synchronization/engine.go (2)
engine/common/synchronization/request_handler.go (1)
NewRequestHandler(67-93)engine/common/synchronization/request_handler_engine.go (1)
NewResponseSender(47-51)
state/cluster/badger/mutator_test.go (1)
fvm/evm/offchain/query/view.go (1)
View(23-31)
engine/collection/synchronization/engine_test.go (5)
utils/unittest/fixtures/quorum_certificate.go (1)
QuorumCertificate(9-9)utils/unittest/fixtures.go (3)
CertifyBlock(2088-2094)BlockHeaderFixture(580-600)WithHeaderHeight(549-553)utils/unittest/fixtures/header.go (1)
Header(10-10)model/flow/synchronization.go (1)
SyncResponse(14-18)model/messages/synchronization.go (1)
SyncResponse(27-27)
state/cluster/mock/snapshot.go (2)
state/cluster/snapshot.go (1)
Snapshot(10-42)utils/unittest/fixtures/quorum_certificate.go (1)
QuorumCertificate(9-9)
engine/common/synchronization/engine_suite_test.go (2)
utils/unittest/fixtures/quorum_certificate.go (1)
QuorumCertificate(9-9)utils/unittest/fixtures.go (1)
CertifyBlock(2088-2094)
engine/collection/synchronization/request_handler.go (3)
utils/unittest/fixtures/quorum_certificate.go (1)
QuorumCertificate(9-9)model/flow/synchronization.go (1)
SyncResponse(14-18)model/messages/synchronization.go (1)
SyncResponse(27-27)
⏰ 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: Integration Tests Others (integration)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (storage)
- 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: Unit Tests (fvm)
- 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 (21)
state/cluster/badger/mutator_test.go (1)
500-503: LGTM - Test fix correctly prevents view collision.The fix ensures that the finalized block doesn't share the same view as the orphaned certified block, which would violate the consensus invariant that only one block can be certified per view. This prevents flaky test failures.
engine/common/synchronization/engine_suite_test.go (3)
39-39: LGTM - QC field addition aligns with new SyncResponse structure.The new
qcfield properly supports testing the Header+CertifyingQC-based sync responses.
67-68: LGTM - QC correctly initialized from the head header.Using
unittest.CertifyBlock(ss.head)ensures the QC'sViewandBlockIDare correctly linked to the header being certified, which is essential for valid sync responses.
119-124: LGTM - Snapshot mock correctly implements QuorumCertificate().The mock returns the pre-created QC that certifies the head, consistent with how the real snapshot would behave.
engine/common/synchronization/engine.go (1)
120-120: LGTM - RequestHandler correctly initialized with state dependency.The
stateparameter is now passed toNewRequestHandler, enabling it to retrieve finalized header and QC information for constructing verifiable sync responses.state/cluster/snapshot.go (1)
26-30: LGTM - QuorumCertificate() method follows established interface patterns.All implementations of the Snapshot interface have been properly updated: state/cluster/badger/snapshot.go, state/cluster/invalid/snapshot.go, and state/cluster/mock/snapshot.go all include the QuorumCertificate() method. This addition enables the sync engine to construct verifiable SyncResponses with the certifying QC.
engine/common/synchronization/request_handler_engine.go (1)
90-100: LGTM!The
stateparameter is correctly threaded through toNewRequestHandler, enabling access to the finalized state needed for populating the newHeaderandCertifyingQCfields inSyncResponse.state/cluster/invalid/snapshot.go (1)
45-48: LGTM!The
QuorumCertificate()method correctly follows the established pattern in this file, returningniland the stored error, consistent withHead(),Collection(), andPending()methods.state/cluster/mock/snapshot.go (1)
105-133: LGTM!Auto-generated mock code correctly implements the
QuorumCertificate()method following the established mockery pattern.engine/common/synchronization/request_handler.go (2)
54-54: LGTM on state field addition.The
protocol.Stateis properly wired through the constructor and stored for use in request handling.Also applies to: 72-72, 82-82
170-174: TODO appropriately tracked.The TODO(8174) comment correctly references the related issue for removing this step once certified sync responses are fully relied upon.
engine/collection/synchronization/engine_test.go (4)
67-68: Good test setup for QC-based flow.The test setup properly creates a QC using
unittest.CertifyBlockand configures the snapshot mock to return it. This aligns with the fixture patterns inutils/unittest/. Based on learnings, using fixtures for realistic test data is the correct approach.Also applies to: 120-125
198-201: Comprehensive assertions for SyncResponse validation.The test properly validates the new structure: Header content, QC content, and critically, that the CertifyingQC.BlockID matches the Header's ID. This ensures the QC actually certifies the returned header.
217-227: Test correctly updated for Header-based SyncResponse.The test constructs a valid SyncResponse with Header and CertifyingQC, and properly references
res.Header.Heightfor the mock expectation.
610-614: Type update reflects interface changes.Using
netint.MessageProcessorinstead ofnetint.Enginecorrectly reflects the removal of public API surface (SubmitLocal, Submit, ProcessLocal) as noted in the AI summary.engine/common/synchronization/engine_test.go (4)
87-90: Good assertions for SyncResponse validation.Properly validates Header, CertifyingQC, and their relationship (BlockID match).
103-123: Test updated correctly for Header-based response.The test properly constructs a SyncResponse with the new fields and validates that
HandleHeightreceives the height fromres.Header.Height.
125-144: Skipped test appropriately deferred.The placeholder test for invalid SyncResponse (with nil CertifyingQC) is correctly skipped with a TODO referencing issue #8174, aligning with the PR description that validation is intentionally deferred to a separate PR.
494-500: Per-iteration header/QC creation is correct.Creating distinct headers and QCs for each iteration ensures the test properly validates processing of multiple items with the new SyncResponse structure.
engine/collection/synchronization/request_handler.go (2)
151-177: Well-structured state access with proper error handling.The implementation correctly:
- Uses a single
finalizedStatesnapshot for bothHead()andQuorumCertificate(), ensuring consistency- Wraps errors with descriptive context for debugging
- Constructs the SyncResponse with the new Header and CertifyingQC fields
This is cleaner than the common implementation due to proper error wrapping.
157-159: TODO appropriately tracked.The TODO(8173) correctly references the related issue for this cleanup. Note that the common implementation uses TODO(8174) - both issues are mentioned as related in the PR description.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @state/cluster/badger/snapshot.go:
- Around line 71-97: The QuorumCertificate method should preserve
storage.ErrNotFound so callers using errors.Is can detect it: after calling
operation.RetrieveBlockChildren in Snapshot.QuorumCertificate, check
errors.Is(err, storage.ErrNotFound) and return that sentinel (or return err
directly) without wrapping it in a new fmt.Errorf; also change the error text to
reference the known block (s.blockID) rather than "finalized head". Do the same
for the RetrieveHeader call (check errors.Is(err, storage.ErrNotFound) and
return the original sentinel/error instead of wrapping) so storage.ErrNotFound
can be detected by callers.
🧹 Nitpick comments (1)
engine/common/synchronization/request_handler.go (1)
158-162: Consider wrapping errors with context for better debugging.The error from
finalizedState.Head()is returned directly without additional context. Per the project's coding guidelines, comprehensive error wrapping aids debugging.Suggested improvement
finalizedState := r.state.Final() finalizedHeader, err := finalizedState.Head() if err != nil { - return err + return fmt.Errorf("could not get finalized header: %w", err) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
engine/collection/synchronization/engine_test.goengine/collection/synchronization/request_handler.goengine/common/synchronization/engine_test.goengine/common/synchronization/request_handler.gomodel/flow/synchronization.gostate/cluster/badger/snapshot.gostate/cluster/snapshot.gostorage/operation/qcs.go
🚧 Files skipped from review as they are similar to previous changes (2)
- engine/collection/synchronization/engine_test.go
- model/flow/synchronization.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/common/synchronization/request_handler.goengine/collection/synchronization/request_handler.goengine/common/synchronization/engine_test.gostate/cluster/badger/snapshot.gostorage/operation/qcs.gostate/cluster/snapshot.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/common/synchronization/request_handler.goengine/collection/synchronization/request_handler.goengine/common/synchronization/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/common/synchronization/request_handler.goengine/collection/synchronization/request_handler.goengine/common/synchronization/engine_test.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
engine/common/synchronization/request_handler.goengine/collection/synchronization/request_handler.goengine/common/synchronization/engine_test.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/common/synchronization/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/operation/qcs.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
storage/operation/qcs.go
🧠 Learnings (3)
📚 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/common/synchronization/request_handler.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 **/*_test.go : Use fixtures for realistic test data as defined in `/utils/unittest/`
Applied to files:
engine/common/synchronization/engine_test.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/operation/qcs.go
🧬 Code graph analysis (5)
engine/common/synchronization/request_handler.go (2)
model/flow/synchronization.go (1)
SyncResponse(14-18)model/messages/synchronization.go (1)
SyncResponse(27-27)
engine/collection/synchronization/request_handler.go (4)
utils/unittest/fixtures/quorum_certificate.go (1)
QuorumCertificate(9-9)model/flow/synchronization.go (1)
SyncResponse(14-18)model/messages/synchronization.go (1)
SyncResponse(27-27)utils/unittest/fixtures/header.go (1)
Header(10-10)
engine/common/synchronization/engine_test.go (2)
utils/unittest/fixtures.go (4)
BlockHeaderFixture(580-600)WithHeaderHeight(549-553)IdentifierFixture(1151-1155)CertifyBlock(2088-2094)model/flow/synchronization.go (1)
SyncResponse(14-18)
state/cluster/badger/snapshot.go (3)
state/cluster/snapshot.go (1)
Snapshot(10-43)storage/operation/children.go (1)
RetrieveBlockChildren(90-108)storage/operation/headers.go (1)
RetrieveHeader(47-49)
state/cluster/snapshot.go (1)
utils/unittest/fixtures/quorum_certificate.go (1)
QuorumCertificate(9-9)
⏰ 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: Unit Tests (consensus)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (state)
- GitHub Check: Integration Tests Others (integration)
- 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 Insecure (insecure)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (9)
storage/operation/qcs.go (1)
26-26: LGTM!Documentation typo fix ("samn" → "same") improves clarity.
state/cluster/snapshot.go (1)
26-31: LGTM!The new
QuorumCertificate()method is well-documented with clear error semantics, following the same pattern as other methods in the interface.engine/common/synchronization/request_handler.go (1)
183-191: LGTM on the response construction with Header and CertifyingQC.The response correctly includes both the finalized header and its certifying QC, enabling receivers to verify the sync response. Same suggestion applies for error wrapping on line 185.
Optional: wrap error with context
qcForFinalizedHeader, err := finalizedState.QuorumCertificate() if err != nil { - return err + return fmt.Errorf("could not get QC for finalized header: %w", err) }engine/common/synchronization/engine_test.go (4)
87-90: Verify pointer-to-value comparison in assertions.Line 87 compares
ss.head(a*flow.Header) withres.Header(aflow.Headervalue). This comparison will work correctly with testify'sassert.Equalsince it dereferences pointers. Same applies to line 89 comparingss.qcwithres.CertifyingQC.The assertions correctly validate the new response structure including QC linkage verification on line 90.
109-123: LGTM!Test correctly constructs a
SyncResponsewith the newHeaderandCertifyingQCfields using theunittest.CertifyBlockfixture helper to generate a properly linked QC.
125-144: Good placeholder for future validation test.The skipped test with TODO reference to issue 8174 is a good reminder to implement SyncResponse validation. Consider adding a brief comment about what behavior should be tested (e.g., response rejection, slashing report).
494-500: LGTM!Per-iteration header creation with properly linked
CertifyingQCensures each test message has a valid, distinct header/QC pair. The assertion on line 500 correctly referencesmsg.Header.Height.engine/collection/synchronization/request_handler.go (2)
151-155: LGTM!The finalized state retrieval and error handling follows proper patterns with descriptive error wrapping.
167-177: LGTM!The QC retrieval and response construction correctly implements the new verifiable SyncResponse structure with
HeaderandCertifyingQC. Error handling is properly wrapped with context.
| // QuorumCertificate returns a valid quorum certificate for the header at this snapshot, if one exists. | ||
| // | ||
| // Expected error returns during normal operations: | ||
| // - [storage.ErrNotFound] is returned if the QC is unknown. | ||
| func (s *Snapshot) QuorumCertificate() (*flow.QuorumCertificate, error) { | ||
| // Implementation detail: QuorumCertificates storage / operation.RetrieveQuorumCertificate only indexes QCs | ||
| // for main consensus blocks, not cluster blocks, so we directly check for any children that would have a | ||
| // QC for the cluster header. | ||
| var pendingIDs flow.IdentifierList | ||
| err := operation.RetrieveBlockChildren(s.state.db.Reader(), s.blockID, &pendingIDs) | ||
| if err != nil { | ||
| // The low-level storage returns `storage.ErrNotFound` in two cases: | ||
| // 1. the block/collection is unknown | ||
| // 2. the block/collection is known but no children have been indexed yet | ||
| // By contract of the constructor, the blockID must correspond to a known collection in the database. | ||
| // A snapshot with s.err == nil is only created for known blocks. Hence, only case 2 is | ||
| // possible here (no children). | ||
| return nil, fmt.Errorf("could not get QC for finalized head %v: %w", s.blockID, err) | ||
| } | ||
| // at least one child exists (pendingIDs[0]) - guaranteed by RetrieveBlockChildren | ||
| var child flow.Header | ||
| err = operation.RetrieveHeader(s.state.db.Reader(), pendingIDs[0], &child) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not retrieve header for unfinalized child %v: %w", pendingIDs[0], err) | ||
| } | ||
| return child.ParentQC(), nil | ||
| } |
There was a problem hiding this comment.
Error wrapping may not preserve storage.ErrNotFound as documented in the interface.
The interface documentation states that [storage.ErrNotFound] is returned if the QC is unknown. However, the error wrapping on line 88 uses fmt.Errorf which may cause callers using errors.Is(err, storage.ErrNotFound) to fail depending on how they check for this condition.
Additionally, the error message "could not get QC for finalized head" is slightly misleading since this snapshot could reference any known block, not necessarily the finalized head.
Suggested improvements
- return nil, fmt.Errorf("could not get QC for finalized head %v: %w", s.blockID, err)
+ return nil, fmt.Errorf("could not get QC for block %v: %w", s.blockID, err)The %w verb should preserve the wrapped error for errors.Is() checks, but verify that callers can properly detect storage.ErrNotFound through this wrapping.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // QuorumCertificate returns a valid quorum certificate for the header at this snapshot, if one exists. | |
| // | |
| // Expected error returns during normal operations: | |
| // - [storage.ErrNotFound] is returned if the QC is unknown. | |
| func (s *Snapshot) QuorumCertificate() (*flow.QuorumCertificate, error) { | |
| // Implementation detail: QuorumCertificates storage / operation.RetrieveQuorumCertificate only indexes QCs | |
| // for main consensus blocks, not cluster blocks, so we directly check for any children that would have a | |
| // QC for the cluster header. | |
| var pendingIDs flow.IdentifierList | |
| err := operation.RetrieveBlockChildren(s.state.db.Reader(), s.blockID, &pendingIDs) | |
| if err != nil { | |
| // The low-level storage returns `storage.ErrNotFound` in two cases: | |
| // 1. the block/collection is unknown | |
| // 2. the block/collection is known but no children have been indexed yet | |
| // By contract of the constructor, the blockID must correspond to a known collection in the database. | |
| // A snapshot with s.err == nil is only created for known blocks. Hence, only case 2 is | |
| // possible here (no children). | |
| return nil, fmt.Errorf("could not get QC for finalized head %v: %w", s.blockID, err) | |
| } | |
| // at least one child exists (pendingIDs[0]) - guaranteed by RetrieveBlockChildren | |
| var child flow.Header | |
| err = operation.RetrieveHeader(s.state.db.Reader(), pendingIDs[0], &child) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not retrieve header for unfinalized child %v: %w", pendingIDs[0], err) | |
| } | |
| return child.ParentQC(), nil | |
| } | |
| // QuorumCertificate returns a valid quorum certificate for the header at this snapshot, if one exists. | |
| // | |
| // Expected error returns during normal operations: | |
| // - [storage.ErrNotFound] is returned if the QC is unknown. | |
| func (s *Snapshot) QuorumCertificate() (*flow.QuorumCertificate, error) { | |
| // Implementation detail: QuorumCertificates storage / operation.RetrieveQuorumCertificate only indexes QCs | |
| // for main consensus blocks, not cluster blocks, so we directly check for any children that would have a | |
| // QC for the cluster header. | |
| var pendingIDs flow.IdentifierList | |
| err := operation.RetrieveBlockChildren(s.state.db.Reader(), s.blockID, &pendingIDs) | |
| if err != nil { | |
| // The low-level storage returns `storage.ErrNotFound` in two cases: | |
| // 1. the block/collection is unknown | |
| // 2. the block/collection is known but no children have been indexed yet | |
| // By contract of the constructor, the blockID must correspond to a known collection in the database. | |
| // A snapshot with s.err == nil is only created for known blocks. Hence, only case 2 is | |
| // possible here (no children). | |
| return nil, fmt.Errorf("could not get QC for block %v: %w", s.blockID, err) | |
| } | |
| // at least one child exists (pendingIDs[0]) - guaranteed by RetrieveBlockChildren | |
| var child flow.Header | |
| err = operation.RetrieveHeader(s.state.db.Reader(), pendingIDs[0], &child) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not retrieve header for unfinalized child %v: %w", pendingIDs[0], err) | |
| } | |
| return child.ParentQC(), nil | |
| } |
🤖 Prompt for AI Agents
In @state/cluster/badger/snapshot.go around lines 71 - 97, The QuorumCertificate
method should preserve storage.ErrNotFound so callers using errors.Is can detect
it: after calling operation.RetrieveBlockChildren in Snapshot.QuorumCertificate,
check errors.Is(err, storage.ErrNotFound) and return that sentinel (or return
err directly) without wrapping it in a new fmt.Errorf; also change the error
text to reference the known block (s.blockID) rather than "finalized head". Do
the same for the RetrieveHeader call (check errors.Is(err, storage.ErrNotFound)
and return the original sentinel/error instead of wrapping) so
storage.ErrNotFound can be detected by callers.
|
|
||
| // if we're sufficiently ahead of the requester, send a response | ||
| res := &messages.SyncResponse{ | ||
| Height: final.Height, |
There was a problem hiding this comment.
This would be a breaking change, and require all the peers to upgrade, right?
I wonder if we can keep the Height field to be backward compatible, so that this change can be rolled out.
There was a problem hiding this comment.
Yes it would be breaking; I think we could keep the Height field for now (with the Height field and current code path being used if and only if the Header/QC is empty)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
engine/common/synchronization/engine_test.go (1)
125-144: Consider enhancing the skipped test stub with clearer expectations.The placeholder test for invalid SyncResponse is appropriately skipped with a TODO reference to issue #8174. However, the test currently calls
ss.e.onSyncResponsebut has no mock expectations set up for the invalid case, which would cause unexpected calls if the test were enabled.When implementing this test, ensure:
- The empty
CertifyingQC(QC with zero-values) is properly detected as invalid- Appropriate misbehavior reporting or logging is verified
HandleHeightshould NOT be called for invalid responsesWould you like me to help sketch out the expected mock setup for when this test is implemented?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
engine/collection/synchronization/engine_test.goengine/common/synchronization/engine_test.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
engine/common/synchronization/engine_test.goengine/collection/synchronization/engine_test.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/common/synchronization/engine_test.goengine/collection/synchronization/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/common/synchronization/engine_test.goengine/collection/synchronization/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/common/synchronization/engine_test.goengine/collection/synchronization/engine_test.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
engine/common/synchronization/engine_test.goengine/collection/synchronization/engine_test.go
🧠 Learnings (1)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to **/*_test.go : Use fixtures for realistic test data as defined in `/utils/unittest/`
Applied to files:
engine/common/synchronization/engine_test.goengine/collection/synchronization/engine_test.go
🧬 Code graph analysis (2)
engine/common/synchronization/engine_test.go (2)
utils/unittest/fixtures.go (4)
BlockHeaderFixture(580-600)WithHeaderHeight(549-553)IdentifierFixture(1151-1155)CertifyBlock(2088-2094)model/flow/synchronization.go (1)
SyncResponse(14-18)
engine/collection/synchronization/engine_test.go (2)
utils/unittest/fixtures.go (3)
CertifyBlock(2088-2094)BlockHeaderFixture(580-600)WithHeaderHeight(549-553)model/flow/synchronization.go (1)
SyncResponse(14-18)
⏰ 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 (network/p2p/node)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (state)
- 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: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (9)
engine/collection/synchronization/engine_test.go (6)
43-43: LGTM! Good use of the CertifyBlock fixture.The test setup properly creates a QC that certifies the head header using the
unittest.CertifyBlockhelper, which correctly setsqc.View = header.Viewandqc.BlockID = header.ID(). This ensures the test data accurately reflects the real relationship between headers and their certifying QCs. Based on learnings, this follows the pattern of using fixtures from/utils/unittest/.Also applies to: 67-68
120-125: LGTM! Snapshot mock correctly extended.The mock properly returns the stored QC and a nil error, matching the expected interface contract for
QuorumCertificate().
195-211: LGTM! Good comprehensive assertions for SyncResponse.The test properly validates:
- Response contains the expected header
- Response contains the expected QC
- QC's BlockID matches the Header's ID (critical for verifiability)
- Nonce and recipient are correct
This provides good coverage for the new verifiable SyncResponse structure.
214-228: LGTM! TestOnSyncResponse correctly updated.The test creates a valid Header+QC pair using the fixture helpers and verifies that
HandleHeightis called with the correct height from the response header.
569-585: LGTM! TestProcessingMultipleItems properly creates distinct Header+QC pairs per iteration.Each iteration correctly creates a unique header with a specific height and a corresponding QC that certifies that header. The mock expectation for
HandleHeightcorrectly referencesmsg.Header.Height.
610-619: LGTM! Type update aligns with interface change.The change from
netint.Enginetonetint.MessageProcessorreflects the removal of the deprecatednetwork.Engineimplementation as noted in the PR description.engine/common/synchronization/engine_test.go (3)
84-101: LGTM! Assertions properly verify QC-Header relationship.The test correctly validates that:
res.Headermatches the expected finalized headerres.CertifyingQCcontains the expected QCres.CertifyingQC.BlockIDequalsss.head.ID()(verifying the QC certifies the header)This provides strong coverage for the new verifiable SyncResponse structure.
103-123: LGTM! TestOnSyncResponse correctly updated for new structure.The test creates a valid Header+QC pair and verifies that
HandleHeightis called withres.Header.Height, correctly reflecting the new data model where height is accessed through the Header.
493-506: LGTM! TestProcessingMultipleItems properly creates per-iteration Header+QC pairs.Each iteration creates a unique header with a deterministic height (
1000 + i) and a corresponding certifying QC. The mock expectations correctly referencemsg.Header.Heightfor theHandleHeightcall.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
engine/common/synchronization/engine.go (1)
299-304: Consider a more robust backwards compatibility check.The current check
res.Header.Height == 0assumes legacy messages will have a zero-initializedHeader. While this works in practice, checkingHeader.Height == 0could theoretically match a valid header at height 0 (genesis).A more explicit approach would be to check if the header's
ParentIDis zero (since only genesis has zero parent, and genesis wouldn't be the finalized head in normal sync scenarios) or check ifCertifyingQC.BlockIDis the zero identifier:💡 Suggested alternative check
// backwards compatibility - ignore the Header/QC if they are not present, and use Height field instead -if res.Header.Height == 0 { +if res.CertifyingQC.BlockID == flow.ZeroID { e.core.HandleHeight(final, res.Height) } else { e.core.HandleHeight(final, res.Header.Height) }This is a minor robustness improvement since genesis sync is an edge case.
engine/common/synchronization/request_handler.go (1)
158-186: Add error wrapping for better debugging.Per coding guidelines, errors should be wrapped for debugging context. The errors at lines 161 and 185 are returned without wrapping, which makes it harder to trace failures in
onSyncRequest.🔧 Suggested fix
func (r *RequestHandler) onSyncRequest(originID flow.Identifier, req *flow.SyncRequest) error { finalizedState := r.state.Final() finalizedHeader, err := finalizedState.Head() if err != nil { - return err + return fmt.Errorf("could not get finalized header: %w", err) } // ... existing code ... // if we're sufficiently ahead of the requester, send a response qcForFinalizedHeader, err := finalizedState.QuorumCertificate() if err != nil { - return err + return fmt.Errorf("could not get QC for finalized header: %w", err) }Based on coding guidelines: "Use comprehensive error wrapping for debugging."
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
engine/collection/synchronization/engine.goengine/collection/synchronization/request_handler.goengine/common/synchronization/engine.goengine/common/synchronization/engine_test.goengine/common/synchronization/request_handler.gomodel/flow/synchronization.go
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/collection/synchronization/engine.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
model/flow/synchronization.goengine/common/synchronization/engine.goengine/common/synchronization/engine_test.goengine/common/synchronization/request_handler.goengine/collection/synchronization/request_handler.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/common/synchronization/engine.goengine/common/synchronization/engine_test.goengine/common/synchronization/request_handler.goengine/collection/synchronization/request_handler.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/common/synchronization/engine.goengine/common/synchronization/engine_test.goengine/common/synchronization/request_handler.goengine/collection/synchronization/request_handler.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
engine/common/synchronization/engine.goengine/common/synchronization/engine_test.goengine/common/synchronization/request_handler.goengine/collection/synchronization/request_handler.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/common/synchronization/engine_test.go
🧠 Learnings (2)
📚 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 **/*_test.go : Use fixtures for realistic test data as defined in `/utils/unittest/`
Applied to files:
engine/common/synchronization/engine_test.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:
engine/common/synchronization/request_handler.go
🧬 Code graph analysis (5)
model/flow/synchronization.go (2)
utils/unittest/fixtures/header.go (1)
Header(10-10)utils/unittest/fixtures/quorum_certificate.go (1)
QuorumCertificate(9-9)
engine/common/synchronization/engine.go (2)
engine/common/synchronization/request_handler.go (1)
NewRequestHandler(67-93)engine/common/synchronization/request_handler_engine.go (1)
NewResponseSender(47-51)
engine/common/synchronization/engine_test.go (5)
utils/unittest/fixtures/header.go (1)
Header(10-10)utils/unittest/fixtures.go (4)
BlockHeaderFixture(580-600)WithHeaderHeight(549-553)IdentifierFixture(1151-1155)CertifyBlock(2088-2094)model/flow/synchronization.go (1)
SyncResponse(14-19)utils/unittest/fixtures/util.go (1)
NoError(6-10)utils/unittest/fixtures/quorum_certificate.go (1)
QuorumCertificate(9-9)
engine/common/synchronization/request_handler.go (2)
model/flow/synchronization.go (1)
SyncResponse(14-19)model/messages/synchronization.go (1)
SyncResponse(27-27)
engine/collection/synchronization/request_handler.go (2)
model/flow/synchronization.go (1)
SyncResponse(14-19)model/messages/synchronization.go (1)
SyncResponse(27-27)
⏰ 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: 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: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: analyze-code (go)
- GitHub Check: Docker Build
🔇 Additional comments (10)
model/flow/synchronization.go (1)
11-19: LGTM! Struct updated correctly for verifiable sync responses.The addition of
HeaderandCertifyingQCfields aligns with the PR objective to makeSyncResponseverifiable. RetainingHeightfor backwards compatibility is appropriate.One consideration: both
HeaderandQuorumCertificateare value types (not pointers), which means they will always be initialized with zero values. The engine code atengine/common/synchronization/engine.go(lines 300-304) usesres.Header.Height == 0to detect legacy messages. This works but relies on the assumption that a valid header will never haveHeight == 0(genesis block edge case). Since genesis is typically height 0, consider whether this could cause issues for nodes syncing from genesis.engine/common/synchronization/engine.go (1)
120-120: LGTM! Constructor updated to pass state dependency.The
stateparameter is correctly passed toNewRequestHandler, enabling finalized header and QC retrieval for response construction.engine/common/synchronization/request_handler.go (2)
54-54: LGTM! State dependency correctly integrated.The
protocol.Statedependency is properly added to the struct, constructor signature, and initialization. This enables finalized header and QC retrieval for constructing verifiable sync responses.Also applies to: 72-72, 82-82
187-192: LGTM! Response construction is correct.The
SyncResponseis properly constructed with all required fields. TheHeaderandCertifyingQCare correctly dereferenced from pointers to value types matching the struct definition. SettingHeightalongsideHeader.Heightmaintains backwards compatibility.engine/common/synchronization/engine_test.go (4)
87-90: LGTM! Assertions correctly validate new response fields.The assertions properly verify:
res.Headermatches the expected headerres.CertifyingQCmatches the expected QCres.CertifyingQC.BlockIDcorresponds tores.Header.ID()(integrity check)This ensures the response contains a valid header-QC pair. Based on learnings, the test correctly uses fixtures from
utils/unittest/.
103-131: LGTM! Test covers both new and legacy response formats.The test properly validates:
- New format:
SyncResponsewithHeaderandCertifyingQCfields (lines 113-121)- Legacy format:
SyncResponsewith onlyHeightfield (lines 123-129)Both paths correctly assert
HandleHeightis called with the appropriate height value.
133-152: Appropriate placeholder for deferred validation test.The skipped test is correctly structured to validate invalid sync responses once QC/Header verification is implemented in the follow-up PR (#8174). The test scenario (valid header with empty QC) correctly represents an invalid state that should be rejected.
501-514: LGTM! Test correctly constructs responses with per-iteration headers.Each iteration creates a unique header and corresponding QC using fixtures. The
HandleHeightexpectation correctly referencesmsg.Header.Height, matching the engine's processing logic for new-format messages.engine/collection/synchronization/request_handler.go (2)
150-187: LGTM! Collection handler correctly updated for verifiable sync responses.The implementation properly:
- Retrieves finalized state via
r.state.Final()(cluster state)- Gets the QC via
finalizedState.QuorumCertificate()with error wrapping- Constructs
SyncResponsewith all new fields (Header,CertifyingQC)Error handling includes proper context wrapping (lines 154, 169-170), which is better than the common handler implementation.
167-178: Good error wrapping here - consider applying same pattern to common handler.This implementation correctly wraps the QC retrieval error with context (line 169-170). The parallel implementation in
engine/common/synchronization/request_handler.go(lines 184-186) returns the error without wrapping. Consider aligning both implementations for consistency.
| final := e.finalizedHeaderCache.Get() | ||
| e.core.HandleHeight(final, res.Height) | ||
| // backwards compatibility - ignore the Header/QC if they are not present, and use Height field instead | ||
| if res.Header.Height == 0 { |
There was a problem hiding this comment.
Can you verify two test cases:
- if an old flow.SyncResponse message is set with a height H, after it's been encoded and decoded to the new flow.SyncResponse message, the res.Height is H, and the res.Header.Height is 0
- if a new flow.SyncResponse message is set with a height H, after it's been encoded and decoded as the old flow.SyncResponse message, the res.Height is H.
There was a problem hiding this comment.
Unfortunately:
- passes - old response is decoded to the new response successfully (with all the values as expected)
- fails - the new response fails to decode as the old response, because the Header and CertifyingQC it receives are unknown fields, and we use a config to enable that error for network messages:
flow-go/model/encoding/cbor/codec.go
Lines 44 to 47 in 18a4624
There was a problem hiding this comment.
So it means, new node can fetch blocks from old node, but old node can not fetch blocks from new node.
I think this is probably ok, and we can still roll it out. Because when rolling out, most of the old nodes are all up to date, they don't need to sync blocks from other peers, once an old node is restarted, it becomes new node, and can still fetch blocks from old nodes or other new nodes.
Instead of just containing a height, SyncResponse now contains a header and QC certifying the header, proving that >2/3rds of nodes on the corresponding chain have reached that height. This PR only updates the SyncResponse struct and makes minor updates to the sync engine (removing the implementation of deprecated
network.Engineand replacing withnetwork.MessageProcessor), and does not add validation for the Header or QC (split into a separate PR).The Height field and code path using that field are retained for backwards compatibility (d14aaee):
SyncResponsewith the real header at Height=0 would imply the sender of theSyncRequestwas at height below 0Issues: #8173 #8174
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.