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

core/types: add MarshalBinary, UnmarshalBinary for Receipt #22806

Merged

Conversation

i-norden
Copy link
Contributor

@i-norden i-norden commented May 3, 2021

These are analogous to the methods of the same name on the Transaction type. I think it is helpful to include methods for serializing/deserializing an individual Receipt to/from the expected consensus encoding whether it is a legacy or a 2718/2930 type receipt, since Encode/DecodeRLP no longer does this for 2718/2930 receipts. Also added to the comments to help clarify the subtle difference in output between these serialization methods.

@i-norden i-norden force-pushed the receipt_binary_marshall_unmarshall_methods branch from afa2589 to 1338110 Compare May 3, 2021 20:09
@fjl
Copy link
Contributor

fjl commented May 3, 2021

The idea is pretty nice. Not sure if I like the verbosity in comments, especially for Transaction.

@fjl fjl changed the title Receipt MarshalBinary and UnmarshalBinary methods core/types: add MarshalBinary, UnmarshalBinary for Receipt May 3, 2021
@i-norden
Copy link
Contributor Author

i-norden commented May 3, 2021

Would you prefer I remove the comment changes? It was not immediately clear why my receipt and transaction encodings (using EncodeRLP) were suddenly breaking from consensus. Having to wrap a Receipt into a Receipts type and use EncodeIndex seems clunky to me.

Some of confusion around the EncodeRLP stemmed from the wording of EIP 2718.

The EncodeRLP method for an EIP-2718 tx is outputting rlp(TransactionType || TransactionPayload).

This makes sense from the definition of Transaction in EIP-2718:

"Transaction is either TransactionType || TransactionPayload or LegacyTransaction"

So the RLP encoding of Transaction is either rlp(TransactionType || TransactionPayload) or rlp(LegacyTransaction).

But EIP-2718 also states

"LegacyTransaction is rlp([nonce, gasPrice, gasLimit, to, value, data, v, r, s])"

So strictly following the same logic that is being used for a 2718 Transaction would suggest the output of EncodeRLP for a legacy Transaction should now be rlp(rlp([nonce, gasPrice, gasLimit, to, value, data, v, r, s])).

@fjl
Copy link
Contributor

fjl commented May 3, 2021

@i-norden During the process of adding typed transactions, everyone got very confused about RLP, and there were very long pointless debates about the exact notation that would express the intent correctly. Looks like they got it wrong in the end.

In practice it is really simple: legacy transactions are encoded exactly the same they always were, as an RLP list.
In RLP context, non-legacy transactions are encoded as RLP byte arrays.

Having to wrap a Receipt into a Receipts type and use EncodeIndex seems clunky to me.

You shouldn't do this anyway, the documentation of DerivableList says: "This is internal, do not use these methods.".
I think your change to add MarshalBinary and the inverse is great, but let's not add the confusing notation in comments.
It is sufficient to say that MarshalBinary returns the consensus encoding of a receipt.

@i-norden i-norden force-pushed the receipt_binary_marshall_unmarshall_methods branch from 1338110 to df7ac83 Compare May 3, 2021 21:39
@i-norden
Copy link
Contributor Author

i-norden commented May 4, 2021

That makes sense @fjl, thanks! I'll look into opening a PR against the EIP doc to clarify things. I rolled back the verbose comments.

@fjl
Copy link
Contributor

fjl commented May 10, 2021

We can merge this after EIP-1559 is in.

@fjl fjl added this to the 1.10.4 milestone May 11, 2021
@fjl
Copy link
Contributor

fjl commented May 17, 2021

EIP-1559 (tx type 2) has landed, so this needs a rebase.

@i-norden i-norden force-pushed the receipt_binary_marshall_unmarshall_methods branch 2 times, most recently from 3eb017f to de87f34 Compare May 27, 2021 00:19
@i-norden
Copy link
Contributor Author

@fjl done, apologies for the delay!

@i-norden i-norden force-pushed the receipt_binary_marshall_unmarshall_methods branch from de87f34 to 0f5208b Compare June 7, 2021 17:48
@i-norden i-norden force-pushed the receipt_binary_marshall_unmarshall_methods branch from 0f5208b to bc52622 Compare June 16, 2021 22:12
@fjl fjl modified the milestones: 1.10.4, 1.10.5 Jun 17, 2021
@i-norden i-norden force-pushed the receipt_binary_marshall_unmarshall_methods branch 2 times, most recently from 5cd9c3d to 40d644c Compare June 21, 2021 23:19
@karalabe karalabe removed this from the 1.10.5 milestone Jul 14, 2021
@i-norden i-norden force-pushed the receipt_binary_marshall_unmarshall_methods branch from 1c4dc72 to 569222b Compare July 28, 2021 17:27
@karalabe karalabe modified the milestones: 1.10.7, 1.10.8 Aug 10, 2021
@fjl fjl modified the milestones: 1.10.8, 1.10.9 Aug 24, 2021
@fjl fjl modified the milestones: 1.10.9, 1.10.10 Sep 29, 2021
@fjl fjl self-assigned this Oct 10, 2021
@fjl
Copy link
Contributor

fjl commented Oct 10, 2021

Sorry again for the delay. I will get this merged next week.

@fjl fjl merged commit e4f570f into ethereum:master Oct 13, 2021
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Oct 13, 2021
quentinlesceller added a commit to blockcypher/go-ethereum that referenced this pull request Dec 6, 2021
quentinlesceller added a commit to blockcypher/go-ethereum that referenced this pull request Feb 17, 2022
quentinlesceller added a commit to blockcypher/go-ethereum that referenced this pull request Jun 16, 2022
quentinlesceller added a commit to blockcypher/go-ethereum that referenced this pull request Aug 17, 2022
quentinlesceller added a commit to blockcypher/go-ethereum that referenced this pull request Aug 23, 2022
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
quentinlesceller added a commit to blockcypher/go-ethereum that referenced this pull request Jan 25, 2023
quentinlesceller added a commit to blockcypher/go-ethereum that referenced this pull request Mar 22, 2023
roysc added a commit to cerc-io/plugeth-utils that referenced this pull request Jul 19, 2023
roysc added a commit to cerc-io/plugeth-utils that referenced this pull request Jul 19, 2023
quentinlesceller added a commit to blockcypher/go-ethereum that referenced this pull request Aug 16, 2023
@github-cerc-io github-cerc-io deleted the receipt_binary_marshall_unmarshall_methods branch October 2, 2023 20:57
quentinlesceller added a commit to blockcypher/go-ethereum that referenced this pull request Nov 22, 2023
matthieu pushed a commit to blockcypher/go-ethereum that referenced this pull request Feb 14, 2024
matthieu pushed a commit to blockcypher/go-ethereum that referenced this pull request Mar 11, 2024
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.

None yet

4 participants