feat(stats): report what the chain did, not only that it accepted - #78
feat(stats): report what the chain did, not only that it accepted#78bdchatham wants to merge 4 commits into
Conversation
The run report named no outcome at all. Every count existed in the collector and none of it reached the operator, so a run whose transactions all reverted printed the same send rate and the same latency distribution as a run that did everything it claimed. The new section prints three layers and never totals across them. Committed and reverted are what the chain did. Expired and the two drop counts are what this run did to its own transactions. StatusUnavailable and InflightAtShutdown are what it did not observe (TOT-012, TOT-019). Three arms, and the difference between two of them is the point. A run that tried to measure and failed prints its zeros under a warning, because an absent number reads as zero (TOT-013). A run with --track-receipts off prints no counts at all, because "committed 0" for a run that never asked manufactures a chain finding out of a flag setting. The counts come from the collector, not from InclusionSummary.Included, which adds committed and reverted together. That conflation is the defect this feature removes. main reads the inclusion summary before printing rather than after. The report could not name an outcome while it ran first. The old closing log line goes with it: it printed the conflated included= and would now contradict the report on the same stdout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every guard broken on purpose first: an all-expired run mislabelled as a measurement failure, an untracked run printing counts, the revert share taken over accepted rather than executed, and the status_unavailable row dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR SummaryLow Risk Overview Introduces
Reviewed by Cursor Bugbot for commit ca26b9e. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
@seidroid review |
…lt-1078-outcome-report
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 021beff. Configure here.
| b.WriteString(fmt.Sprintf("Of the %d the chain executed, %d reverted (%.2f%%).\n", | ||
| ex, e.Reverted, float64(e.Reverted)/float64(ex)*100)) | ||
| } else { | ||
| b.WriteString("Check block_fetch_errors and the inclusion tracker log for the cause.\n") |
There was a problem hiding this comment.
Zero-execution footer misdiagnoses chain findings
Medium Severity
The tracked-run footer treats every zero executed total as an instrument failure and always points at block_fetch_errors. An all-expired run already read every block and left those transactions out, which is a chain finding. That line reintroduces the measurement-failure diagnosis wholeRunUnavailable is written to avoid.
Reviewed by Cursor Bugbot for commit 021beff. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a well-structured, well-tested === Transaction Outcomes === section to the final report, sourcing execution counts from the collector instead of the conflated InclusionSummary.Included, and reorders main so the inclusion summary is read before the report prints. The design and tests are solid; a few edges overstate or hide information the section exists to make honest.
Findings: 0 blocking | 5 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
status_unavailablenow has two different values in one run: the printed report uses the collector'sOutcomeStatusUnavailabletally, whileEmitRunSummaryand the⚠️log line still useInclusionSummary.StatusUnavailable, which folds ins.duplicates(stats/inclusion_tracker.go:1004). An operator comparing stdout to the emitted run summary sees the same name with different numbers. The PR notes the duplicate folding as out of scope; at minimum consider documenting the divergence where the two are produced, so the mismatch reads as known rather than as a counting bug. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
Re-review of PR #78 at the same head as the previous seidroid review (no new commits since 021beff), so all four prior inline findings are still present and are re-reported here. The === Transaction Outcomes === section itself is well-structured and well-tested — counts sourced from the collector rather than the conflated InclusionSummary.Included, and main reordered so the inclusion summary is read before the report prints.
Findings: 0 blocking | 6 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
status_unavailablestill has two different values in one run: the printed report uses the collector'sOutcomeStatusUnavailabletally, whileEmitRunSummaryand the⚠️log line useInclusionSummary.StatusUnavailable, which folds ins.duplicates(stats/inclusion_tracker.go:1004). Removing the📦 Inclusion:line also means stdout no longer carries the tracker's own tallies at all, so the two views can only be compared against the emitted run summary — where the same name now shows different numbers. The PR notes the duplicate folding as out of scope; consider documenting the divergence where the two are produced so the mismatch reads as known rather than as a counting bug. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| // Print final statistics | ||
| logger.LogFinalStats(inclusionSummary, cfg.Settings.InclusionReapAfter.ToDuration()) |
There was a problem hiding this comment.
[suggestion] Still unaddressed from the previous review (no commits since). This passes the raw config value, but the tracker floors a non-positive reapAfter to 30s (stats/inclusion_tracker.go:231-233). With inclusionReapAfter: 0, transactions are actually reaped after 30s while the report prints (reaped after 0s) — the deadline is precisely the number that makes expired interpretable, so a wrong one is worse than none. Consider carrying the effective reapAfter out of the tracker (e.g. as a field on InclusionSummary) and printing that instead.
| // and a report carrying several conditional warnings teaches its reader to skip | ||
| // warnings. | ||
| func (e ExecutionOutcomes) everythingReverted() bool { | ||
| return e.Tracked && e.Committed == 0 && e.Reverted > 0 |
There was a problem hiding this comment.
[suggestion] Still unaddressed from the previous review (no commits since). everythingReverted ignores coverage, so the warning it gates can overstate. With Accepted=1000, Committed=0, Reverted=40, StatusUnavailable=960, wholeRunUnavailable is false (Reverted > 0) and this fires, printing "The chain took 40 and every one reverted. This run did no work on-chain." — a chain-wide claim at 4% coverage, while 960 statuses went unread and may well have committed. That is the same manufactured chain finding the rest of the type guards against, and the coverage line only appears well below the warning. Consider scoping the sentence to what was observed ("of the 40 statuses read, none committed") or requiring meaningful coverage before asserting the run did no work on-chain.
| func (e ExecutionOutcomes) unaccounted() uint64 { | ||
| placed := e.Committed + e.Reverted + e.Expired + e.DroppedAtCap + | ||
| e.DroppedAtHandoff + e.StatusUnavailable + e.Unrecorded + e.InflightAtShutdown | ||
| if placed > e.Accepted { |
There was a problem hiding this comment.
[suggestion] Still unaddressed from the previous review (no commits since). The clamp makes the residual one-sided: an over-count is exactly the counting bug this figure is documented to catch, but placed > Accepted renders as unaccounted 0, which the section header ("Bookkeeping: both must be 0") reads as clean. E.g. the tracker recording a terminal outcome twice for one hash, or RecordOutcome creating a perOperation entry for a key the send path never counted a success under, silently passes. Guarding against uint64 underflow is right, but consider surfacing the overflow direction — print the over-count as a distinct row or note rather than collapsing it to the healthy value.
| // Tracked is derived from whether a run carried an inclusion tracker, never | ||
| // set by hand. A bool that can disagree with the counts beside it is the | ||
| // defect this type exists to prevent. | ||
| Tracked bool |
There was a problem hiding this comment.
[suggestion] Still unaddressed from the previous review (no commits since). Tracked has no json tag, so it serialises as "Tracked" among otherwise snake_case keys — and since FinalStats.Execution is not omitted for a zero value, an untracked run writes execution_outcomes with committed: 0, reverted: 0 into the JSON report file. The text arm deliberately prints no counts in that case because "committed 0" for a run that never asked manufactures a chain finding out of a flag setting; a JSON consumer gets exactly those zeros and must know to check a differently-cased key first. Suggest json:"tracked" at minimum.
|
|
||
| if !e.Tracked { | ||
| b.WriteString(fmt.Sprintf( | ||
| "Execution status was not tracked: this run had --track-receipts off.\n"+ |
There was a problem hiding this comment.
[suggestion] Tracked is false whenever the tracker was not constructed, which is also the case when --track-receipts is on but --dry-run is set or endpoints is empty (main.go:294). In those runs this prints "this run had --track-receipts off" as a fact about the operator's flags when it was actually on, and then advises re-running with the flag already in use. That is the same class of unwarranted claim the rest of this type is built to avoid. Consider a flag-neutral phrasing ("execution status was not tracked for this run") or plumbing the reason through so the remedy line matches.
golangci-lint runs staticcheck with the QF checks on, which bare staticcheck does not, so these three only appeared under make lint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
TOT-012, TOT-013, TOT-019. Fifth in the outcome-tracking stack, and the one an operator sees.
The run report named no outcome at all. Every count already existed in the collector and none of it reached the report, so a run whose transactions all reverted printed the same send rate and the same latency distribution as a run that did everything it claimed.
What it prints
Three layers, never totalled across. What the chain did, what this run did to its own transactions, and what it failed to observe. Totalling them makes a generator throttling itself look like a chain rejecting work.
Three arms, and the difference between two of them is the point
A run that tried to measure and failed prints its zeros under a warning, because an absent number reads as zero (TOT-013). No row inside that arm is conditional on being non-zero.
A run with
--track-receiptsoff prints no counts at all. "committed 0" for a run that never asked manufactures a chain finding out of a flag setting.The guard against the mirror defect: a run whose transactions all expired does not take the unavailable banner. That run read every block and saw nothing, which is a claim it is entitled to make. Calling it a measurement failure is the same invention pointed the other way.
Two things this had to fix to work
The counts come from the collector, not
InclusionSummary.Included. That field increments for bothOutcomeCommittedandOutcomeReverted(inclusion_tracker.go:664-669) — it is the conflation this feature removes.mainreads the inclusion summary before printing, not after. The report ran first and so could never name an outcome. The old📦 Inclusion:log line goes with it: it printed the conflatedincluded=and would now contradict the report on the same stdout.The revert denominator
Of the N the chain executeddivides by committed + reverted, not by accepted. Dividing by accepted understates the problem in proportion to how much of the run went unobserved — at half coverage the two answers differ by a factor of two.Checked
Every guard broken on purpose: an all-expired run mislabelled as a measurement failure, an untracked run printing counts, the revert share taken over accepted, and the
status_unavailablerow dropped. All four caught.gofmt,go vet, full suite clean.Known
InclusionSummary.StatusUnavailablefolds in duplicate registrations, which are resend collisions rather than measurement failures, so they inflate the one count TOT-019 exists to protect. Not fixed here; it needs an outcome state the spec does not have.