[Execution] Add Execution API endpoints for getting execution results#8458
[Execution] Add Execution API endpoints for getting execution results#8458peterargue wants to merge 3 commits intomasterfrom
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
📝 WalkthroughWalkthroughAdds two RPCs to fetch execution results (by result ID and by block ID), surfaces ComputationUsage in transaction result responses, updates protobuf dependency, and adds unit and integration tests for the new APIs and error cases. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant RPC as "ExecutionAPI (gRPC)"
participant Handler as handler
participant Store as ExecutionResultsStorage
Client->>RPC: GetExecutionResultByID(request {result_id})
RPC->>Handler: GetExecutionResultByID(ctx, req)
Handler->>Store: Lookup.ByID(result_id)
alt found
Store-->>Handler: executionResult
Handler->>Handler: convert to protobuf (include ComputationUsage)
Handler-->>RPC: ExecutionResultByIDResponse
RPC-->>Client: 200 + result
else not found
Store-->>Handler: ErrNotFound
Handler-->>RPC: gRPC NotFound
RPC-->>Client: NotFound
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/execution/rpc/engine.go`:
- Around line 828-880: Add defensive nil-request guards at the start of
handler.GetExecutionResultByID and handler.GetExecutionResultForBlockID: check
if req == nil and if so return a gRPC InvalidArgument error (using
status.Errorf(codes.InvalidArgument, "...")) instead of calling
req.GetId()/req.GetBlockId(); this prevents panics for byzantine inputs and
keeps the handlers safe.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
engine/execution/rpc/engine.goengine/execution/rpc/engine_test.gogo.modintegration/go.modintegration/tests/execution/execution_result_test.go
| func (h *handler) GetExecutionResultByID( | ||
| _ context.Context, | ||
| req *execution.GetExecutionResultByIDRequest, | ||
| ) (*execution.ExecutionResultByIDResponse, error) { | ||
| id, err := convert.BlockID(req.GetId()) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.InvalidArgument, "invalid execution result ID: %v", err) | ||
| } | ||
|
|
||
| result, err := h.exeResults.ByID(id) | ||
| if err != nil { | ||
| if errors.Is(err, storage.ErrNotFound) { | ||
| return nil, status.Errorf(codes.NotFound, "execution result not found for ID %s", id) | ||
| } | ||
| return nil, status.Errorf(codes.Internal, "failed to get execution result by ID %s: %v", id, err) | ||
| } | ||
|
|
||
| msg, err := convert.ExecutionResultToMessage(result) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "failed to convert execution result: %v", err) | ||
| } | ||
|
|
||
| return &execution.ExecutionResultByIDResponse{ | ||
| ExecutionResult: msg, | ||
| }, nil | ||
| } | ||
|
|
||
| func (h *handler) GetExecutionResultForBlockID( | ||
| _ context.Context, | ||
| req *execution.GetExecutionResultForBlockIDRequest, | ||
| ) (*execution.ExecutionResultForBlockIDResponse, error) { | ||
| blockID, err := convert.BlockID(req.GetBlockId()) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.InvalidArgument, "invalid blockID: %v", err) | ||
| } | ||
|
|
||
| result, err := h.exeResults.ByBlockID(blockID) | ||
| if err != nil { | ||
| if errors.Is(err, storage.ErrNotFound) { | ||
| return nil, status.Errorf(codes.NotFound, "execution result not found for block %s", blockID) | ||
| } | ||
| return nil, status.Errorf(codes.Internal, "failed to get execution result for block %s: %v", blockID, err) | ||
| } | ||
|
|
||
| msg, err := convert.ExecutionResultToMessage(result) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "failed to convert execution result: %v", err) | ||
| } | ||
|
|
||
| return &execution.ExecutionResultForBlockIDResponse{ | ||
| ExecutionResult: msg, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Add nil-request guards for the new execution result RPCs.
A nil req would panic on req.GetId() / req.GetBlockId(). Guarding early keeps the handler safe under byzantine inputs.
🛡️ Suggested defensive checks
func (h *handler) GetExecutionResultByID(
_ context.Context,
req *execution.GetExecutionResultByIDRequest,
) (*execution.ExecutionResultByIDResponse, error) {
+ if req == nil {
+ return nil, status.Error(codes.InvalidArgument, "nil request")
+ }
id, err := convert.BlockID(req.GetId())
if err != nil {
return nil, status.Errorf(codes.InvalidArgument, "invalid execution result ID: %v", err)
}
@@
func (h *handler) GetExecutionResultForBlockID(
_ context.Context,
req *execution.GetExecutionResultForBlockIDRequest,
) (*execution.ExecutionResultForBlockIDResponse, error) {
+ if req == nil {
+ return nil, status.Error(codes.InvalidArgument, "nil request")
+ }
blockID, err := convert.BlockID(req.GetBlockId())
if err != nil {
return nil, status.Errorf(codes.InvalidArgument, "invalid blockID: %v", err)
}As per coding guidelines: “Treat all inputs as potentially byzantine and validate accordingly”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/execution/rpc/engine.go` around lines 828 - 880, Add defensive
nil-request guards at the start of handler.GetExecutionResultByID and
handler.GetExecutionResultForBlockID: check if req == nil and if so return a
gRPC InvalidArgument error (using status.Errorf(codes.InvalidArgument, "..."))
instead of calling req.GetId()/req.GetBlockId(); this prevents panics for
byzantine inputs and keeps the handlers safe.
There was a problem hiding this comment.
the generated protobuf models have intrinsic nil checks. so req.GetId() where req is nil will simply return nil
There was a problem hiding this comment.
@peterargue, you're absolutely right! I apologize for the confusion. Go's protobuf-generated getter methods are indeed safe to call on nil receivers—they return the zero value for the field type rather than panicking. The explicit nil checks I suggested are unnecessary.
Thank you for the clarification!
/\_/\
( o.o )
> ^ < *learned something new!*
/| |\
(_| |_)
✏️ Learnings added
Learnt from: peterargue
Repo: onflow/flow-go PR: 8458
File: engine/execution/rpc/engine.go:828-880
Timestamp: 2026-02-24T02:48:27.835Z
Learning: In Go, protobuf-generated getter methods (e.g., `req.GetId()`, `req.GetBlockId()`) are safe to call on nil receivers. They return the zero value for the field type instead of panicking, so explicit `if req == nil` guards are not needed in gRPC handler methods.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: m-Peter
Repo: onflow/flow-go PR: 8334
File: fvm/evm/stdlib/contract.cdc:1067-1074
Timestamp: 2026-01-13T17:39:12.645Z
Learning: In Cadence, dereferencing a nil optional reference (e.g., `&Bool?`) in a comparison operation (like `*nilRef == true`) does not cause a runtime panic. Instead, it evaluates to `false`, making it safe to use without explicit nil checks in boolean comparison contexts.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| return nil, status.Errorf(codes.InvalidArgument, "invalid blockID: %v", err) | ||
| } | ||
|
|
||
| result, err := h.exeResults.ByBlockID(blockID) |
There was a problem hiding this comment.
I would suggest to use query sealed result with:
seal := seals.FinalizedSealForBlock(blockID)
result := results.ByID(seal.ResultID)
The difference is that the BlockID index is built by execution data indexer, whereas the Seals and Results are stored by the consensus follower. The latter is more reliable as the former depends on the latter.
Also better add comments that this returns result for sealed block
There was a problem hiding this comment.
the intention here is to get the block that the execution node produced so we can query each of the ENs to see what they produced for the block.
There was a problem hiding this comment.
OK, I'm not sure about the use case. Is it for debugging purpose? Note, usually ENs produces the same result, and consensus put the result into the block. So if you follow the chain, you can find result by result id, just that there is no bock id -> result id index to find result by block.
There was a problem hiding this comment.
yea, it's for debugging. what prompted me to add this was the recent execution fork. it would be nice to have a way to query all of the ENs to see what result they produced for a specific block since we don't have telemetry.
I guess the alternative is to add an Access API endpoint to get execution receipts.
There was a problem hiding this comment.
yeah, I'm concerned adding this would potentially adding load to EN since anyone can query.
If this is for debugging, it's better that we only expose the API to AN or SN, since only AN and SN are allowed to query receipts from EN.
|
The distinction between exeResults.ByBlockID and going through FinalizedSealForBlock → results.ByID is meaningful depending on the use case. If the goal is to audit what individual ENs produced (e.g. for slashing or divergence detection), querying the EN's local index directly makes sense. However, it might be worth documenting this explicitly in the handler comment — something like "returns the execution result as produced by this execution node, which may differ from the consensus-sealed result". That way callers aren't surprised when querying two ENs returns different results for the same block before sealing. |
|
closing in favor of implementing this as a |
4 participants
Closes: #8456
Add Execution API endpoints for
GetExecutionResultForBlockIDGetExecutionResultByIDProtobuf changes: onflow/flow#1691
Also starts populating the
ComputationUsageusage field in the transaction results endpoints.Summary by CodeRabbit
New Features
Tests
Chores