[Access] Improvements for SubscribeTransactionStatuses statuses handling
#6736
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6736 /- ##
==========================================
- Coverage 41.12% 41.12% -0.01%
==========================================
Files 2107 2107
Lines 185330 185326 -4
==========================================
- Hits 76220 76217 -3
Misses 102710 102708 -2
- Partials 6400 6401 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Nope, just run tests. I will check it on the localnet. |
| @@ -36,7 36,7 @@ type backendSubscribeTransactions struct { | |||
| type TransactionSubscriptionMetadata struct { | |||
| *access.TransactionResult | |||
| txReferenceBlockID flow.Identifier | |||
| blockWithTx *flow.Header | |||
| blockWithTx *flow.Block | |||
There was a problem hiding this comment.
I have checked underlying code path and couldn't find a place why we essentially need a flow.Block. My suggestion here would be to stick to flow.Header and actually refactor usages of this field to accept flow.Header instead of flow.Block. I think that we should carry only the minimal needed information since extra info increases mental complexity and makes the APIs more convoluted and less intuitive.
Suppose I see a method signature:
GetTransactionResultFromExecutionNode(ctx context.Context,
block *flow.Block,
transactionID flow.Identifier,
requiredEventEncodingVersion entities.EventEncodingVersion)
when I see such signature I assume that it does something with the block payload since it passes a block rather than a header and the difference between block and header is exactly the payload, in fact it is misleading since it utilizes only fields of flow.Header. To avoid such misleading APIs I would propose to make a refactoring to clean this.
| func (b *backendSubscribeTransactions) searchForTransactionResult( | ||
| ctx context.Context, | ||
| txInfo *TransactionSubscriptionMetadata, | ||
| ) (*access.TransactionResult, error) { | ||
| _, err := b.executionResults.ByBlockID(txInfo.BlockID) | ||
| txResult, err := b.backendTransactions.GetTransactionResultFromStorage(ctx, txInfo.blockWithTx, txInfo.TransactionID, txInfo.eventEncodingVersion) | ||
| if err != nil { |
There was a problem hiding this comment.
at this point you can receive multiple sentinel errors and I assume an exception as well. We are basically sweeping all errors under one if statement but I would suggest to explicitly filter if it's an expected error. Otherwise we might get into a situation where a node observes a critical failure(lets say a storage failure where the badger DB is corrupted) but we are still trying to operate on best-effort scenario which is unacceptable.
| txInfo.CollectionID, | ||
| txInfo.eventEncodingVersion, | ||
| ) | ||
| // If any error occurs with local storage - request transaction result from EN |
There was a problem hiding this comment.
as I said in previously comment, "any" error is a bit to extreme
| // if either the execution node reported no results | ||
| if status.Code(err) == codes.NotFound { | ||
| // No result yet, indicate that it has not been executed | ||
| return nil, nil |
There was a problem hiding this comment.
This convention is not explained in the documentation, usually if there is no error then a result has to be available, returning nil, nil is undesirable, I am leaning to updating the documentation and returning a sentinel error to inform caller that there is no data available.
Closes #6574
Updated the logic for checking transaction results. The system now always checks local storage first, and if no transaction result is found, it attempts to retrieve it from the execution node.