[Access] SendAndSubscribeTransactionStatuses endpoint implementation for Access Streaming API #5310

Guitarheroua · 2024-01-29T16:55:06Z

Implementation of SendAndSubscribeTransactionStatuses endpoint for Access Streaming API.

Depends on: onflow/flow-emulator#613

FLIP: onflow/flips#229

…be-blocks' into guitarheroua/send-and-subscribe-transaction_statuses

…n_statuses

codecov-commenter · 2024-02-19T09:02:39Z

Codecov Report

Attention: Patch coverage is 72.45509% with 46 lines in your changes are missing coverage. Please review.

Project coverage is 56.79%. Comparing base (c0b292e) to head (6c092e1).

Files	Patch %	Lines
.../access/rpc/backend/backend_stream_transactions.go	67.70%	22 Missing and 9 partials ⚠️
engine/access/subscription/streamer.go	36.36%	4 Missing and 3 partials ⚠️
engine/common/rpc/convert/transactions.go	0.00%	6 Missing ⚠️
engine/access/rpc/backend/backend_transactions.go	87.50%	1 Missing ⚠️
...ss/rpc/backend/transactions_local_data_provider.go	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5310      +/-   ##
==========================================
+ Coverage   55.63%   56.79%   +1.16%     
==========================================
  Files        1036      800     -236     
  Lines      101233    81209   -20024     
==========================================
- Hits        56320    46124   -10196     
+ Misses      40590    31378    -9212     
+ Partials     4323     3707     -616

Flag	Coverage Δ
unittests	`56.79% <72.45%> (+1.16%)`	⬆️

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.

…f github.com:Guitarheroua/flow-go into guitarheroua/send-and-subscribe-transaction_statuses

…eroua/4753-local-index-tx-result

…be-blocks' into guitarheroua/send-and-subscribe-transaction_statuses

…n_statuses

…f github.com:Guitarheroua/flow-go into guitarheroua/send-and-subscribe-transaction_statuses

…eroua/send-and-subscribe-transaction_statuses

peterargue

I don't think the logic in backendSubscribeTransactions quite captures what's needed. I believe it will return pending until the tx is indexed, which happens after the block is sealed.

Instead of checking if we have the tx result in the local storage, check if we know which block is supposed to include the tx. this is how we do it in GetTransactionResult
https://github.com/onflow/flow-go/blob/master/engine/access/rpc/backend/backend_transactions.go#L670-L682

This will tell us that the tx is included in a finalized block. once you know the block, you can just track the block's status. After finalized, we'll wait for it to be executed. You can check that by checking for the executionResult. If one exists, the tx is executed. From executed, we just need to check if the sealed.

access/handler.go

engine/access/rpc/backend/backend_stream_transactions.go

access/handler.go

engine/access/rpc/backend/backend_stream_transactions.go

engine/access/rpc/backend/backend_stream_transactions_test.go

engine/access/rpc/backend/backend_stream_transactions.go

peterargue · 2024-03-18T18:50:50Z

engine/access/rpc/backend/backend_stream_transactions.go

