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

RPC: improve supporting tx execution levels #10792

Merged
merged 2 commits into from
Apr 4, 2024

Conversation

telezhnaya
Copy link
Contributor

@telezhnaya telezhnaya commented Mar 14, 2024

More about tx execution levels: https://docs.near.org/api/rpc/transactions#tx-status-result

The levels that allow having not all the execution outcomes (Included, IncludedFinal), actually had to wait for all of the execution outcomes anyway.
[1.37.3 had a separate partial fix for supporting Included status, but let's not overcomplicate]
With the current change, we can show everything we have at the moment without extra waiting.
Also, current change helps us to be 1 step closer to resolving #6834
That would be my next step after we merge this PR.

TLDR: I partially copy pasted the code.
I could have rewritten chain:get_final_transaction_result instead, but it has LOTS of usages everywhere. I decided it's just not safe.
I anyway changed it a little with the new function chain:get_execution_status (hopefully in a better way) - see the details below in the comment.
But the new logic with partial execution outcomes list is used only in view_client (aka RPC, as far as I see from the usages).

@telezhnaya telezhnaya force-pushed the outcomes branch 2 times, most recently from afb6ad8 to 6050470 Compare April 2, 2024 22:43
@telezhnaya telezhnaya changed the title chain: return tx results even if not all the execution outcomes are ready RPC: improve supporting tx execution levels Apr 2, 2024
} else {
FinalExecutionStatus::Started
};
}
Copy link
Contributor Author

@telezhnaya telezhnaya Apr 2, 2024

Choose a reason for hiding this comment

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

I've decided to extract this logic from get_final_transaction_result to a separate function, and rewrite it, fixing a number of potential bugs in the meantime.

Some important context:
The receipt may produce tons of new receipts. However, the corresponding execution outcome will have the status ExecutionStatusView::SuccessReceiptId with only one id inside - it's the last receipt id of all the produced receipts. So if you want to see the status, you have to go through all the list, it's not enough to look at the status of the id inside SuccessReceiptId field.


Results

  • I started checking all the produced receipts statuses, not only the last one
  • I stopped relying on the order of the receipts
  • I fixed another bug. Before, we have
match &outcome_with_id.outcome.status {
                         ExecutionStatusView::Unknown if num_outcomes == 1 => {
                             Some(FinalExecutionStatus::NotStarted)
                         }
                         ...
}

In reality, if we have 1 outcome, it means the status is at least Started.

@telezhnaya telezhnaya force-pushed the outcomes branch 3 times, most recently from ca71739 to bf9a3e8 Compare April 3, 2024 00:20
@telezhnaya telezhnaya marked this pull request as ready for review April 3, 2024 00:44
@telezhnaya telezhnaya requested a review from a team as a code owner April 3, 2024 00:44
@telezhnaya telezhnaya requested a review from Longarithm April 3, 2024 00:44
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

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

Project coverage is 71.54%. Comparing base (cbf977a) to head (33174b4).

Files Patch % Lines
chain/chain/src/chain.rs 82.25% 8 Missing and 3 partials ⚠️
chain/client/src/view_client.rs 71.42% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #10792   +/-   ##
=======================================
  Coverage   71.54%   71.54%           
=======================================
  Files         758      758           
  Lines      152058   152086   +28     
  Branches   152058   152086   +28     
=======================================
+ Hits       108785   108812   +27     
- Misses      38767    38772    +5     
+ Partials     4506     4502    -4     
Flag Coverage Δ
backward-compatibility 0.24% <0.00%> (-0.01%) ⬇️
db-migration 0.24% <0.00%> (-0.01%) ⬇️
genesis-check 1.42% <0.00%> (-0.01%) ⬇️
integration-tests 37.17% <44.28%> (-0.01%) ⬇️
linux 70.00% <81.42%> (+<0.01%) ⬆️
linux-nightly 71.03% <81.42%> (+<0.01%) ⬆️
macos 54.53% <70.00%> (+0.01%) ⬆️
pytests 1.65% <0.00%> (-0.01%) ⬇️
sanity-checks 1.44% <0.00%> (-0.01%) ⬇️
unittests 67.17% <70.00%> (-0.01%) ⬇️
upgradability 0.29% <0.00%> (-0.01%) ⬇️

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.

Copy link
Member

@Longarithm Longarithm left a comment

Choose a reason for hiding this comment

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

Thank you! Please also look at the comments.

Ideally we should have a test for new behaviour. We likely have tests which check that getting recursive outcomes fails at some early block and succeeds later. We can add checks there that getting partial outcomes always succeeds and gives ids which indeed correspond to spawned receipts.

