Skip to content

feat: add returndata to tx receipts #542

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

charles-cooper
Copy link

right now, returndata is not part of the tx receipt. this means that consumers of the API need to find alternative ways to fetch the returndata from a transaction, the most common being debug_traceTransaction. however, this is non-standard and not universally supported. it would be helpful if the returndata were simply part of the transaction receipt.

right now, returndata is not part of the tx receipt. this means that
consumers of the API need to find alternative ways to fetch the
returndata from a transaction, the most common being
`debug_traceTransaction`. however, this is non-standard and not
universally supported. it would be helpful if the returndata were simply
part of the transaction receipt.
@@ -119,3 +119,7 @@ ReceiptInfo:
title: blob gas price
description: The actual value per gas deducted from the sender's account for blob gas. Only specified for blob transactions as defined by EIP-4844.
$ref: '#/components/schemas/uint'
returnData:
title: returndata
description: The returndata from the call. This is an optional field that the client is not required to return if the returndata is not available. If it is not available, it should return `nil`.
Copy link

Choose a reason for hiding this comment

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

the return data is usually not stored because it can retrieved by re-executing the tx again, hence it's only part of the trace responses.

if this is an option, then it's ambiguous what the client impl should do, always re-execute or always return null

but having a standard call for obtaining the returndata would be very useful, so maybe this should be a standalone function instead?

Copy link
Author

Choose a reason for hiding this comment

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

i left it optional exactly for this reason, clients probably do not have the returndata stored, so in the short-term they can keep doing exactly what they have been doing. it is preferred by consumers that the returndata is there, but if it is nil then they can fall back to workaround (as they currently do).

Copy link
Author

Choose a reason for hiding this comment

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

a standardized eth_getReturnData would also work for this purpose, although it seems inefficient since in the (very common) case that you want to inspect the returndata, you need to make two calls instead of just one.

Copy link
Author

Choose a reason for hiding this comment

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

just want to include some points offline - asking for returndata in getTxReceipt may increase latency and/or pricing for infra providers

Copy link
Author

Choose a reason for hiding this comment

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

you need to make two calls instead of just one.

by the way, having two calls is pretty problematic because there are race conditions when you have to call the node twice (there could be a reorg or the node provider could switch clients and the second call returns null or a different value). the ideal state of affairs is to get the result back in one call.

Copy link
Author

@charles-cooper charles-cooper May 23, 2024

Choose a reason for hiding this comment

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

another issue in practice is that RPC providers fail all the time, so a very common case is that you get the result of eth_getTransactionReceipt but then the node fails, and the following call to grab the trace (or alternatives like the above proposed eth_getReturnData) fails. it's not a showstopper, you can do a second retry loop or failover to another RPC. but it's more ideal -- just simpler and less prone to failure -- to get the returndata back in a single call.

@LukaszRozmej
Copy link

I prefer either:

  1. mandatory storage of this data and changing eth/XX protocol to be able to sync it OR
  2. separate method as it allows for better separation of optional and mandatory data and better balance load of the calls as re-runing transaction is more costly than reading from receipt database.

@charles-cooper
Copy link
Author

I prefer either:

  1. mandatory storage of this data and changing eth/XX protocol to be able to sync it OR

how feasible is it to change the receipt database?

@LukaszRozmej
Copy link

LukaszRozmej commented May 23, 2024

I prefer either:

  1. mandatory storage of this data and changing eth/XX protocol to be able to sync it OR

how feasible is it to change the receipt database?

way less than the alternative, it would have to be hardforked and wouldn't apply to previous receipts.
So am for adding new method like eth_call but for historical transactions only.

@s1na
Copy link
Contributor

s1na commented May 23, 2024

So am for adding new method like eth_call but for historical transactions only.

Perhaps eth_replayTransaction.

I would be for adding tx-boundary state to eth_call (e.g. eth_call({}, "latest", 2) where 2 is the tx index). The challenge is that geth has extra optional parameters to override state and block fields. So to make this tx index compatible across clients we'd first need to standardize those 2 fields.

Because of this eth_replayTransaction seems the path of least resistance.

@charles-cooper
Copy link
Author

So am for adding new method like eth_call but for historical transactions only.

Perhaps eth_replayTransaction.

what does eth_replayTransaction do? basically eth_call but with a transaction hash?

@s1na
Copy link
Contributor

s1na commented May 25, 2024

what does eth_replayTransaction do? basically eth_call but with a transaction hash?

Yep, that is the idea that Lukasz proposed. I just suggested a name :)

@fjl
Copy link
Contributor