+		}
+
+		if txInfo.lastTxStatus == flow.TransactionStatusSealed || txInfo.lastTxStatus == flow.TransactionStatusExpired {
+			return nil, fmt.Errorf("transaction final status %s was already reported %w", txInfo.lastTxStatus.String(), subscription.ErrResponseNotAvailableForBlock)


what about returning some form of "end of data" error here. Then the subscription can just close the connection without an error if that error is encountered?

This is the scenario I am talking about in: https://github.com/onflow/flow-go/pull/5310/files#r1530079851
This falls under consolidation of error handling for subscriptions. For this PR I would return nil, nil and leave a TODO to take care of it later with a proper termination logic. @peterargue thoughts?

the challenge with nil, nil is it's unclear how the subscription would differentiate when a block should be skipped and when the stream should be shutdown. returning "end of data" would make it explicit

engine/access/subscription/streamer.go

access/handler.go

engine/access/rpc/backend/backend_stream_transactions.go

durkmurder · 2024-03-19T10:24:28Z

engine/access/rpc/backend/backend_stream_transactions.go

+
+		// The same transaction status should not be reported, so return here with no response
+		if txInfo.lastTxStatus == txStatus {
+			return nil, fmt.Errorf("transaction status %s was already reported %w", txInfo.lastTxStatus.String(), subscription.ErrResponseNotAvailableForBlock)


not a huge fan of using sentinels for branching between happy case scenarios, I think this return return nil, nil and streamer should send nothing in case it sees a response like this.

durkmurder · 2024-03-19T10:35:18Z

engine/access/rpc/backend/transactions_local_data_provider.go

@@ -21,6 +21,9 @@ import (
 	"github.com/onflow/flow-go/storage"
 )

+// ErrTransactionNotInBlock represents an error indicating that the transaction is not found in the block.
+var ErrTransactionNotInBlock = errors.New("transaction not in block")


Why do we need this sentinel? To me it seems we can return storage.ErrNotFound instead of this.

I requested this. It's because the methods used to lookup a transaction in a block need to lookup each collection in the guarantee list, then check if the transaction exists in one of the collections. Collections are synced by the Access node after the block is finalized, so it's common for there to be collections that aren't available yet when doing this lookup for non-sealed blocks.

This sentinel makes it easy for the caller to differentiate between a block that has missing collections, and one where all collections were indexed, but the tx just wasn't included.

engine/access/subscription/streamer.go

durkmurder · 2024-03-19T10:37:58Z

engine/access/subscription/streamer.go

+				continue
+			}
+
+			if errors.Is(err, storage.ErrNotFound) ||


I think this is where we need to take a step back and consolidate error handling. An action for a future issue.

+1. In a separate PR, we should update all the other endpoints to use ErrBlockNotReady.

peterargue · 2024-03-20T22:15:34Z

access/handler.go

+	subCtx, cancel := context.WithCancel(stream.Context())
+	defer cancel()


since we're not using cancel anymore, can you remove it

Suggested change

subCtx, cancel := context.WithCancel(stream.Context())

defer cancel()

ctx := stream.Context()

peterargue · 2024-03-20T22:24:44Z

engine/access/rpc/backend/transactions_local_data_provider.go

@@ -21,6 +21,9 @@ import (
 	"github.com/onflow/flow-go/storage"
 )

+// ErrTransactionNotInBlock represents an error indicating that the transaction is not found in the block.
+var ErrTransactionNotInBlock = errors.New("transaction not in block")


I requested this. It's because the methods used to lookup a transaction in a block need to lookup each collection in the guarantee list, then check if the transaction exists in one of the collections. Collections are synced by the Access node after the block is finalized, so it's common for there to be collections that aren't available yet when doing this lookup for non-sealed blocks.

This sentinel makes it easy for the caller to differentiate between a block that has missing collections, and one where all collections were indexed, but the tx just wasn't included.

peterargue

added a couple small comments, but otherwise looks good.

peterargue · 2024-03-20T22:53:47Z

engine/access/subscription/streamer.go

+				continue
+			}
+
+			if errors.Is(err, storage.ErrNotFound) ||


+1. In a separate PR, we should update all the other endpoints to use ErrBlockNotReady.

peterargue · 2024-03-20T23:06:31Z

integration/tests/access/cohort1/access_api_test.go

+		s.Assert().Equal(txID, sdk.Identifier(resp.GetId()))
+
+		expectedCounter++
+		finalTxStatus = resp.Status


can you have it also check that it receives notifications for each of the statuses in the processes

This was checked, and it returns the next pattern "Pending"->"Finalized"->"Sealed". Should return all the statuses including "Executed", as agreed, this will be done in the next iteration, when this endpoint will be expanded with TX Result.

