Embedding transaction status code in receipts #658
Conversation
|
May I ask, in option 2, why you use " |
|
Given that everywhere the |
EIPS/eip-draft-returndata.md
Outdated
| ## Specification | ||
| Option 1: For blocks where block.number >= METROPOLIS_FORK_BLKNUM, the intermediate state root is replaced by `status + return_data`, where `status` is the 1 byte status code, with 0 indicating failure (due to any operation that can cause the transaction or top-level call to revert) and 1 indicating success, and `return_data` is the data returned from the `RETURN` or `REVERT` opcode of the top-level call, and where `+` indicates concatenation. | ||
|
|
||
| Option 2: For blocks where block.number >= METROPOLIS_FORK_BLKNUM, the intermediate state root is replaced by `keccak256(status + return_data)`, where `status`, `return_data` and `+` have meanings as described in option 1. Additionally, new wire protocol messages are added for `GetReturnData` and `ReturnData`, permitting nodes to fetch the status and return data for transactions contained in specified blocks. |
There was a problem hiding this comment.
Option 2 won't work since the data will be available only on nodes that actually ran the transaction. So you're back to the original issue you set out to solve. Also if you only have the hash of the result, there's no way to do a "success, but here's a message" scenario. Not sure if relevant.
There was a problem hiding this comment.
I guess if you disallow "success + message", then we could use sha(success + <empty>) as the check in fast/light nodes to see if something went wrong without knowing what.
There was a problem hiding this comment.
Option 2 would work by adding a new data type for nodes to synchronise: return data. Fast synced clients would fetch this for all past receipts, and light nodes would fetch it on demand.
Can't light clients replay a tx by fetching the state branches (merkle proofs) for all accounts that the tx touches? Since within a block any prior tx may have modified an account, all prior tx's would also have to be replayed. But in principle, light clients are capable of replaying transactions. |
Typo, thanks.
The CALL opcode returns 0 on failure and 1 on success. |
|
If something like EIP 101 is eventually adopted, I'm wondering how it would interact with a definition of failure as "any operation that can cause the transaction or top-level call to revert." Under EIP 101, wouldn't the "top-level call" be the execution which pays the gas fee to the miner? Then every transaction that pays a gas fee to |
|
After out of band discussion with Vitalik and others, I've amended this proposal to simply insert a 1 byte return status (1 for success, 0 for failure). |
Now transaction receipts contain one byte indicating success or failure.
|
I am not fully familiar with this part of the protocol, but does this EIP only refer to a P2P message or does it also influence what the If the latter, wouldn't it make sense including a third option to signal revert as mentioned in #206 (comment)? |
EIPS/eip-draft-returndata.md
Outdated
| Instead, we propose to replace the intermediate state root, already obsoleted by EIP98, with either the return status (1 for success, 0 for failure) and any return/revert data, or the hash of the above. This both allows callers to determine success status, and remedies the previous omission of return data from the receipt. | ||
|
|
||
| ## Specification | ||
| For blocks where block.number >= METROPOLIS_FORK_BLKNUM, the intermediate state root is replaced by a status code, a single byte with 0 indicating failure (due to any operation that can cause the transaction or top-level call to revert) and 1 indicating success. |
There was a problem hiding this comment.
"a single byte with 0 or 1" is somewhat confusing because RLP representation of 0 (number) is empty byte array (i.e. zero bytes).
This should say either "number 0 or 1" or something like "array of size 1 with 0 or 1 byte"
cpp currently implemented it as a number, I think it's more natural.
|
Since this will encompass #98, how about |
|
The motivation section discusses including return/revert data, yet the specification section doesn't mention it. Is this specification incomplete? If so I recommend adding a TBD or something to make that more clear for the time being. If not, am I missing something about how return/revert data is provided for in the transaction receipt? |
EIPS/eip-draft-returndata.md
Outdated
| Instead, we propose to replace the intermediate state root, already obsoleted by EIP98, with either the return status (1 for success, 0 for failure) and any return/revert data, or the hash of the above. This both allows callers to determine success status, and remedies the previous omission of return data from the receipt. | ||
|
|
||
| ## Specification | ||
| For blocks where block.number >= METROPOLIS_FORK_BLKNUM, the intermediate state root is replaced by a status code, a single byte with 0 indicating failure (due to any operation that can cause the transaction or top-level call to revert) and 1 indicating success. |
There was a problem hiding this comment.
Now the activation should be BYZANTIUM_FORK_BLKNUM.
There was a problem hiding this comment.
@pirapira Are there actually any tests for this EIP? If so, can you mention them here? Thanks!
There was a problem hiding this comment.
For instance, this receipt trie should be calculated using the new receipt format https://github.com/ethereum/tests/blob/develop/BlockchainTests/bcStateTests/blockhashTests.json#L22
|
Can someone add example cases for the different outcomes with concrete exemplary values to the specification? |
|
@Arachnid |
|
@Arachnid This PR lacks very much a concrecte specification, are you still responsible for this and can update this since the EIP is actually getting into |
Why: * Issue #757 was created after we noticed a few constraint violations for the `error` column in the `transactions` table. The constraint violations came about as we attempted to update transactions with a successful status but some value for the error field. The constraint is violated in this scenario because we expect successful transactions to not have an error. [EIP 658](ethereum/EIPs#658), which apparently is also called [EIP 98](ethereum/EIPs#98 (comment)) , says that the status should be set to 1 (success) if the outermost code execution succeeded (internal transaction with index 0), or it should be set to 0 (failure) if the outermost code execution failed. We're currently deriving the status from the last internal transaction when in fact we should be deriving it from the first internal transaction. * Issue link: #757 This change addresses the need by: * Editing `Explorer.Chain.Import.update_transactions/2` to set the error for a transaction equal to the error, if any, of the internal transaction with index 0. * Editing `Explorer.Chain.Import.update_transactions/2` to set a transaction's status as successful if it's internal transaction at index 0 doesn't have an error.
Why: * Issue #757 was created after we noticed a few constraint violations for the `error` column in the `transactions` table. The constraint violations came about as we attempted to update transactions with a successful status but some value for the error field. The constraint is violated in this scenario because we expect successful transactions to not have an error. [EIP 658](ethereum/EIPs#658), which apparently is also called [EIP 98](ethereum/EIPs#98 (comment)) , says that the status should be set to 1 (success) if the outermost code execution succeeded (internal transaction with index 0), or it should be set to 0 (failure) if the outermost code execution failed. We're currently deriving the status from the last internal transaction when in fact we should be deriving it from the first internal transaction. * Issue link: #757 This change addresses the need by: * Editing `Explorer.Chain.Import.update_transactions/2` to set the error for a transaction equal to the error, if any, of the internal transaction with index 0. * Editing `Explorer.Chain.Import.update_transactions/2` to set a transaction's status as successful if it's internal transaction at index 0 doesn't have an error, and as failed if it does. This only applies to pre-Byzantium and Ethereum Classic, for which transaction status is not set from receipts.
Great news |
Success |
|
Other options? |
I prefer to use “Success” |
|
You are aware that you are discussing on a 6 year old PR that cannot change, and the decision is embedded in the Ethereum protocol, so the only way to change it is via an EIP? |
|
|
|
This EIP replaces the intermediate state root field of the receipt with either the contract return data and status, or a hash of that value.