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

Incorrect status returned by GetTransactionStatus #2167

Open
DrZoltanFazekas opened this issue Jan 17, 2025 · 8 comments · May be fixed by #2170
Open

Incorrect status returned by GetTransactionStatus #2167

DrZoltanFazekas opened this issue Jan 17, 2025 · 8 comments · May be fixed by #2170
Assignees

Comments

@DrZoltanFazekas
Copy link
Contributor

The status should be 3 according to https://dev.zilliqa.com/zilliqa2/api/apis/api/zilliqa/GetTransactionStatus/ but the value returned is 1 e.g.:

cast rpc GetTransactionStatus 0x7edd27517945f2dbe5725593d85694e6840e42b8959ff6d99ef471c34ec26110 --rpc-url https://api.zq2-prototestnet.zilliqa.com

{"ID":"7edd27517945f2dbe5725593d85694e6840e42b8959ff6d99ef471c34ec26110","_id":null,"amount":"50000000000000","data":"","epochInserted":"10290041","epochUpdated":"10290041","gasLimit":"1200","gasPrice":"2000000016","lastModified":"1737059340905672","modificationState":2,"status":1,"nonce":"49","senderAddr":"0x036beea18d938a51beea396095f78f96f6a8a95d2e8043d7f82ae9576e4792a6af","signature":"0x3fb5586ebfcc9b36c99bb2110494c9919aefa5bcb9b9bcd1f540206d67ddcc9e524e36c0ad19f64643154ff33ec0526079dce8906fa141b99694e3089e1b0cd0","success":true,"toAddr":"0xea207bde83659a939727365282d784075d259f61","version":"21954561"}
@maxconway
Copy link
Contributor

maxconway commented Jan 17, 2025

So at the moment transaction statuses are based off the fields accepted and success in the receipt, with the following logic:

 let status_code =
            if receipt.accepted.is_some() && receipt.accepted.unwrap() && receipt.success {
                TxnStatusCode::Confirmed // 3
            } else if receipt.success {
                TxnStatusCode::Dispatched // 1
            } else {
                let errors: Vec<ScillaError> =
                    receipt.errors.into_iter().flat_map(|(_k, v)| v).collect();
                if errors.len() == 1 {
                    match errors[0] {
                        ScillaError::CallContractFailed => TxnStatusCode::FailScillaLib,
                        ScillaError::CreateContractFailed => TxnStatusCode::Error,
                        ScillaError::GasNotSufficient => TxnStatusCode::InsufficientGas,
                        ScillaError::BalanceTransferFailed => TxnStatusCode::InsufficientBalance,
                        _ => TxnStatusCode::Error,
                    }
                } else {
                    TxnStatusCode::Error
                }
            };

So in this case, if a status of 1 is being returned, this implies that either accepted==None or accepted==Some(false).
So I guess the answer is to just ignore the accepted field and return 3 if receipt.success or an error otherwise?
Does that sound like the correct behaviour @DrZoltanFazekas?

@DrZoltanFazekas
Copy link
Contributor Author

DrZoltanFazekas commented Jan 17, 2025

IMO, TxnStatusCode::Confirmed i.e. status code 3 is the final state meaning that the transaction got included into a block (which is instantly final on ZQ1). I don't think receipt.accepted represents this state, does it?

Based on the description in the docs, status code 2 is the same as pending in the EVM world (transaction is in the mempool and can be included in the next block) and status code 4 the same as queued (transaction is in the mempool but can't be included in the next block because of a nonce gap).

I'm not sure if TxnStatusCode::Dispatched is implemented correctly either. AFAIK receipt.success means the same as status = 0x1 in the EVM transaction receipt i.e. the transaction did not revert. Is that the right condition for the status code 1?

I tried to look up the semantics of these fields and codes in the old Zilliqa 1 dev docs, but it was not as helpful as I expected: https://dev.zilliqa.com/zilliqa1/developers/transaction-lifecycle/dev-txn-receipt/

@maxconway maxconway linked a pull request Jan 17, 2025 that will close this issue
@maxconway maxconway linked a pull request Jan 17, 2025 that will close this issue
@maxconway
Copy link
Contributor

Sorry I put in the pull request before I saw your reply.
I don't know the intentions of the fields in the receipt. @JamesHinshelwood might, it looks like he added the accepted field 9 months ago, but the success field 2 years ago, so the meanings could definitely have drifted since then though.
I've had a look online to see how other people define this sort of things but I've seen documentation that says things like "status (integer): Either 1 (success) or 0 (failure)." So that doesn't shed much light.

If you take a look in PR #2170 I've dropped support for TxnStatusCode::Dispatched entirely, and I'm basically just reporting TxnStatusCode::Confirmed unless there's a specific reason not to. It sounds like this is a bit over-optimistic, and we should actually be adding code to specifically query whether the transaction's block is finalised, and reporting 2 before it is finalised and 3 after, unless there's any error?

@JamesHinshelwood
Copy link
Contributor

success in a Zilliqa receipt means "did the transaction revert?"
accepted means "did the 'to address' receive any ZIL as a result of this transaction?"

@maxconway
Copy link
Contributor

maxconway commented Jan 17, 2025

So that sounds like the previous implementation was correct? We should be returning Confirmed (3) if and only if accepted==Some(true)?

I've just noticed the discussion on Slack and that appears to support that?

@DrZoltanFazekas
Copy link
Contributor Author

Where is the discussion on Slack?

My understanding of success was correct i.e. it has nothing to do with status code 1 (Dispatched). A transaction that reverts can also be dispatched and even included in a block i.e. become confirmed.

Thanks for clarifying the meaning of accepted @JamesHinshelwood. If I understood correctly, it only stores whether the recipient of the transaction accepted any ZIL sent by the sender i.e. it has nothing to do with the status code 3 (Confirmed).

I'll send an invite to discuss how to handle these legacy status codes inherited from Zilliqa 1 @maxconway @JamesHinshelwood

@maxconway
Copy link
Contributor

Yep good idea re a discussion on this.
The Slack discussion I was referencing was the one with Mark Carrol - if I understand correctly he was asking about the receipt.status field in eth_getTransactionReceipt, which is just a renaming of the receipt.success we're discussing here

Just for reference, I've traced through where these fields in the receipt are used:

AFAICS, receipt.success is used in:

  • The GetTransactionStatus endpoint we're discussing here
  • Other APIs where the receipt is returned
  • The db where it's stored
  • consensus.rs where it's set based on apply_result.success()
  • computing the transaction receipt's hash
  • The converter, where its set based on transaction.receipt.success

So its semantics are only relevant in terms of being produced by the converter and apply_result.success() in consensus.rs and consumed by APIs.

receipt.accepted is used in exactly the same way - produced by the converter and consensus.rs and consumed by the APIs, with the addition of also being used to set the modification_state field in GetTransactionStatus, though much like this one I guessed a bit at what that should represent.

@saeed-zil saeed-zil removed their assignment Jan 20, 2025
@maxconway
Copy link
Contributor

New statuses will be:

| modificationState | status | Description                                                             |
| ------------------- | -------- | ----------------------------------------------------------------------- |
| 0                   | 1        | Pending in mempool or in a non-finalized block    |
| 2                   | 3        | in a finalized block    |
| 2                   | 255      | error                                            |

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 a pull request may close this issue.

4 participants