Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Access] Store Transaction Result error messages in db #6468

Conversation

UlyanaAndrukhiv
Copy link
Contributor

@UlyanaAndrukhiv UlyanaAndrukhiv commented Sep 17, 2024

Closes: #6302

Context

This PR implements the following changes:

  • New cli flag:
    A new cli flag--store-tx-result-error-messages has been added, which defaults to off. When enabled, transaction error messages are stored in the protocol database.

  • New Storage Object:
    Introduced a new storage object for transaction error messages in the protocol database (Badger).
    Added methods Store, ByBlockID, ByBlockIDTransactionID,ByBlockIDTransactionIndex, and Existsto handle storage and retrieval.

  • Data Structure:
    For each failed transaction, the following fields are stored: TransactionID, ErrorMessage, Index and ExecutorID (the node ID of the execution node from which the message was received).

  • Fetching:
    Error messages are fetched from ENs in parallel during the ingestion process, using the fixed and preferred EN lists. In case no EN returns a valid response, the fetching process does not block or fail.

  • API changes:
    The API now uses its local db first if available before requesting from execution nodes stored error messages.

  • Testing:
    Unit and integration tests are added to cover the new functionality, ensuring error messages are stored correctly and API behavior is as expected when the flag is enabled.

@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2024

Codecov Report

Attention: Patch coverage is 46.35678% with 427 lines in your changes missing coverage. Please review.

Project coverage is 41.14%. Comparing base (1d55978) to head (94ccde2).

Files with missing lines Patch % Lines
engine/common/rpc/utils.go 0.00% 157 Missing ⚠️
storage/mock/transaction_result_error_messages.go 0.00% 105 Missing ⚠️
cmd/access/node_builder/access_node_builder.go 0.00% 61 Missing ⚠️
...torage/badger/transaction_result_error_messages.go 77.44% 20 Missing and 10 partials ⚠️
storage/badger/operation/transaction_results.go 0.00% 25 Missing ⚠️
engine/access/ingestion/engine.go 33.33% 23 Missing and 1 partial ⚠️
...tion/tx_error_messages/tx_error_messages_engine.go 79.51% 14 Missing and 3 partials ⚠️
...estion/tx_error_messages/tx_error_messages_core.go 89.23% 5 Missing and 2 partials ⚠️
engine/access/rpc/backend/backend_transactions.go 99.08% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6468      +/-   ##
==========================================
- Coverage   41.16%   41.14%   -0.02%     
==========================================
  Files        2031     2036       +5     
  Lines      179463   179997     +534     
==========================================
+ Hits        73869    74055     +186     
- Misses      99381    99727     +346     
- Partials     6213     6215       +2     
Flag Coverage Δ
unittests 41.14% <46.35%> (-0.02%) ⬇️

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.

@Guitarheroua
Copy link
Collaborator

The first round for now!

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't finished reviewing all of your recent changes, but here's some early feedback

engine/access/rpc/backend/backend_transactions.go Outdated Show resolved Hide resolved
model/flow/transaction_result.go Show resolved Hide resolved
storage/transaction_results.go Outdated Show resolved Hide resolved
cmd/access/node_builder/access_node_builder.go Outdated Show resolved Hide resolved
engine/access/ingestion/engine.go Outdated Show resolved Hide resolved
engine/access/ingestion/engine.go Outdated Show resolved Hide resolved
engine/access/ingestion/engine.go Outdated Show resolved Hide resolved
engine/access/ingestion/engine.go Outdated Show resolved Hide resolved
engine/access/ingestion/engine.go Outdated Show resolved Hide resolved
engine/access/ingestion/engine.go Outdated Show resolved Hide resolved
storage/transaction_results.go Show resolved Hide resolved
engine/access/ingestion/engine.go Outdated Show resolved Hide resolved
engine/access/ingestion/engine.go Outdated Show resolved Hide resolved
engine/access/ingestion/engine.go Outdated Show resolved Hide resolved
engine/access/ingestion/engine.go Outdated Show resolved Hide resolved
@peterargue
Copy link
Contributor

hey @zhangchiqing can you take a look at this PR? particularly the new DB logic

Copy link
Member

@zhangchiqing zhangchiqing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Just have one issue. Please see comment.

engine/access/ingestion/engine.go Outdated Show resolved Hide resolved
Copy link
Member

@zhangchiqing zhangchiqing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Guitarheroua Guitarheroua added this pull request to the merge queue Oct 23, 2024
Merged via the queue into onflow:master with commit b80055f Oct 23, 2024
55 checks passed
@Guitarheroua Guitarheroua deleted the UlyanaAndrukhiv/6302-store-transaction-result-error-messages branch October 23, 2024 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Access] Store Transaction Result error messages in db
5 participants