Skip to content

Conversation

@fbac
Copy link
Collaborator

@fbac fbac commented Oct 21, 2025

Return structured payerreport.VerifyReportResult from payerreport.Verifier.VerifyReport and update attestation worker logs to bubble up verifier reasons

Rename payerreport.Verifier.IsValidReport to payerreport.Verifier.VerifyReport and change its return type to payerreport.VerifyReportResult with IsValid and Reason. Update attestation worker to log utils.ReasonField and act on the structured result; add utils.ReasonField and switch mint success logs to utils.AmountField. Adjust tests and mocks to the new interface and result type. Note: fix a Go variable shadowing issue in VerifyReport where err is redeclared.

📍Where to Start

Start with the VerifyReport implementation in verifier.go, then review its callers in attestation.go and the updated tests in verifier_test.go.

Changes since #1264 opened

  • Refactored variable declaration and removed unused code in PayerReportVerifier.VerifyReport method [9744de8]

📊 Macroscope summarized 9744de8. 5 files reviewed, 2 issues evaluated, 2 issues filtered, 0 comments posted

🗂️ Filtered Issues

pkg/payerreport/verifier.go — 0 comments posted, 2 evaluated, 2 filtered
  • line 136: VerifyReport treats reports with StartSequenceID == EndSequenceID as valid without verifying that PayersMerkleRoot equals the canonical hash of an empty set. The comment explicitly states the merkle root must always be the hash of an empty set, but the implementation returns IsValid: true unconditionally. This allows malformed empty reports with an incorrect PayersMerkleRoot to be accepted as valid, leading to incorrect attestations and downstream state inconsistency. [ Low confidence ]
  • line 204: verifyMerkleRoot converts report.EndSequenceID from uint64 to int64 using a direct cast int64(report.EndSequenceID) when calling isAtMinuteEnd. If EndSequenceID exceeds math.MaxInt64, this conversion will wrap to a negative value, causing incorrect validation (e.g., mismatching the last sequence ID) and potentially rejecting valid reports. The rest of the codebase uses utils.Uint64ToInt64 to guard against overflow; the same check should be applied here to avoid silent overflow and logic errors. [ Low confidence ]

@fbac fbac requested a review from a team as a code owner October 21, 2025 18:06
@graphite-app
Copy link

graphite-app bot commented Oct 21, 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 merged commit 75899c1 into main Oct 26, 2025
11 checks passed
@fbac fbac deleted the 10-21-is_valid_report_bubble_err branch October 26, 2025 21:21
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