chain/client/src/view_client.rs Outdated Show resolved Hide resolved
chain/chain/src/chain.rs Outdated Show resolved Hide resolved
chain/chain/src/chain.rs Outdated Show resolved Hide resolved
integration-tests/src/user/runtime_user.rs Outdated Show resolved Hide resolved
@telezhnaya telezhnaya added this pull request to the merge queue Apr 4, 2024
Merged via the queue into near:master with commit 1d082db Apr 4, 2024
27 of 29 checks passed
@telezhnaya telezhnaya deleted the outcomes branch April 4, 2024 20:26
telezhnaya added a commit to telezhnaya/nearcore that referenced this pull request Apr 5, 2024
telezhnaya added a commit to telezhnaya/nearcore that referenced this pull request Apr 5, 2024
telezhnaya added a commit to telezhnaya/nearcore that referenced this pull request Apr 5, 2024
github-merge-queue bot pushed a commit that referenced this pull request Apr 5, 2024
This PR is based on #10948 because
it touches the same places
It's better to look at the separate commits

So first commit can be reviewed separately in
#10948

Second commit is a partial revert of
#10792
I just returned this piece back, no changes is made

https://github.com/near/nearcore/pull/10792/files#diff-ef9c6aaa80a330e446c5365f42be9bff37ba4f898cf519dadd7e17545783c77cL3099-L3126

Third commit is the meaningful part. We need to adjust the logic with
the old implementation.
1. We now treat incomplete list of the receipts as a valid state. It
means no `expect` usage, handle empty list of outcomes
2. We no longer can use `status` field for `finalExecutionStatus`
VanBarbascu pushed a commit that referenced this pull request Apr 8, 2024
More about tx execution levels:
https://docs.near.org/api/rpc/transactions#tx-status-result

The levels that allow having not all the execution outcomes (Included,
IncludedFinal), actually had to wait for all of the execution outcomes
anyway.
[1.37.3 had a separate partial fix for supporting Included status, but
let's not overcomplicate]
With the current change, we can show everything we have at the moment
without extra waiting.
Also, current change helps us to be 1 step closer to resolving
#6834
That would be my next step after we merge this PR.

TLDR: I partially copy pasted the code.
I could have rewritten `chain:get_final_transaction_result` instead, but
it has LOTS of usages everywhere. I decided it's just not safe.
I anyway changed it a little with the new function
`chain:get_execution_status` (hopefully in a better way) - see the
details below in the comment.
But the new logic with partial execution outcomes list is used only in
view_client (aka RPC, as far as I see from the usages).
VanBarbascu pushed a commit that referenced this pull request Apr 8, 2024
This PR is based on #10948 because
it touches the same places
It's better to look at the separate commits

So first commit can be reviewed separately in
#10948

Second commit is a partial revert of
#10792
I just returned this piece back, no changes is made

https://github.com/near/nearcore/pull/10792/files#diff-ef9c6aaa80a330e446c5365f42be9bff37ba4f898cf519dadd7e17545783c77cL3099-L3126

Third commit is the meaningful part. We need to adjust the logic with
the old implementation.
1. We now treat incomplete list of the receipts as a valid state. It
means no `expect` usage, handle empty list of outcomes
2. We no longer can use `status` field for `finalExecutionStatus`
VanBarbascu pushed a commit that referenced this pull request Apr 8, 2024
More about tx execution levels:
https://docs.near.org/api/rpc/transactions#tx-status-result

The levels that allow having not all the execution outcomes (Included,
IncludedFinal), actually had to wait for all of the execution outcomes
anyway.
[1.37.3 had a separate partial fix for supporting Included status, but
let's not overcomplicate]
With the current change, we can show everything we have at the moment
without extra waiting.
Also, current change helps us to be 1 step closer to resolving
#6834
That would be my next step after we merge this PR.

TLDR: I partially copy pasted the code.
I could have rewritten `chain:get_final_transaction_result` instead, but
it has LOTS of usages everywhere. I decided it's just not safe.
I anyway changed it a little with the new function
`chain:get_execution_status` (hopefully in a better way) - see the
details below in the comment.
But the new logic with partial execution outcomes list is used only in
view_client (aka RPC, as far as I see from the usages).
VanBarbascu pushed a commit that referenced this pull request Apr 8, 2024
This PR is based on #10948 because
it touches the same places
It's better to look at the separate commits

So first commit can be reviewed separately in
#10948

Second commit is a partial revert of
#10792
I just returned this piece back, no changes is made

https://github.com/near/nearcore/pull/10792/files#diff-ef9c6aaa80a330e446c5365f42be9bff37ba4f898cf519dadd7e17545783c77cL3099-L3126

Third commit is the meaningful part. We need to adjust the logic with
the old implementation.
1. We now treat incomplete list of the receipts as a valid state. It
means no `expect` usage, handle empty list of outcomes
2. We no longer can use `status` field for `finalExecutionStatus`
bowenwang1996 added a commit that referenced this pull request Apr 10, 2024
bowenwang1996 added a commit that referenced this pull request Apr 10, 2024
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.

2 participants