…n_statuses

access/handler.go

durkmurder

Ready to go

Co-authored-by: Yurii Oleksyshyn <[email protected]>

Guitarheroua added 5 commits January 15, 2024 11:24

Implemented transaction status streaming

b9c1537

Fixed backend and tests

56b9f5b

Fixed tests

117b881

Fixed comments

62c09c0

Merge remote-tracking branch 'UlyanaAndrukhiv/UlyanaAndrukhiv/subscri…

08e3de0

…be-blocks' into guitarheroua/send-and-subscribe-transaction_statuses

Guitarheroua changed the title ~~Guitarheroua/send and subscribe transaction statuses~~ [Access] SendAndSubscribeTransactionStatuses endpoint implementation for Access Streaming API Jan 29, 2024

Guitarheroua and others added 7 commits February 1, 2024 00:33

Fixed issue with multiple subscribtion.

87f04d5

Merge remote-tracking branch 'UlyanaAndrukhiv/UlyanaAndrukhiv/subscri…

b9bf00c

…be-blocks' into guitarheroua/send-and-subscribe-transaction_statuses

Merge remote-tracking branch 'UlyanaAndrukhiv/UlyanaAndrukhiv/subscri…

2594bf5

…be-blocks' into guitarheroua/send-and-subscribe-transaction_statuses

Merge remote-tracking branch 'UlyanaAndrukhiv/UlyanaAndrukhiv/subscri…

fd4bf03

…be-blocks' into guitarheroua/send-and-subscribe-transaction_statuses

Merge remote-tracking branch 'UlyanaAndrukhiv/UlyanaAndrukhiv/subscri…

51ef06a

…be-blocks' into guitarheroua/send-and-subscribe-transaction_statuses

Make changes for Chain State tracker

5ffeae7

Merge branch 'master' into guitarheroua/send-and-subscribe-transactio…

59817d4

…n_statuses

Guitarheroua marked this pull request as ready for review February 19, 2024 08:56

Guitarheroua requested review from peterargue and yhassanzadeh13 as code owners February 19, 2024 08:56

Guitarheroua and others added 11 commits February 19, 2024 11:20

Linted

9cc36df

Merge branch 'guitarheroua/send-and-subscribe-transaction_statuses' o…

c402ec2

…f github.com:Guitarheroua/flow-go into guitarheroua/send-and-subscribe-transaction_statuses

Merge branch 'master' of github.com:Guitarheroua/flow-go into guitarh…

78e2ad1

…eroua/4753-local-index-tx-result

Added godoc

a77f107

Merge remote-tracking branch 'UlyanaAndrukhiv/UlyanaAndrukhiv/subscri…

48db475

…be-blocks' into guitarheroua/send-and-subscribe-transaction_statuses

Added comments

8defed0

Fixed conflicts

087ba90

Merge branch 'master' into guitarheroua/send-and-subscribe-transactio…

ea30dd4

…n_statuses

Fixed subscribtion and tests.

6bbba07

Merge branch 'guitarheroua/send-and-subscribe-transaction_statuses' o…

1579d57

…f github.com:Guitarheroua/flow-go into guitarheroua/send-and-subscribe-transaction_statuses

Merge branch 'master' of github.com:Guitarheroua/flow-go into guitarh…

d625f4a

…eroua/send-and-subscribe-transaction_statuses

peterargue reviewed Mar 1, 2024

View reviewed changes

Fixed issue with finalized tx status

67b9ff2

peterargue reviewed Mar 15, 2024

View reviewed changes

engine/access/rpc/backend/backend_stream_transactions.go Outdated Show resolved Hide resolved

Fixed incorrect tx subscribtion behaviour

3679c76

Guitarheroua requested a review from peterargue March 18, 2024 15:47

peterargue reviewed Mar 18, 2024

View reviewed changes

durkmurder reviewed Mar 19, 2024

View reviewed changes