fjl commented Dec 18, 2024

I don't think this should be added. In the Ethereum protocol, transactions exist in order to write to the state, and transactions do not have a returned value. The 'output' of a transaction are the logs, and they go into the receipt. Relying on transaction return data for any use case is just wrong.

@fvictorio
Copy link

@fjl one reason this is useful is for reverted transactions. If you want to get the revert reason, you need the return data of the transaction.

@alialobidm alialobidm mentioned this pull request Jan 3, 2025
@wmitsuda
Copy link

Hi, some colleague of mine is interested in similar feature and while discussing possible solutions he pointed me at this issue (as it was my preferred approach, but he was pointing me that there was some contention about the approach).

So, I'd like to give my $0.02:

  • I'm the author of ots_getTransactionError in Erigon, but I consider that solution as a "band-aid". If I were to invent Ethereum and create an API from scratch, I'd put it into receipts as it is the result of an included transaction execution.
  • I don't see the return data as more or less "execution result" than logs or any other value in the receipts.
  • The argument about requiring nodes to store it or not, or how expensive that call should be is debatable and very implementation-specific (will elaborate below).
  • I actually think the solution proposed here is quite good and it is the best of both worlds (will elaborate below).

Execution cost/result storage

I think it is a mistake to block such feature based on the assumption of disk requirements or execution costs.

For example, in Erigon 3 receipts are not stored, they are always calculated on the fly. Transaction-based execution granularity makes it cheap. Should this feature be blocked because other implementations need to execute the whole block to calculate the result? Or because they require the result to be stored on disk as part of receipts? I don't think so.

Instead, we should look for some loose approach to enable API evolution (see next section).

API evolution

The approach proposed in this PR by making the new field optional is quite good because:

  • it doesn't require client implementations that don't want/don't have that info to implement it, or want implement it at their own pace.
  • it doesn't block client implementations that want to implement it from doing it.
  • it clearly labels that feature as optional, so users that are interested in using it can look for client implementations that implement it, and at the same time defines a standard interface (i.e. users don't need to adapt their code to different implementations).

In fact I think that's a good approach that could be applied to any additions to existing interfaces. Otherwise execution APIs will be stuck forever to decisions made at the beginning and that doesn't necessarily represent users needs today.

@charles-cooper
Copy link
Author

charles-cooper commented Mar 24, 2025

Relying on transaction return data for any use case is just wrong.

@fjl that's just simply not true. relying on transaction return data is not only valid, but useful. i'm not really sure what to say here since there are in fact a lot of reasons / use cases for this, although i'm not sure this is the right place to enumerate them. if you're engaging in good faith, i can go through some reasons why. i'm just rather surprised that "we need a standardized way to get returndata" is somehow contentious among client devs.

in fact, maintainers from three different smart contract development frameworks have already chimed in on this thread alone (myself, @fvictorio and @mattsse) on the usefulness of this. people are even submitting EIPs to issue logs for the revert data (ethereum/EIPs#9390), since it's currently so difficult to get the returndata from a transaction.

@wmitsuda
Copy link

in fact, maintainers from three different smart contract development frameworks have already chimed in on this thread alone (myself, @fvictorio and @mattsse) on the usefulness of this. people are even submitting EIPs to issue logs for the revert data (ethereum/EIPs#9390), since it's currently so difficult to get the returndata from a transaction.

the synthetic logs approach in that EIP feels more like a workaround for the refusal of the changed proposed here rather than a natural solution for this problem.

I'd prefer this one being adopted than adding a synthetic log and introducing a corner case to contract execution.

@shohamc1
Copy link

Hi there, I'm one of the authors of the EIP for emitting logs with revert data. Not having a straightforward way to get the return data is an oversight which the community seems to have worked itself around so far, but this does not change the fact that this should be addressed - via logs or in receipts.

I am in support of this change, albeit by making it necessary. I would like to see consistent behaviour across implementations, as I believe it is not fair to users of the standard JSON-RPC to have to choose between multiple implementations to get their desired behaviour. Having this divergent behaviour also means it's harder to build tooling around this - "Is the return data really null or does the RPC not have it? What implementation is the RPC running?"

It is also naive to assume that users choose their client implementation - most developers use RPC providers, and this decision is made by RPC providers who might not run a specific implementation for a small subset of users.

Doing it via logs was an effort to have this data available without requiring a ecosystem wide upgrade - Erigon does not have to transition it's database for this change, but I think most (if not all) other implementations will. I am not really opinionated on doing it via logs or in receipt - I would just like to see this done sooner rather than later.

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.

8 participants