fix(rpc): update eth_tx_from_signed_eth_message to support legacy transactions #4523

hanabi1224 · 2024-07-12T15:02:24Z

Summary of changes

Changes introduced in this pull request:

update eth_tx_from_signed_eth_message to support legacy transaction types namely LEGACY_HOMESTEAD_TX and LEGACY_155_TX

     Running `target/quick/forest-tool api compare /home/me/fr/snapshots/calibnet/forest_snapshot_calibnet_2024-07-15_height_1789837.forest.car.zst --filter EthGetBlock --n-tipsets=100`
| RPC Method                                         | Forest | Lotus |
|----------------------------------------------------|--------|-------|
| Filecoin.EthGetBlockByHash (200)                   | Valid  | Valid |
| Filecoin.EthGetBlockByNumber (200)                 | Valid  | Valid |
| Filecoin.EthGetBlockTransactionCountByHash (100)   | Valid  | Valid |
| Filecoin.EthGetBlockTransactionCountByNumber (100) | Valid  | Valid |

Manually verified the below payload that was failing before

{"jsonrpc":"2.0","id":0,"method":"Filecoin.EthGetBlockByHash","params":
 ["0x3d885d56e4ecd2be098ddbb159ba2dfc6b6756680ffeed2441062fc677efea7a",true]
}

Reference issue to close (if applicable)

Closes #4519 #4518 #4477

Change checklist

I have performed a self-review of my own code,
I have made corresponding changes to the documentation,
I have added tests that prove my fix is effective or that my feature works (if possible),
I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

…nsactions

…gned_eth_message

LesnyRumcajs · 2024-07-15T07:10:16Z

src/rpc/methods/eth.rs

