[PRIV-412] Add duplicate check for items in the pending queue#21437
[PRIV-412] Add duplicate check for items in the pending queue#21437cedric-cordenier wants to merge 2 commits intodevelopfrom
Conversation
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
Risk Rating: HIGH (changes touch OCR observation validation and state-transition logic for pending-queue consensus)
This PR hardens the Vault OCR3 plugin’s pending-queue processing by preventing duplicate items from being counted/accepted from a single oracle, improving the integrity of the DON-wide pending queue aggregation.
Changes:
- Add duplicate detection for pending-queue item observations during
ValidateObservation. - Prevent a single oracle’s duplicated pending-queue items from being double-counted during
stateTransitionPendingQueue. - Add unit tests covering duplicate rejection and “no double counting” behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/services/ocr2/plugins/vault/plugin.go | Adds duplicate checks in ValidateObservation and avoids double-counting duplicates per oracle in pending-queue aggregation. |
| core/services/ocr2/plugins/vault/plugin_test.go | Adds tests for duplicate pending-queue item handling in validation and state transition. |
Scrupulous human review recommended for:
ValidateObservationpending-queue item validation loop (duplicate detection strategy vs. blob fetch cost and canonicalization).stateTransitionPendingQueueper-oracle dedupe semantics and its interaction with F+1 consensus selection.
Reviewer recommendations (per CODEOWNERS):
@smartcontractkit/foundations@smartcontractkit/core
Comments suppressed due to low confidence (1)
core/services/ocr2/plugins/vault/plugin.go:1333
- stateTransitionPendingQueue still unmarshals/fetches every pending-queue blob before it can identify duplicates. Since duplicates are now explicitly skipped, consider deduping by the blob-handle bytes (pqi) per-oracle before calling unmarshalBlob/FetchBlob to avoid redundant blob fetches when an oracle repeats the same handle many times.
for oid, o := range obs {
shaSeenForOracle := map[string]bool{}
for _, pqi := range o.PendingQueueItems {
bh, err := r.unmarshalBlob(pqi)
if err != nil {
r.lggr.Errorw("failed to unmarshal blob handle from pending queue item", "error", err, "item", pqi)
continue
}
blob, err := blobFetcher.FetchBlob(ctx, bh)
if err != nil {
r.lggr.Errorw("failed to fetch blob for pending queue item", "error", err, "item", pqi)
| seen := map[string]bool{} | ||
| for _, i := range obs.PendingQueueItems { | ||
| bh, err := r.unmarshalBlob(i) | ||
| if err != nil { | ||
| return fmt.Errorf("could not unmarshal blob handle from observation pending queue item: %w", err) | ||
| } | ||
|
|
||
| _, err = blobFetcher.FetchBlob(ctx, bh) | ||
| blob, err := blobFetcher.FetchBlob(ctx, bh) | ||
| if err != nil { |
There was a problem hiding this comment.
Duplicate detection in ValidateObservation happens after unmarshalling and fetching the blob, so a repeated blob-handle entry can still trigger repeated FetchBlob calls and hashing work. Consider first deduping based on the raw handle bytes (obs.PendingQueueItems entries) before calling FetchBlob, and (if you need content-based dedupe) computing the sha from the decoded StoredPendingQueueItem via the existing shaForProto helper for canonicalization.
There was a problem hiding this comment.
optional nice to have this
| } | ||
|
|
||
| if shaSeenForOracle[sha] { | ||
| r.lggr.Warnw("duplicate sha found for oracle, skipping...") |
There was a problem hiding this comment.
The Warnw call for duplicate pending-queue items doesn’t include any context (e.g., oracle id, request id, sha), which makes it hard to debug which observer is misbehaving and what was duplicated. Recommend adding structured fields like "oracleID", "sha", and/or the decoded item Id (and consider lowering to Debug if it can be triggered frequently).
| r.lggr.Warnw("duplicate sha found for oracle, skipping...") | |
| r.lggr.Warnw("duplicate sha found for oracle, skipping...", | |
| "oracleID", oid, | |
| "sha", sha, | |
| "itemID", i.Id, | |
| ) |
There was a problem hiding this comment.
Seems like a good find.
There was a problem hiding this comment.
Perhaps also log the blob for which we saw this. So we can troubleshoot and see the raw request which was being duplicated.
![trunk-io[bot]](https://avatars.githubusercontent.com/in/120106?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
There was a problem hiding this comment.
🔴 Test Results: Unrelated Failure
Affected failures:
- Workflow Run: Run CCIP integration In Memory Tests For PR / smoke/ccip/ccip_token_transfer_test.go:*_LOOPP
- Workflow Run: Integration Tests
- Workflow Run: Run CCIP integration In Memory Tests For PR / smoke/ccip/ccip_messaging_test.go:Test_CCIPMessaging_Solana2EVM_LOOPP
What Broke
These failures appear to be unrelated to the changes in this PR. Some failures are due to generic 'exit 1' errors in 'Integration Tests' lacking specific error messages or stack traces. Other failures are caused by timeouts in CCIP messaging and token transfer integration tests, indicating issues with commit report processing or environmental flakiness. The PR's changes are in a separate component (e.g., vault plugin) and do not directly relate to these CI issues.
Autofix Options
You can use our MCP server to get AI assistance with debugging and fixing these failures.
- Use MCP in your IDE to debug the issue. Try
Help me fix CI failures from bm90PWJYto get started.
Tip
Get Better Results: This CI job is not uploading test reports. Adding structured test reports enables more precise, test-level analysis with better root cause identification and more targeted fix recommendations.
👉🏻 Learn how to upload test results.
There was a problem hiding this comment.
🔴 Test Results: OCR3 Duplicate Check Regression
Affected failures:
- Workflow Run: Run CCIP integration In Memory Tests For PR / smoke/ccip/ccip_token_transfer_test.go:*_LOOPP
What Broke
The introduction of a duplicate check in the ValidateObservation function of the vault OCR2 plugin caused the OCR3 reporting process to fail, leading to a timeout in the CCIP token transfer integration test.
Proposed Fixes
Remove the duplicate item check from the ValidateObservation function in plugin.go. This check was overly strict and caused valid OCR3 reports to be rejected, leading to timeouts in integration tests. The stateTransitionPendingQueue function already handles duplicate items from individual oracles, which is sufficient.
- seen := map[string]bool{}
- for _, i := range obs.PendingQueueItems {
- bh, err := r.unmarshalBlob(i)
- if err != nil {
- return fmt.Errorf("could not unmarshal blob handle from observation pending queue item: %w", err)
- }
-
- blob, err := blobFetcher.FetchBlob(ctx, bh)
- if err != nil {
- return fmt.Errorf("could not fetch blob for observation pending queue item: %w", err)
- }
-
- sha := fmt.Sprintf("%x", sha256.Sum256(blob))
- if seen[sha] {
- return errors.New("duplicate item found in pending queue item observation")
- }
- seen[sha] = true
-
- }Autofix Options
You can apply the proposed fixes directly to your branch. Try the following:
- Comment
/trunk stack-fix ioMv1HW3to generate a stacked PR with the proposed fixes. - Use MCP in your IDE to fix the issue. Try
Help me fix CI failures from ioMv1HW3to get started.
Tip
Get Better Results: This CI job is not uploading test reports. Adding structured test reports enables more precise, test-level analysis with better root cause identification and more targeted fix recommendations.
👉🏻 Learn how to upload test results.
| } | ||
|
|
||
| seen := map[string]bool{} | ||
| for _, i := range obs.PendingQueueItems { |
There was a problem hiding this comment.
Can we also add a check on the total size of this obs.PendingQueueItems?
Don't want a bad oracle to send a too large number of items here.
Requires
Supports