engine/access/subscription/streamer.go Outdated Show resolved Hide resolved

durkmurder reviewed Mar 19, 2024

View reviewed changes

Guitarheroua added 4 commits March 19, 2024 13:02

Fixed remarks

16c9318

Removed comments

a321f35

Fixed integration test

bcca0aa

Removed sentinel error for result not available

9d6484c

Guitarheroua mentioned this pull request Mar 19, 2024

[Access] Improve subscription Streamer functionality to graceful shutdown on demand #5561

Open

Added graceful close. Fixed integration test

471fa6c

Guitarheroua requested review from peterargue and durkmurder March 19, 2024 15:14

peterargue reviewed Mar 20, 2024

View reviewed changes

peterargue approved these changes Mar 20, 2024

View reviewed changes

Guitarheroua and others added 2 commits March 21, 2024 11:09

Change to ctx without cancel

81465f2

Merge branch 'master' into guitarheroua/send-and-subscribe-transactio…

a2eb6f7

…n_statuses

durkmurder reviewed Mar 21, 2024

View reviewed changes

access/handler.go Outdated Show resolved Hide resolved

durkmurder approved these changes Mar 21, 2024

View reviewed changes

Guitarheroua and others added 2 commits March 21, 2024 14:01

Update access/handler.go

6c092e1

Co-authored-by: Yurii Oleksyshyn <[email protected]>

Updated flow-emulator version

c48a862

peterargue enabled auto-merge March 21, 2024 14:22

Updated flow-emulator version

49a6c68

peterargue added this pull request to the merge queue Mar 21, 2024

Merged via the queue into onflow:master with commit 022f21f Mar 21, 2024
55 checks passed

Guitarheroua mentioned this pull request Mar 21, 2024

[Access] SendAndSubscribeTransactionStatuses should include the transaction result in the response. #5566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Access] SendAndSubscribeTransactionStatuses endpoint implementation for Access Streaming API #5310

[Access] SendAndSubscribeTransactionStatuses endpoint implementation for Access Streaming API #5310

Guitarheroua commented Jan 29, 2024 •

edited

Loading

codecov-commenter commented Feb 19, 2024 •

edited

Loading

peterargue left a comment

peterargue Mar 18, 2024 •

edited

Loading

durkmurder Mar 19, 2024

peterargue Mar 20, 2024 •

edited

Loading

durkmurder Mar 19, 2024

durkmurder Mar 19, 2024

peterargue Mar 20, 2024 •

edited

Loading

durkmurder Mar 19, 2024

peterargue Mar 20, 2024

peterargue Mar 20, 2024

peterargue Mar 20, 2024 •

edited

Loading

peterargue left a comment

peterargue Mar 20, 2024

peterargue Mar 20, 2024

Guitarheroua Mar 21, 2024

durkmurder left a comment

		subCtx, cancel := context.WithCancel(stream.Context())
		defer cancel()

	subCtx, cancel := context.WithCancel(stream.Context())
	defer cancel()
	ctx := stream.Context()

[Access] SendAndSubscribeTransactionStatuses endpoint implementation for Access Streaming API #5310

[Access] SendAndSubscribeTransactionStatuses endpoint implementation for Access Streaming API #5310

Conversation

Guitarheroua commented Jan 29, 2024 • edited Loading

codecov-commenter commented Feb 19, 2024 • edited Loading

Codecov Report

peterargue left a comment

Choose a reason for hiding this comment

peterargue Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterargue Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterargue Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterargue Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

peterargue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durkmurder left a comment

Choose a reason for hiding this comment

Guitarheroua commented Jan 29, 2024 •

edited

Loading

codecov-commenter commented Feb 19, 2024 •

edited

Loading

peterargue Mar 18, 2024 •

edited

Loading

peterargue Mar 20, 2024 •

edited

Loading

peterargue Mar 20, 2024 •

edited

Loading

peterargue Mar 20, 2024 •

edited

Loading