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

[3.1] Update cleos to return an error code on failed trx processing #199

Merged
merged 6 commits into from
Sep 22, 2022

Conversation

heifner
Copy link
Member

@heifner heifner commented Sep 21, 2022

Leap 3.1 returns transaction failure traces by default. EOSIO/eos 2.0 would instead throw an exception.
The 3.1 release notes included the following:

New command line options for cleos:

  • --use-old-send-rpc
    • "Use old RPC /v1/chain/send_transaction, rather than new RPC /v1/chain/send_transaction2"
  • -t,--return-failure-trace defaults to true use -t false for previous behavior
    • "Return partial traces on failed transactions"
    • Note: Automation which depends on cleos to return a non-zero value on transaction failure will instead need to parse the traces or use option -t false.

This PR addresses the cleos non-zero return values for failed transaction execution. With this PR, cleos will parse the returned transaction traces and return a non-zero return value for transactions that failed to speculatively execute.

Before:

$ ./cleos  push action  eosio.token transfer '{"from":"eosio","to":"testera11111","quantity":"0.0001 SYS","memo":"hi"}'
failed transaction: d19173082af2e0dee7d11a638adabc646f55b81364b923b71c6d467fbff30b76  <unknown> bytes  <unknown> us
error 2022-09-21T15:20:49.283 cleos     main.cpp:601                  print_result         ] soft_except->to_detail_string(): 3040003 tx_no_auths: Transaction should have at least one required authority
transaction must have at least one authorization
    {}
    nodeos  transaction_context.cpp:753 validate_referenced_accounts

$ echo $?
0

After:

$ ./cleos  push action  eosio.token transfer '{"from":"eosio","to":"testera11111","quantity":"0.0001 SYS","memo":"hi"}'
failed transaction: 1488c901f8d1952eae4fddd509758a639cabf5f1ea86967bf6215580c264f519  <unknown> bytes  <unknown> us
error 2022-09-21T15:20:42.847 cleos.3.1 main.cpp:575                  print_result         ] soft_except->to_detail_string(): 3040003 tx_no_auths: Transaction should have at least one required authority
transaction must have at least one authorization
    {}
    nodeos  transaction_context.cpp:753 validate_referenced_accounts

$ echo $?
3

Resolves #139

@heifner heifner marked this pull request as ready for review September 21, 2022 20:44
int r = 0;
if (result.is_object() && result.get_object().contains("processed")) {
const auto& processed = result["processed"];
if( processed.get_object().contains( "except" ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to check if processed is an object before get_object()ing it? You do the is_object() check for result & except. And a failure here with processed not being an object would be outside of the try block internal to this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that would be better. I copy/pasted from print_result which does not have that. I'll add the check.

@@ -205,7 +205,8 @@ def checkDelayedOutput(popen, cmd, ignoreError=False):
Utils.checkOutputFileWrite(start, cmd, output, error)
if popen.returncode != 0 and not ignoreError:
Utils.Print("ERROR: %s" % error)
raise subprocess.CalledProcessError(returncode=popen.returncode, cmd=cmd, output=error)
# for now just include both stderr and stdout in output, python 3.5+ supports both a stdout and stderr attributes
Copy link
Member

Choose a reason for hiding this comment

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

what is the Python 3.5+ comment here getting at -- even Ubuntu 18 is using 3.6 so if there is a 3.5+ feature we should be able to use it

Copy link
Member Author

Choose a reason for hiding this comment

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

stderr was being placed in output. Python 3.5+ has both an output and a stderr. The better fix is to put stdout into output and stderr into stderr. However, output is used in 20+ places in many tests. I didn't want to make that big of a change in release/3.1. In main I plan to sperate out the stdout and stderr and update the 20+ usages of the CalledProcessError.

@heifner heifner requested a review from spoonincode September 22, 2022 03:52
if( except.is_object() ) {
try {
auto soft_except = except.as<std::optional<fc::exception>>();
if( soft_except ) {
Copy link
Member

Choose a reason for hiding this comment

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

At this point we've established that except is an object. Do we really need to treat except as an optional type still?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Removed optional.

@heifner heifner merged commit 508ff97 into release/3.1 Sep 22, 2022
@heifner heifner deleted the GH-139-cleos-return-codes-3.1 branch September 22, 2022 16:15
@heifner heifner added the OCI Work exclusive to OCI team label Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants