rpc: ignore refunds in RPC tx methods #10948
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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`
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 6, 2024
With the changes made in #10948, we may no longer see refund receipts in the result.
VanBarbascu
pushed a commit
that referenced
this pull request
Apr 8, 2024
(drums) Resolves #6834 This PR changes the default behaviour of `broadcast_tx_commit`, `send_tx`, `tx`, `EXPERIMENTAL_tx_status` RPC methods. We no longer wait for refund receipts by default. If you do need to wait for refund receipts, you need ask about it explicitly by using `TxExecutionStatus::Final` option (`"wait_until": "FINAL"` in the json request). [We had a discussion with @bowenwang1996 about the case described below, tldr: he thinks it's unreachable. I haven't reproduced it, but I still think it may be an issue. I'm investigating, leave it here for now] Ugly side-effect: Sometimes, the response for any of mentioned methods may look like ```json { "jsonrpc": "2.0", "result": { "final_execution_status": "EXECUTED_OPTIMISTIC", "status": "Started", ... } ``` The full response format may be found [here](https://docs.near.org/api/rpc/transactions) `status` field corresponds to [`FinalExecutionStatus` structure](https://github.com/near/nearcore/blob/master/core/primitives/src/views.rs#L1354-L1364): ```rust pub enum FinalExecutionStatus { /// The execution has not yet started. #[default] NotStarted, /// The execution has started and still going. Started, /// The execution has failed with the given error. Failure(TxExecutionError), /// The execution has succeeded and returned some value or an empty vec encoded in base64. SuccessValue(#[serde_as(as = "Base64")] Vec<u8>), } ``` In `status`, we store the result of all the execution process, which, strictly saying, is not finalised without refund receipts. So, in the example above, `final_execution_status` conflicts with `status`. We say "it's executed, but not really", which may be confusing for the users. Replacing `status` with some other value would be even a bigger problem. Current situation is a weird truth. Replaced value would be a lie, which is much worse. Ideally, I'd prefer to drop `status` field from the response, but it's impossible because of compatibility reasons.
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
(drums) Resolves #6834 This PR changes the default behaviour of `broadcast_tx_commit`, `send_tx`, `tx`, `EXPERIMENTAL_tx_status` RPC methods. We no longer wait for refund receipts by default. If you do need to wait for refund receipts, you need ask about it explicitly by using `TxExecutionStatus::Final` option (`"wait_until": "FINAL"` in the json request). [We had a discussion with @bowenwang1996 about the case described below, tldr: he thinks it's unreachable. I haven't reproduced it, but I still think it may be an issue. I'm investigating, leave it here for now] Ugly side-effect: Sometimes, the response for any of mentioned methods may look like ```json { "jsonrpc": "2.0", "result": { "final_execution_status": "EXECUTED_OPTIMISTIC", "status": "Started", ... } ``` The full response format may be found [here](https://docs.near.org/api/rpc/transactions) `status` field corresponds to [`FinalExecutionStatus` structure](https://github.com/near/nearcore/blob/master/core/primitives/src/views.rs#L1354-L1364): ```rust pub enum FinalExecutionStatus { /// The execution has not yet started. #[default] NotStarted, /// The execution has started and still going. Started, /// The execution has failed with the given error. Failure(TxExecutionError), /// The execution has succeeded and returned some value or an empty vec encoded in base64. SuccessValue(#[serde_as(as = "Base64")] Vec<u8>), } ``` In `status`, we store the result of all the execution process, which, strictly saying, is not finalised without refund receipts. So, in the example above, `final_execution_status` conflicts with `status`. We say "it's executed, but not really", which may be confusing for the users. Replacing `status` with some other value would be even a bigger problem. Current situation is a weird truth. Replaced value would be a lie, which is much worse. Ideally, I'd prefer to drop `status` field from the response, but it's impossible because of compatibility reasons.
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
This reverts commit ddcddba.
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(drums)
Resolves #6834
This PR changes the default behaviour of
broadcast_tx_commit,send_tx,tx,EXPERIMENTAL_tx_statusRPC methods. We no longer wait for refund receipts by default.If you do need to wait for refund receipts, you need ask about it explicitly by using
TxExecutionStatus::Finaloption ("wait_until": "FINAL"in the json request).[We had a discussion with @bowenwang1996 about the case described below, tldr: he thinks it's unreachable. I haven't reproduced it, but I still think it may be an issue. I'm investigating, leave it here for now]
Ugly side-effect:
Sometimes, the response for any of mentioned methods may look like
{ "jsonrpc": "2.0", "result": { "final_execution_status": "EXECUTED_OPTIMISTIC", "status": "Started", ... }The full response format may be found here
statusfield corresponds toFinalExecutionStatusstructure:In
status, we store the result of all the execution process, which, strictly saying, is not finalised without refund receipts.So, in the example above,
final_execution_statusconflicts withstatus. We say "it's executed, but not really", which may be confusing for the users.Replacing
statuswith some other value would be even a bigger problem. Current situation is a weird truth. Replaced value would be a lie, which is much worse.Ideally, I'd prefer to drop
statusfield from the response, but it's impossible because of compatibility reasons.