Skip to content

Conversation

@fbac
Copy link
Collaborator

@fbac fbac commented Oct 10, 2025

Refactor pkg/payerreport attestation verification by changing payerreport.getMinuteFromEnvelope to return only an int32 minute and updating payerreport.PayerReportVerifier.IsValidReport to return true for empty reports to fix the attestation worker critical path

This pull request adjusts minute extraction utilities and report verification flow in pkg/payerreport to streamline attestation verification. It changes payerreport.getMinuteFromEnvelope to return only an int32 minute value and updates payerreport.getMinuteFromSequenceID accordingly. It refactors payerreport.PayerReportVerifier.IsValidReport to immediately return true for empty reports and to propagate the boolean from verifyMerkleRoot with any error. It modifies payerreport.PayerReportVerifier.verifyMerkleRoot to treat invalid-report conditions as (false, nil) and to reserve error returns for system failures. It lowers the log level in payerreport.PayerReportVerifier.isAtMinuteEnd for non-terminal sequence IDs.

  • Refactor payerreport.PayerReportVerifier.verifyMerkleRoot to return (false, nil) for invalid-report states and keep errors for system failures in verifier.go
  • Change payerreport.getMinuteFromEnvelope to return only an int32 minute and update payerreport.getMinuteFromSequenceID in utils.go
  • Update payerreport.PayerReportVerifier.IsValidReport to return true for empty reports and propagate verifyMerkleRoot results in verifier.go
  • Lower log severity to debug in payerreport.PayerReportVerifier.isAtMinuteEnd in verifier.go

📍Where to Start

Start with the payerreport.PayerReportVerifier.IsValidReport method in verifier.go, then follow into payerreport.PayerReportVerifier.verifyMerkleRoot and the updated payerreport.getMinuteFromEnvelope in utils.go.


Changes since #1238 opened

  • Removed exported error variables from payerreport package [f57212d]
  • Updated test cases to remove references to deleted error variables [f57212d]

📊 Macroscope summarized f57212d. 2 files reviewed, 3 issues evaluated, 2 issues filtered, 1 comment posted

🗂️ Filtered Issues

pkg/payerreport/utils.go — 0 comments posted, 1 evaluated, 1 filtered
  • line 35: Potential silent data loss during time-to-minute conversion: getMinuteFromEnvelope returns int32 minutes by calling utils.MinutesSinceEpoch, which casts to int32. If the envelope time is far enough from the epoch, the minute count can overflow/truncate, yielding incorrect values without error. These minutes are then used in database queries (BuildPayerReport and GetLastSequenceIDForOriginatorMinute), potentially causing incorrect lookups and validation outcomes. [ Previously rejected ]
pkg/payerreport/verifier.go — 1 comment posted, 2 evaluated, 1 filtered
  • line 108: Contract and error-semantics change in verifyMerkleRoot: several conditions that previously returned specific errors (e.g., ErrInvalidReportStart, ErrMessageNotAtMinuteEnd, ErrMerkleRootMismatch) now return (false, nil), classifying them as invalid reports rather than errors. Additionally, the overflow case for OriginatorNodeID now returns the raw error from utils.Uint32ToInt32 instead of the prior sentinel ErrInvalidOriginatorID. These changes alter the externally visible contract—callers relying on specific errors to differentiate invalid-input vs. system-error cases will observe different behavior. [ Low confidence ]

@fbac fbac requested a review from a team as a code owner October 10, 2025 16:43
@graphite-app
Copy link

graphite-app bot commented Oct 10, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • Queue - adds this PR to the back of the merge queue
  • Hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@fbac fbac force-pushed the 10-10-fix_critical_attestation_path branch 5 times, most recently from 0140e07 to 60ab95d Compare October 10, 2025 16:58
@fbac fbac force-pushed the 10-10-fix_critical_attestation_path branch from 60ab95d to 2adb0f3 Compare October 10, 2025 17:24
@fbac fbac merged commit 42dca90 into main Oct 10, 2025
11 checks passed
@fbac fbac deleted the 10-10-fix_critical_attestation_path branch October 10, 2025 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants