V1.10.2 statediff 0.0.20 #68

i-norden · 2021-04-30T23:48:07Z

i-norden commented

2021-04-30 23:48:07 +00:00

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

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 #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](https://eips.ethereum.org/EIPS/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 commented

2021-05-01 15:54:26 +00:00

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

Sign in to join this conversation.

No reviewers

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cerc-io/go-ethereum#68