fix: return values correctly not nil

[chatton]

Overview

I think this can close off the main feature issues, for any follow up improvements we can make new issues.

closes #2956

Before this fix I tried enabling tracing but saw nothing and discovered there was a panic due to the values not being returned correctly in the error case in the decorator. Tracing should propagate correctly now

[claude]

Claude finished @chatton's task in 1m 29s —— View job

---

Code Review Complete ✅

Tasks

• Read changed files (tracing.go and tracing_test.go)
• Review the git diff to understand exact changes
• Analyze the fix for correctness and Go best practices
• Review test coverage and quality
• Provide comprehensive code review feedback

---

Summary

This PR correctly fixes a critical bug in the tracing decorator where return values were being discarded and replaced with nil/zero values in error cases. The fix is excellent and follows Go best practices. The added test coverage is comprehensive and well-designed.

🎯 Code Quality & Architecture

Strengths:

• Correct Go idiom: The fix properly follows Go's convention of returning valid partial results alongside errors. This is especially important for decorator patterns that must transparently pass through both values and errors.
• Minimal, surgical change: Only 3 lines changed in the implementation—exactly what's needed, nothing more. No over-engineering.
• Clean decorator pattern: The tracing decorator correctly wraps the inner BlockProducer without altering its behavior, only adding observability.
• Proper interface compliance: Maintains the BlockProducer interface contract correctly.

Architecture observations:

• The decorator pattern implementation is clean and idiomatic (block/internal/executing/tracing.go:15-29)
• OpenTelemetry integration follows best practices with proper span lifecycle management
• Context propagation is handled correctly throughout

✅ Error Handling & Resilience

Excellent error handling:

1. Preserves original return values (tracing.go:51, 80, 98): The core fix ensures that when the inner function returns (value, error), both are propagated correctly. This is critical because:

   • Some error scenarios return partial but valid data (e.g., ErrNoTransactionsInBatch with an empty batch)
   • Discarding these values can cause nil pointer panics downstream
   • Callers may need to inspect the partial data to understand the error context

2. Proper error recording: All error paths correctly record the error to the span and set error status codes

3. No error swallowing: Original errors are always returned unchanged

4. Consistent pattern: All three fixed methods follow the same correct pattern

Before vs After:

// BEFORE (incorrect):
batchData, err := t.inner.RetrieveBatch(ctx)
if err != nil {
    span.RecordError(err)
    span.SetStatus(codes.Error, err.Error())
    return nil, err  // ❌ Discards potentially valid batchData
}

// AFTER (correct):
batchData, err := t.inner.RetrieveBatch(ctx)
if err != nil {
    span.RecordError(err)
    span.SetStatus(codes.Error, err.Error())
    return batchData, err  // ✅ Preserves batchData value
}

🔒 Security

No security concerns identified:

• No credentials or sensitive data exposed in traces
• Error messages are safely propagated without modification
• No injection vectors introduced
• Trace attributes only include non-sensitive metadata (heights, counts, hashes)

⚡ Performance & Resource Efficiency

Excellent performance characteristics:

• Zero additional allocations introduced by the fix
• No blocking operations or goroutine lifecycle issues
• Span lifecycle properly managed with defer span.End()
• No resource leaks in error paths

Minor observation (not blocking):

• Line tracing.go:102 encodes the app hash to hex on every successful ApplyBlock. This is fine for the current usage, but if tracing becomes a hot path, consider whether this encoding could be deferred to the trace backend.

🧪 Testing & Reliability

Outstanding test coverage:

1. Three new tests perfectly validate the fix (tracing_test.go:324-419):

   • TestTracedBlockProducer_RetrieveBatch_ErrorWithValue: Verifies batch data is preserved with error
   • TestTracedBlockProducer_CreateBlock_ErrorWithValue: Verifies header and data are preserved with error
   • TestTracedBlockProducer_ApplyBlock_ErrorWithValue: Verifies state is preserved with error

2. Test quality highlights:

   • Clear documentation: Each test has excellent comments explaining the "why" (e.g., lines 324-326)
   • Precise assertions: Uses require.Same to verify identity, not just equality—ensuring no copies are made
   • Complete coverage: Tests verify both the error is returned AND the value is preserved
   • Span validation: All tests verify tracing behavior remains correct even with the fix

3. Existing test suite: The PR maintains all existing tests (18 test cases total), ensuring no regression in success paths or pure error paths.

Test pattern example (very well done):

// Clear intent and comprehensive validation
batch, err := producer.RetrieveBatch(ctx)
require.Error(t, err)
require.Equal(t, "no transactions in batch", err.Error())
require.NotNil(t, batch, "batch should not be nil when inner returns value with error")
require.Same(t, expectedBatch, batch, "batch should be the same instance returned by inner")