+    fn test_rlp_encoding_legacy() {
+        let tx = EthTx {
+            r#type: EIP_LEGACY_TX_TYPE.into(),
+            chain_id: 314159.into(),


Where is this test case coming from? Can we add a link to the filscan/filfox transaction?

LesnyRumcajs · 2024-07-15T07:16:39Z

src/rpc/methods/eth.rs

+    Ok((r.into(), s.into(), v.into()))
+}
+
+fn recover_legacy_homestead_sig(sig: &Signature) -> Result<(EthBigInt, EthBigInt, EthBigInt)> {


Let's avoid having hard logic in the rpc module. This should be used for schema cosmetics and re-using existing elements. I know it's not the case for now, but avoiding adding debt here will be helpful for a future refactor.

This looks pretty similar to

forest/src/eth/eip_155_transaction.rs

Line 43 in 07b0a87

pub(crate) fn with_signature(

- can we avoid logic duplication?

…gned_eth_message

LesnyRumcajs · 2024-07-16T09:36:52Z

src/rpc/methods/eth/eth_tx.rs

+        Ok(Hash(keccak(self.rlp_signed_message()?)))
+    }
+
+    pub fn rlp_signed_message(&self) -> Result<Vec<u8>> {


Those seems like nitty-gritty details of Ethereum transactions. Should this live in the eth module?

LesnyRumcajs · 2024-07-16T15:18:57Z

src/eth/eip_1559_transaction.rs

+        stream.append(&format_bigint(&self.s)?);
+
+        let mut rlp = stream.out().to_vec();
+        rlp.insert(0, 2);


Let's comment what is this.

LesnyRumcajs · 2024-07-16T15:19:28Z

src/eth/eip_1559_transaction.rs

@@ -61,6 +61,42 @@ impl EthEip1559TxArgs {

        Ok(self)
    }
+
+    pub fn rlp_signed_message(&self) -> anyhow::Result<Vec<u8>> {
+        let mut stream = rlp::RlpStream::new_list(12);


Is there a less scary API that doesn't require changing this number on a new field?

Switched to the unbounded API

LesnyRumcajs · 2024-07-16T15:20:53Z

src/rpc/methods/eth/eth_tx.rs

+/// Ethereum Improvement Proposals 1559 transaction type. This EIP changed Ethereum’s fee market mechanism.
+/// Transaction type can have 3 distinct values:
+/// - 0 for legacy transactions
+/// - 1 for transactions introduced in EIP-2930
+/// - 2 for transactions introduced in EIP-1559
+pub const EIP_LEGACY_TX_TYPE: u64 = 0;
+pub const EIP_1559_TX_TYPE: u64 = 2;
+
+pub const ETH_LEGACY_HOMESTEAD_TX_CHAIN_ID: u64 = 0;


Let's have these constants in the eth module.

LesnyRumcajs · 2024-07-16T15:23:21Z

src/eth/eip_155_transaction.rs


        self.r = r;
        self.s = s;
        self.v = v;

        Ok(self)
    }
+
+    pub fn rlp_signed_message(&self) -> anyhow::Result<Vec<u8>> {


Can we have some tests for this (and other transaction types)? Can be in a different PR. The magic number makes it scary, so I'd like to ensure this doesn't brake if someone adds a new field.

This has been covered by fn test_eth_hash_*

I'd find it safer to have lower-level tests here, even if it means some duplication, but it's not a huge deal.

…gned_eth_message

LesnyRumcajs · 2024-07-17T07:21:44Z

src/eth/eip_155_transaction.rs


        self.r = r;
        self.s = s;
        self.v = v;

        Ok(self)
    }
+
+    pub fn rlp_signed_message(&self) -> anyhow::Result<Vec<u8>> {


I'd find it safer to have lower-level tests here, even if it means some duplication, but it's not a huge deal.

hanabi1224 added 6 commits July 12, 2024 21:05

fix(rpc): update eth_tx_from_signed_eth_message to support legacy tra…

169cf44

…nsactions

Merge remote-tracking branch 'origin/main' into hm/fix-eth_tx_from_si…

e25ea81

…gned_eth_message

fix hash fn

93b7d97

Merge remote-tracking branch 'origin/main' into hm/fix-eth_tx_from_si…

f009c9d

…gned_eth_message

remove TODO

3e7653f

cleanup

3e87bfa

This was linked to issues Jul 12, 2024

panic in api_cmd #4518

Closed

CI RPC parity test failures in Filecoin.EthGetBlockByHash and Filecoin.EthGetBlockByNumber #4519

Closed

FIP-0091 support for Ethereum RPC methods. #4477

Closed

cleanup

82c2727

hanabi1224 marked this pull request as ready for review July 15, 2024 05:30

hanabi1224 requested a review from a team as a code owner July 15, 2024 05:30

hanabi1224 requested review from lemmih and LesnyRumcajs and removed request for a team July 15, 2024 05:30

LesnyRumcajs requested changes Jul 15, 2024

View reviewed changes

elmattic mentioned this pull request Jul 15, 2024

Move eth mapping creation to a background task #4474

Merged

4 tasks

Merge branch 'main' into hm/fix-eth_tx_from_signed_eth_message

33478dc

hanabi1224 mentioned this pull request Jul 16, 2024

chore(deps): bump the patch-versions group with 5 updates #4529

Merged

hanabi1224 added 4 commits July 16, 2024 13:03

remove redundant eth tx logic

15f80d5

cleanup

b821b5a

Merge remote-tracking branch 'origin/main' into hm/fix-eth_tx_from_si…

122ab4f

…gned_eth_message

cleanup

7f9cbe7

lemmih approved these changes Jul 16, 2024

View reviewed changes

LesnyRumcajs reviewed Jul 16, 2024

View reviewed changes

hanabi1224 added 2 commits July 16, 2024 19:17

move eth_hash out of rpc module

0882d32

1 more test

8b22871

hanabi1224 requested a review from LesnyRumcajs July 16, 2024 12:34

LesnyRumcajs reviewed Jul 16, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into hm/fix-eth_tx_from_si…

0a3144d

…gned_eth_message

refine rlp encoding logic

36c03a0

hanabi1224 force-pushed the hm/fix-eth_tx_from_signed_eth_message branch from 4b75183 to 36c03a0 Compare July 16, 2024 23:16

hanabi1224 added 2 commits July 17, 2024 07:32

spellcheck

92851f2

Merge branch 'main' into hm/fix-eth_tx_from_signed_eth_message

b4d2db6

hanabi1224 requested a review from LesnyRumcajs July 17, 2024 07:13

LesnyRumcajs approved these changes Jul 17, 2024

View reviewed changes

hanabi1224 added this pull request to the merge queue Jul 17, 2024

Merged via the queue into main with commit b60983a Jul 17, 2024
28 checks passed

hanabi1224 deleted the hm/fix-eth_tx_from_signed_eth_message branch July 17, 2024 07:54

hanabi1224 mentioned this pull request Jul 17, 2024

fix(ci): re-enable eth mapping check #4543

Merged

4 tasks

LesnyRumcajs mentioned this pull request Jul 17, 2024

Extend coverage for Ethereum transaction parsing #4478

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rpc): update eth_tx_from_signed_eth_message to support legacy transactions #4523

fix(rpc): update eth_tx_from_signed_eth_message to support legacy transactions #4523

hanabi1224 commented Jul 12, 2024 •

edited

Loading

LesnyRumcajs Jul 15, 2024

hanabi1224 Jul 16, 2024

LesnyRumcajs Jul 15, 2024

hanabi1224 Jul 16, 2024

LesnyRumcajs Jul 16, 2024

hanabi1224 Jul 16, 2024

LesnyRumcajs Jul 16, 2024

hanabi1224 Jul 16, 2024

LesnyRumcajs Jul 16, 2024

hanabi1224 Jul 16, 2024

LesnyRumcajs Jul 16, 2024

hanabi1224 Jul 16, 2024

LesnyRumcajs Jul 16, 2024

hanabi1224 Jul 16, 2024

LesnyRumcajs Jul 17, 2024

LesnyRumcajs Jul 17, 2024

fix(rpc): update eth_tx_from_signed_eth_message to support legacy transactions #4523

fix(rpc): update eth_tx_from_signed_eth_message to support legacy transactions #4523

Conversation

hanabi1224 commented Jul 12, 2024 • edited Loading

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanabi1224 commented Jul 12, 2024 •

edited

Loading