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

V1.10.2 statediff 0.0.20 #68

Merged
merged 12 commits into from
May 3, 2021
Merged

Conversation

i-norden
Copy link
Collaborator

@i-norden i-norden commented Apr 30, 2021

This took a lot longer than expected. This includes the fixes to support EIP-2718/EIP-2930 transactions.

  • Introduced unit test to catch the original error described in comment on Support EIP-2930 #64
  • Introduced fix for this issue
  • Introduce new tx_type field in the transaction_cids table
  • Introduce new access_list_element table which contains the access list elements which reference the transaction_cids tx they belong to by FK
  • Introduce metrics for access list elements
  • Introduce a ton of unit tests for IPLD decoding/encoding, these were lifted from go-ipld-eth but had to be updated extensively to work with the newer versions of all the eth and ipld dependencies and to accommodate some changes/fixes we had made with our IPLD code
  • Introduce new MarshalBinary and UnmarshalBinary methods for Receipt. This should probably be upstreamed, they ought to already exist as they are counterparts to analogous Transaction methods.

To add to that last point:

A lot of confusion was due to these methods not existing on Receipt and the methods we previously used, EncodeRLP/DecodeRLP, now being under documented and overloaded.

In particular, the EncodeRLP method for an EIP-2718 tx is outputting rlp(TransactionType || TransactionPayload). For EIP-2930 txs this is rlp(0x01 || rlp([chainId, nonce, gasPrice, gasLimit, to, value, data, access_list, yParity, senderR, senderS])).

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 be rlp(rlp([nonce, gasPrice, gasLimit, to, value, data, v, r, s])).

When I get time I may upstream a patch to the EIP doc to help clarify this as well, but I need to give it some more thought.

In this context it is more appropriate to define LegacyTransaction and LegacyReceipt in EIP-2718 as

"LegacyTransaction is [nonce, gasPrice, gasLimit, to, value, data, v, r, s]" and "LegacyReceipt is [status, cumulativeGasUsed, logsBloom, logs]"

Except then the assertion that "the transaction/receipt root in the block header MUST be the root hash of patriciaTrie(rlp(Index) => Transaction)" breaks down as for legacy txs it expects rlp([nonce, gasPrice, gasLimit, to, value, data, v, r, s]) and for 2718 it expects TransactionType || TransactionPayload, not rlp(TransactionType || TransactionPayload).

@AFDudley
Copy link

AFDudley commented May 1, 2021

Awesome work. Please make the appropriate upstream PRs. Thanks!

@i-norden i-norden force-pushed the v1.10.2-statediff-0.0.20 branch from 47106bc to ded4e12 Compare May 3, 2021 17:31
@i-norden i-norden force-pushed the v1.10.2-statediff-0.0.20 branch from ded4e12 to 4336bf6 Compare May 3, 2021 17:41
@i-norden i-norden force-pushed the v1.10.2-statediff-0.0.20 branch from 9609b7c to b13ce2a Compare May 3, 2021 18:11
@i-norden i-norden merged commit e3ff284 into v1.10.2-statediff May 3, 2021
@i-norden i-norden mentioned this pull request May 3, 2021
@i-norden i-norden deleted the v1.10.2-statediff-0.0.20 branch May 3, 2021 20:14
@i-norden i-norden restored the v1.10.2-statediff-0.0.20 branch May 3, 2021 20:18
@github-cerc-io github-cerc-io deleted the v1.10.2-statediff-0.0.20 branch October 2, 2023 20:57
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.

2 participants