📚 Documentation & Developer Experience

Good documentation:

• Test comments clearly explain the purpose and importance of the fix
• Function comments on the decorator remain accurate
• The PR description includes a screenshot showing the tracing now works correctly

Suggestion for future enhancement (not blocking):
Consider adding a package-level comment or doc.go explaining the decorator's role and that it preserves all return values transparently, including partial results in error cases. This would help future maintainers understand the design intent.

🎯 Specific Review Comments

tracing.go:51 - RetrieveBatch fix ✅

return batchData, err  // Previously: return nil, err

Excellent. This correctly preserves the batch data returned by the inner producer, which may contain valid information even when an error occurs (e.g., an empty batch with ErrNoTransactionsInBatch).

tracing.go:80 - CreateBlock fix ✅

return header, data, err  // Previously: return nil, nil, err

Excellent. Properly preserves both header and data. Critical for debugging and error context.

tracing.go:98 - ApplyBlock fix ✅

return state, err  // Previously: return types.State{}, err

Excellent. Preserves the state struct instead of returning a zero value. This is important because types.State is a struct (not a pointer), and the inner implementation may populate fields even on error.

Test assertion style (tracing_test.go:412-413)

The field-by-field assertions for types.State are appropriate here. While Gemini suggested using require.Equal(t, expectedState, state), the current approach is actually better because:

1. It's more explicit about which fields matter for this test
2. It's more resilient to future additions of fields to types.State that might not be relevant to this test
3. The assertion messages clearly document the intent

No changes needed.

✨ Overall Assessment

This is a high-quality fix that:

• ✅ Solves the panic issue correctly
• ✅ Follows Go best practices for error handling
• ✅ Maintains the decorator pattern correctly
• ✅ Has excellent test coverage
• ✅ Introduces no performance or security concerns
• ✅ Is minimal and surgical—changes only what's needed

Recommendation: Approve and merge ✅

The PR is ready to close out the tracing feature. The fix is correct, well-tested, and follows ev-node's coding standards.

Minor Suggestions for Follow-up (Optional)

These are not blocking but could be considered for future improvements:

1. Consider adding package documentation explaining the tracing decorator's transparent pass-through behavior
2. If tracing becomes performance-critical, profile the hex encoding in ApplyBlock success path
3. Consider integration tests that exercise the full tracing pipeline with real error scenarios

---

[gemini-code-assist]

Summary of Changes

Hello @chatton, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves an issue within the tracing decorator where it was incorrectly discarding valid return values by replacing them with nil or zero-value types when an error was encountered. The change ensures that the original return values are preserved and passed through, even in error scenarios, which is crucial for maintaining data integrity and preventing unexpected panics when tracing is active. This improves the robustness of the tracing mechanism and provides more complete context during error handling.

Highlights

• Tracing Decorator Fix: Corrected the tracedBlockProducer methods (RetrieveBatch, CreateBlock, ApplyBlock) to ensure that non-nil return values are propagated correctly even when an error occurs. Previously, these methods would return nil or zero-value structs on error, potentially leading to panics or loss of valuable context.
• Enhanced Test Coverage: Added new unit tests (TestTracedBlockProducer_RetrieveBatch_ErrorWithValue, TestTracedBlockProducer_CreateBlock_ErrorWithValue, TestTracedBlockProducer_ApplyBlock_ErrorWithValue) to explicitly verify that values are passed through the tracing decorator alongside errors, confirming the fix and preventing regressions.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[github-actions]

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Jan 22, 2026, 10:55 AM

[gemini-code-assist]

Code Review

This pull request addresses a critical bug in the tracing decorator where partially valid return values were being discarded in error scenarios, potentially causing panics. The fix correctly propagates these values along with the error, which is standard practice in Go. The changes in block/internal/executing/tracing.go are accurate and consistent across RetrieveBatch, CreateBlock, and ApplyBlock. I appreciate the addition of new tests in block/internal/executing/tracing_test.go that specifically cover these error-with-value scenarios, ensuring the fix is robust. I have one minor suggestion to improve the assertions in one of the new tests for better clarity and future-proofing.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.96%. Comparing base (555a48d) to head (301561b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3004      +/-   ##
==========================================
+ Coverage   57.89%   57.96%   +0.06%     
==========================================
  Files         110      110              
  Lines       10526    10526              
==========================================
+ Hits         6094     6101       +7     
+ Misses       3781     3774       -7     
  Partials      651      651              

Flag	Coverage Δ
combined	57.96% <100.00%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.