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] Simplify error response from execute_transaction_block of transaction_execution_api #12989

Merged
merged 9 commits into from
Aug 14, 2023

Conversation

wlmyng
Copy link
Contributor

@wlmyng wlmyng commented Jul 13, 2023

Description

Overall simplify execution-side errors on execute_transaction_block RPC method by informing the caller how to resolve in the error message and reducing the data we put into the data field.

  1. (modify) QuorumDriverError::InvalidUserSignature, ::ObjectsDoubleUsed, NonRecoverableTransactionError are now considered client errors
  2. (new) ::TimeoutBeforeFinality, as a transient error -32000
  3. (modify) For NonRecoverableTransactionError, populate 'data' field with vector of error strings. Previously, we dump Vec<(SuiError, StakeUnit, Vec)> which just becomes a huge object. Transient errors are filtered out, unless they are the only error in the data field, in which case we return a generic error string and ask the caller to just try again.
  4. (new) For ObjectsDoubleUsed error response, populate the data field with a map of transaction -> vec(object_ref)
  5. unit testing to verify conversion

Test Plan

How did you test the new or updated feature?


If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • protocol change
  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

All transaction execution errors from execute_transaction_block of client-fault now return a -32002 error code. If you encounter this error code, there is most likely an issue in your transaction inputs.
Previously, when executing a transaction failed on the RPC, you would receive a, "Transaction has non recoverable errors from at least 1/3 of validators" after the transaction failed to execute. You now receive an improved error message, "Transaction execution failed due to issues with transaction inputs, please review the errors and try again: {errors}", where {errors} is a string list of actionable errors. After you resolve the errors indicated, your transaction should succeed.

@vercel
Copy link

vercel bot commented Jul 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 10, 2023 6:13pm
7 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview Aug 10, 2023 6:13pm
explorer-storybook ⬜️ Ignored (Inspect) Aug 10, 2023 6:13pm
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Aug 10, 2023 6:13pm
mysten-ui ⬜️ Ignored (Inspect) Visit Preview Aug 10, 2023 6:13pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Aug 10, 2023 6:13pm
sui-wallet-kit ⬜️ Ignored (Inspect) Visit Preview Aug 10, 2023 6:13pm
wallet-adapter ⬜️ Ignored (Inspect) Aug 10, 2023 6:13pm

@wlmyng wlmyng changed the title Improve execution-side errors on RPC Improve error handling for execute_transaction_block method of transaction_execution_api on RPC Jul 13, 2023
@wlmyng wlmyng changed the title Improve error handling for execute_transaction_block method of transaction_execution_api on RPC [rpc] Simplify error response from execute_transaction_block of transaction_execution_api Jul 13, 2023
crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/metrics.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Looks great to me! I'll defer to @longbowlu as the expert in this part of the codebase to give the final stamp, but love the improvements on error readability overall, thanks for working on this @wlmyng !

crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/metrics.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@longbowlu longbowlu 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, thank you!

crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/error.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/metrics.rs Outdated Show resolved Hide resolved
@wlmyng wlmyng merged commit f786180 into main Aug 14, 2023
38 checks passed
@wlmyng wlmyng deleted the rpc-handle-exec-errors-on-main branch August 14, 2023 17:49
damirka pushed a commit that referenced this pull request Aug 23, 2023
…action_execution_api (#12989)

## Description 

Overall simplify execution-side errors on `execute_transaction_block`
RPC method by informing the caller how to resolve in the error message
and reducing the data we put into the `data` field.

1. (modify) QuorumDriverError::InvalidUserSignature,
::ObjectsDoubleUsed, NonRecoverableTransactionError are now considered
client errors
2. (new) ::TimeoutBeforeFinality, as a transient error -32000
4. (modify) For NonRecoverableTransactionError, populate 'data' field
with vector of error strings. Previously, we dump Vec<(SuiError,
StakeUnit, Vec<ConciseAuthorityPublicKeyBytes>)> which just becomes a
huge object. Transient errors are filtered out, unless they are the only
error in the data field, in which case we return a generic error string
and ask the caller to just try again.
6. (new) For ObjectsDoubleUsed error response, populate the `data` field
with a map of transaction -> vec(object_ref)
7. unit testing to verify conversion

## Test Plan 

How did you test the new or updated feature?

---
If your changes are not user-facing and not a breaking change, you can
skip the following section. Otherwise, please indicate what changed, and
then add to the Release Notes section as highlighted during the release
process.

### Type of Change (Check all that apply)

- [ ] protocol change
- [x] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes
All transaction execution errors from execute_transaction_block of
client-fault will now return a -32002 error code. If you encounter this
error code, there is most likely something at fault in your transaction
inputs.
Previously, when executing a transaction fails on the rpc, you would
receive a, "Transaction has non recoverable errors from at least 1/3 of
validators" after failing to execute a transaction. You will now receive
an improved error message, "Transaction execution failed due to issues
with transaction inputs, please review the errors and try again:
{errors}". {errors} is a string list of actionable errors. Upon
remediation, you should be able to successfully retry your transaction.

---------

Co-authored-by: Lu Zhang <8418040+longbowlu@users.noreply.github.com>
ebmifa added a commit that referenced this pull request Aug 29, 2023
## Description 
Adding protocol change highlight to release notes script

## Test Plan 
```
eugene@mac-studio ~/code/sui (eugene/add_protocol_highlight_to_release_notes) $ ./scripts/generate-release-notes.sh releases/sui-v1.7.0-release releases/sui-v1.8.0-release
Sui Protocol Version in this release: XX


#13124:
Add protocol config feature flags for zkLogin to enable testing in Devnet, use updated proof verification logics for zkLogin signature verification.

#13417:

When building Move code, there are now additional linter warnings related to comparing collections from Sui framework code (`Bag`, `Table`, and `TableVec`). Note that this comparison is not a structural one based on the collection content, which is what one might expect, so Sui now indicates this via a linter warning.

#12989:
All transaction execution errors from `execute_transaction_block` of `client-fault` now return a -32002 error code. If you encounter this error code, there is most likely an issue in your transaction inputs.
Previously, when executing a transaction failed on the RPC, you would receive a, "Transaction has non recoverable errors from at least 1/3 of validators" after the transaction failed to execute. You now receive an improved error message, "Transaction execution failed due to issues with transaction inputs, please review the errors and try again: {errors}", where `{errors}` is a string list of actionable errors. After you resolve the errors indicated, your transaction should succeed.

#13194:

When building Move code, there are now additional linter warnings related to freezing an object containing (directly or indirectly) other (wrapped) object. Freezing such an object prevents unwrapping of inner objects.

#12575:

The details included in error messages returned during dependency graph construction might differ from the previous error messages, but they still include similar details and information.


#12933:
Error code designation is updated to support a more cohesive error reporting structure. Internal errors that arise while reading from authority return a `-32603` error code. Client-fault errors that arise while reading from authority return a `-32602` error code. Error strings are not modified.

#13312:

Removes the `--legacy-digest` flag from the `sui client upgrade` and `sui move build` CLI commands, as Sui networks no longer require package digests to be calculated using the legacy algorithm.
```
randall-Mysten pushed a commit that referenced this pull request Sep 6, 2023
## Description 
Adding protocol change highlight to release notes script

## Test Plan 
```
eugene@mac-studio ~/code/sui (eugene/add_protocol_highlight_to_release_notes) $ ./scripts/generate-release-notes.sh releases/sui-v1.7.0-release releases/sui-v1.8.0-release
Sui Protocol Version in this release: XX


#13124:
Add protocol config feature flags for zkLogin to enable testing in Devnet, use updated proof verification logics for zkLogin signature verification.

#13417:

When building Move code, there are now additional linter warnings related to comparing collections from Sui framework code (`Bag`, `Table`, and `TableVec`). Note that this comparison is not a structural one based on the collection content, which is what one might expect, so Sui now indicates this via a linter warning.

#12989:
All transaction execution errors from `execute_transaction_block` of `client-fault` now return a -32002 error code. If you encounter this error code, there is most likely an issue in your transaction inputs.
Previously, when executing a transaction failed on the RPC, you would receive a, "Transaction has non recoverable errors from at least 1/3 of validators" after the transaction failed to execute. You now receive an improved error message, "Transaction execution failed due to issues with transaction inputs, please review the errors and try again: {errors}", where `{errors}` is a string list of actionable errors. After you resolve the errors indicated, your transaction should succeed.

#13194:

When building Move code, there are now additional linter warnings related to freezing an object containing (directly or indirectly) other (wrapped) object. Freezing such an object prevents unwrapping of inner objects.

#12575:

The details included in error messages returned during dependency graph construction might differ from the previous error messages, but they still include similar details and information.


#12933:
Error code designation is updated to support a more cohesive error reporting structure. Internal errors that arise while reading from authority return a `-32603` error code. Client-fault errors that arise while reading from authority return a `-32602` error code. Error strings are not modified.

#13312:

Removes the `--legacy-digest` flag from the `sui client upgrade` and `sui move build` CLI commands, as Sui networks no longer require package digests to be calculated using the legacy algorithm.
```
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.

None yet

4 participants