Skip to content

Only mark validator sets aggregated for valset proofs#465

Merged
oxsteins merged 3 commits into
symbioticfi:mainfrom
Trynax:fix/only-valset-proof-updates-status
May 28, 2026
Merged

Only mark validator sets aggregated for valset proofs#465
oxsteins merged 3 commits into
symbioticfi:mainfrom
Trynax:fix/only-valset-proof-updates-status

Conversation

@Trynax

@Trynax Trynax commented May 28, 2026 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

User description

closes #464

PR Type

Bug fix, Tests

Description

  • Restrict valset aggregation status updates

  • Match proofs against valset metadata

  • Ignore unrelated epoch-bound aggregation proofs

  • Add regression coverage for both paths

Diagram Walkthrough

flowchart LR
  proof["Aggregation proof received"]
  save["Save aggregation proof"]
  meta["Load validator set metadata"]
  match["Request IDs match?"]
  update["Mark valset as HeaderAggregated"]
  done["Emit proof signal and finish"]

  proof -- "process" --> save
  save -- "then" --> meta
  meta -- "check" --> match
  match -- "yes" --> update
  match -- "no" --> done
  update -- "continue" --> done
Loading

File Walkthrough
Relevant files
Bug fix
entity_processor.go
Guard valset aggregation status by request match                 

internal/usecase/entity-processor/entity_processor.go

  • Add GetValidatorSetMetadata to repository contract
  • Replace unconditional status update with guarded helper
  • Update status only when RequestID matches metadata
  • Ignore missing metadata instead of failing
 +23/-2   
Tests
entity_processor_test.go
Cover matching and non-matching proof behavior                     

internal/usecase/entity-processor/entity_processor_test.go

  • Add regression test for unrelated proofs
  • Verify unrelated proofs keep valset unaggregated
  • Add positive test for matching valset proofs
  • Confirm latest aggregated header updates correctly
 +122/-0 
entity_processor.go
Add repository mock for metadata lookup                                   

internal/usecase/entity-processor/mocks/entity_processor.go

  • Generate mock support for GetValidatorSetMetadata
  • Enable tests and callers to expect metadata lookups
 +15/-0   

@github-actions

github-actions Bot commented May 28, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

464 - Fully compliant

Compliant requirements:

  • Only mark a validator set as aggregated when the processed aggregation proof corresponds to the dedicated valset-header request for that epoch.
  • Ignore unrelated epoch-bound aggregation proofs so they do not advance validator-set aggregation state.
  • Prevent unrelated proofs from affecting the latest aggregated validator set tracking.
  • Add regression coverage for both the matching and non-matching proof paths.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ No major issues detected

@Trynax

Trynax commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

@oxsteins

chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed May 28, 2026
View reviewed changes

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c847f1e7f0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread internal/usecase/entity-processor/entity_processor.go Outdated
}

func (s *EntityProcessor) markValidatorSetAggregatedIfValsetProof(ctx context.Context, aggregationProof symbiotic.AggregationProof) error {
metadata, err := s.cfg.Repo.GetValidatorSetMetadata(ctx, aggregationProof.Epoch)

@chatgpt-codex-connector chatgpt-codex-connector Bot May 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Look up valset metadata by target epoch

For normal next-valset header proofs, this uses the signing epoch, not the validator-set epoch whose metadata was stored: valset_listener_uc.go builds the signature request with RequiredEpoch: prevValSet.Epoch but stores ValidatorSetMetadata{Epoch: valSet.Epoch, RequestID: extendedSig.RequestID()}. When that aggregation proof is processed, aggregationProof.Epoch is therefore the previous epoch, so this lookup either misses (and is silently ignored as ErrEntityNotFound) or compares against the previous valset's metadata, leaving the new valset status/latest-aggregated epoch unupdated even though the matching proof was saved.

Useful? React with 👍 / 👎.

@github-actions

github-actions Bot commented May 28, 2026

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Check metadata before persisting

GetValidatorSetMetadata now runs only after SaveProof, so a transient lookup failure
can leave the proof persisted while ProcessAggregationProof returns an error. A
retry will likely hit the existing proof and never retry the validator-set update,
so resolve whether this is a valset proof before persisting the proof.

internal/usecase/entity-processor/entity_processor.go [189-196]

+shouldMarkValset, err := s.shouldMarkValidatorSetAggregated(ctx, aggregationProof)
+if err != nil {
+	tracing.RecordError(span, err)
+	return err
+}
+
 if err := s.cfg.Repo.SaveProof(ctx, aggregationProof); err != nil {
 	tracing.RecordError(span, err)
 	return errors.Errorf("failed to add aggregation proof: %w", err)
 }
 
-if err := s.markValidatorSetAggregatedIfValsetProof(ctx, aggregationProof); err != nil {
-	return err
+if shouldMarkValset {
+	if err := s.cfg.Repo.UpdateValidatorSetStatus(ctx, aggregationProof.Epoch, symbiotic.HeaderAggregated); err != nil {
+		return errors.Errorf("failed to update validator set status: %w", err)
+	}
 }
Suggestion importance[1-10]: 7

__

Why: This identifies a real partial-failure risk in ProcessAggregationProof: SaveProof can succeed and then GetValidatorSetMetadata can fail, leaving persisted state that may block a clean retry. The suggested reordering is relevant and mostly correct, although it does not fully remove all possible post-save inconsistency paths in this flow.

Medium

@sentry

sentry Bot commented May 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ernal/usecase/entity-processor/entity_processor.go 83.33% 2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@Trynax

Trynax commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

gm @oxsteins , I just pushed some changes based on the suggestions.

Mainly moved the valset metadata check before saving the proof, and updated it to use the target valset epoch.

@oxsteins oxsteins merged commit c5107fb into symbioticfi:main May 28, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

@oxsteins oxsteins oxsteins approved these changes

Assignees

No one assigned

Labels

Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Aggregation proofs may be marking validator sets as aggregated too broadly

2 participants

@Trynax @oxsteins