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

Embedding transaction status code in receipts #658

Merged
merged 6 commits into from Nov 30, 2017

Conversation

@Arachnid
Copy link
Collaborator

@Arachnid Arachnid commented Jun 30, 2017

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.

@tjayrush
Copy link

@tjayrush tjayrush commented Jun 30, 2017

May I ask, in option 2, why you use "GetReturnData and ReturnData, permitting nodes to fetch the status and return data" as opposed to say GetStatus and GetReturnData which would match the names of items presumably being retrieved?

@karalabe
Copy link
Member

@karalabe karalabe commented Jun 30, 2017

Given that everywhere the 0 status code means success, should we really invert it here? If you want to stick to it, then perhaps lets call it "Success" and not "Status"?

## 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.

This comment has been minimized.

@karalabe

karalabe Jun 30, 2017
Member

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.

This comment has been minimized.

@karalabe

karalabe Jun 30, 2017
Member

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.

This comment has been minimized.

@Arachnid

Arachnid Jun 30, 2017
Author Collaborator

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.

@cdetrio
Copy link
Member

@cdetrio cdetrio commented Jun 30, 2017

Full nodes can provide RPCs to get a transaction return status and value by replaying the transaction, but fast nodes can only do this for nodes after their pivot point, and light nodes cannot do this at all, making a non-consensus solution impractical.

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.

@Arachnid
Copy link
Collaborator Author

@Arachnid Arachnid commented Jun 30, 2017

@tjayrush

May I ask, in option 2, why you use "GetReturnData and ReturnData, permitting nodes to fetch the status and return data" as opposed to say GetStatus and GetReturnData which would match the names of items presumably being retrieved?

Typo, thanks.

@karalabe

Given that everywhere the 0 status code means success, should we really invert it here? If you want to stick to it, then perhaps lets call it "Success" and not "Status"?

The CALL opcode returns 0 on failure and 1 on success.

@cdetrio
Copy link
Member

@cdetrio cdetrio commented Jul 14, 2017

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 block.coinbase would be a success, and only those transactions that revert the gas payments would be failures.

@Arachnid
Copy link
Collaborator Author

@Arachnid Arachnid commented Jul 14, 2017

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).

pirapira added a commit to pirapira/yellowpaper that referenced this pull request Jul 17, 2017
Now transaction receipts contain one byte indicating success or failure.
@axic
Copy link
Member

@axic axic commented Jul 28, 2017

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 eth_transactionReceipt RPC call do?

If the latter, wouldn't it make sense including a third option to signal revert as mentioned in #206 (comment)?

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.

This comment has been minimized.

@gumb0

gumb0 Jul 28, 2017
Member

"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.

@cdetrio
Copy link
Member

@cdetrio cdetrio commented Jul 28, 2017

Since this will encompass #98, how about Replacing intermediate state roots with return data in transaction receipts for a title?

@MicahZoltu
Copy link
Collaborator

@MicahZoltu MicahZoltu commented Jul 29, 2017

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?

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.

This comment has been minimized.

@pirapira

pirapira Aug 2, 2017
Member

Now the activation should be BYZANTIUM_FORK_BLKNUM.

This comment has been minimized.

@holgerd77

holgerd77 Aug 29, 2017

@pirapira Are there actually any tests for this EIP? If so, can you mention them here? Thanks!

This comment has been minimized.

@pirapira

pirapira Aug 29, 2017
Member

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

@pirapira pirapira mentioned this pull request Aug 2, 2017
12 of 12 tasks complete
@holgerd77
Copy link

@holgerd77 holgerd77 commented Aug 2, 2017

Can someone add example cases for the different outcomes with concrete exemplary values to the specification?

@rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Aug 18, 2017

@Arachnid
To sum up, from METROPOLIS_FORK_BLKNUM later, the PostState field in receipt will been replaced by a field called Status, which indicates the transaction execution result.
The Status field is a single byte, 0x01 represents successfully execution and 0x00 represent s a failure occured(No matter accidental error or a Revert opcode called).
And the field will be added into the consensus comparison. Right?

@holgerd77
Copy link

@holgerd77 holgerd77 commented Aug 29, 2017

@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 Byzantium?

@Arachnid Arachnid deleted the Arachnid:returndata branch Nov 30, 2017
pirapira added a commit to pirapira/yellowpaper that referenced this pull request Jan 19, 2018
Now transaction receipts contain one byte indicating success or failure.
pirapira added a commit to pirapira/yellowpaper that referenced this pull request Jan 19, 2018
Now transaction receipts contain one byte indicating success or failure.
pirapira added a commit to pirapira/yellowpaper that referenced this pull request Jan 19, 2018
Now transaction receipts contain one byte indicating success or failure.
@ivica7
Copy link

@ivica7 ivica7 commented Mar 2, 2018

After consultation with others, I dropped it because of concerns about DoS and spam opportunities; return data isn't charged for (except for memory expansion) but if it's part of consensus, would have to be stored indefinitely with receipts. It's still a possibility to add this to later forks, but I didn't want to rush in something that wasn't fully thought out.

@Arachnid DoS and Spam opportunities are a problem, but IMHO a good compromise would have been to allow to return an arbitrary 8-bit return code with the meaning 0=success, everything else an error. With 0/1 limitation we're only slightly better compared to earlier where we checked gasSent==gasUsed. On the UI side the message that can be presented to the user is only: "Everything is good" or "Something is wrong" -> bad user experience. I hope one day the specification for this will be extended.

sabondano added a commit to blockscout/blockscout that referenced this pull request Sep 19, 2018
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.
sabondano added a commit to blockscout/blockscout that referenced this pull request Sep 19, 2018
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.
sabondano added a commit to blockscout/blockscout that referenced this pull request Sep 19, 2018
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.
sabondano added a commit to blockscout/blockscout that referenced this pull request Sep 19, 2018
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.
dimroc added a commit to smartcontractkit/chainlink that referenced this pull request May 2, 2019
dimroc added a commit to smartcontractkit/chainlink that referenced this pull request May 3, 2019
se3000 added a commit to smartcontractkit/chainlink that referenced this pull request May 6, 2019
@holgerd77 holgerd77 mentioned this pull request Jun 25, 2020
15 of 15 tasks complete
@apur95
apur95 approved these changes Sep 12, 2020
Copy link

@phuttipon phuttipon left a comment

Duplicate of #

Copy link

@jokerksa12 jokerksa12 left a comment

commented on this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet