feat(node): support legacy transactions by cryptoAtwill · Pull Request #1235 · consensus-shipyard/ipc

cryptoAtwill · 2024-12-16T12:14:15Z

Closes #927.

The current main does not allow send_raw_transaction for legacy transactions. This PR removes this assumption. To achieve it, the following changes are made:

Adding OriginKind enum to explicitly specify the original transaction type. The type information is lost when converting to Fvm messages.
Support gas params from legacy transactions as specified in https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0091.md
Any ethereum transaction sent from fendermint client is enforced to be eip1559.
TransactionCache is updated to use (TypeTransaction, Signature) instead of the response dto Transaction.

…gacy-txn

raulk · 2024-12-17T12:23:56Z

fendermint/eth/api/src/conv/from_eth.rs

+        _ => error::error_with_revert(
+            ExitCode::USR_ILLEGAL_ARGUMENT,
+            "txn type not supported",
+            None::<Vec<u8>>,
        ),


Why are we duplicating this logic here? send_raw_transaction is already producing this error?

raulk · 2024-12-17T12:32:35Z

fendermint/eth/api/src/apis/eth.rs

-    }
+    data.tx_cache.insert(
+        msghash,
+        from_eth::to_eth_transaction_response(&tx, sig, msghash)?,


What do you mean with transaction response here? This module is an API, so I interpret as if this is returning the response the API sends to the caller, which isn't the case? (unless there's an error)

We are duplicating the transaction type validation in this method and when we calculate the origin kind above. Can we refactor this to make it DRY?

BTW -- how can we have a module called from_eth that then has to_eth methods? Wouldn't they cancel each other out? 😆

raulk · 2024-12-17T12:33:13Z

fendermint/eth/api/src/conv/from_eth.rs

+pub fn to_eth_transaction_response(
+    tx: &TypedTransaction,
    sig: et::Signature,
    hash: et::TxHash,
-) -> et::Transaction {
-    et::Transaction {
-        hash,
-        nonce: tx.nonce.unwrap_or_default(),
-        block_hash: None,
-        block_number: None,
-        transaction_index: None,
-        from: tx.from.unwrap_or_default(),
-        to: tx.to.and_then(|to| to.as_address().cloned()),
-        value: tx.value.unwrap_or_default(),
-        gas: tx.gas.unwrap_or_default(),
-        max_fee_per_gas: tx.max_fee_per_gas,
-        max_priority_fee_per_gas: tx.max_priority_fee_per_gas,
-        // Strictly speaking a "Type 2" transaction should not need to set this, but we do because Blockscout
-        // has a database constraint that if a transaction is included in a block this can't be null.
-        gas_price: Some(
-            tx.max_fee_per_gas.unwrap_or_default()
-                + tx.max_priority_fee_per_gas.unwrap_or_default(),
+) -> Result<et::Transaction, JsonRpcError> {


I really dislike that the cache uses et::Transaction -- this type is intended as a return type from the JSON-RPC API, not to store canonical signed transactions.

raulk · 2024-12-17T12:34:50Z

fendermint/eth/api/src/conv/from_eth.rs

-            tx.max_fee_per_gas.unwrap_or_default()
-                + tx.max_priority_fee_per_gas.unwrap_or_default(),
+) -> Result<et::Transaction, JsonRpcError> {
+    match &tx {


This all looks very repetitive. Can't you populate the common values from TransactionEssentials via essentials()? And then add the specifics?

Isn't there a method to normalize typed transactions into the Transaction response type in ethers?

Isn't there a method to normalize typed transactions into the Transaction response type in ethers?
From what I found, not really. My guess is the Transaction response contains transaction index, which does not make sense to do the conversion using From.

cryptoAtwill · 2024-12-18T06:27:37Z

@raulk Because the types and from_eth::* and to_eth::* are used in multiple places, I just did a extension to make it work. I think it's a good chance to do a more in depth refactoring. Will update accordingly.

raulk · 2024-12-18T15:58:58Z

fendermint/eth/api/src/apis/eth.rs

            let hash = msg_hash(&res.tx_result.events, &res.tx);
-            let mut tx = to_eth_transaction(msg, chain_id, hash)?;
+            let mut tx = to_eth_transaction_response(msg, chain_id)?;
+            debug_assert_eq!(hash, tx.hash);


Not sure we want to crash the process here? Wouldn't it be best to return an internal error?

they should be the same, debug_assert_eq should be fine for release build though, more like a sanity check for tests.

fendermint/eth/api/src/apis/eth.rs

raulk · 2024-12-18T16:01:37Z

fendermint/eth/api/src/apis/eth.rs

    Ok(None)
 }

+fn normalize_signature(sig: &mut et::Signature) -> JsonRpcResult<()> {


Why not use Signature::as_signature(), which returns a normalized recoverable signature?

Looks like it's not exposed...

Yeah, a lot of methods are not exposed. Could be helpful util tools.

raulk · 2024-12-19T16:48:41Z

fendermint/eth/api/src/apis/eth.rs

    let nonce = msg.sequence;

    let msg = SignedMessage {
+        origin_kind: derive_origin_kind(&tx)?,


Would it make sense to implement TryFrom<TypedTransaction> for OriginKind instead? Then you can do tx.try_into()? here?

raulk · 2024-12-19T16:56:17Z

fendermint/eth/api/src/apis/eth.rs

-    if let Some(tx) = tx.as_eip1559_ref() {
-        let tx = from_eth::to_eth_transaction(tx.clone(), sig, msghash);
-        data.tx_cache.insert(msghash, tx);
-    }


Am I reading right that prior to this PR, if a transaction failed the checks (broadcast_tx_sync or others), it would remain forever in the cache?

It won't remain forever as it's using LRU cache. It will just stay for some time, depending on the configuration.

raulk · 2024-12-19T17:32:21Z

fendermint/eth/api/src/mpool.rs

 /// API would not be able to find them until they are delivered to the application
 /// and indexed by their domain hash, which some tools interpret as the transaction
 /// being dropped from the mempool.
-pub type TransactionCache = Cache<et::TxHash, et::Transaction>;


Honestly, now that I look at the final solution, I think this was better the way it was before. We ended up leaking all of ethers' type complexity into Tendermint. In fact, I don't think TypedTransaction should've leaked anywhere at all. I'll work on rolling this back, removing to_eth_typed_transaction, to_eth_legacy_request, to_eth_eip1559_request, etc.

I feel the conv::* should not be in message crate. It should be shifted to eth crate. That might be better.

Co-authored-by: raulk <raul@protocol.ai>

raulk

Let's go ahead and merge this to unblock users who are waiting for this feature to land. We can clean this up in main later.

cryptoAtwill added 8 commits December 11, 2024 16:53

support legacy txn

169dabd

verify signatures

fe1a1ae

Merge branch 'main' of protocol-github:consensus-shipyard/ipc into le…

2a15a83

…gacy-txn

update tests

04edd61

test case for legacy

fb842b9

update comment

ddbc2e0

fix value

b41e31a

support legacy in tx cache

d6a006d

cryptoAtwill requested a review from a team as a code owner December 16, 2024 12:14

cryptoAtwill added 2 commits December 16, 2024 20:21

support legacy txn

6eb1f36

push test

9f0f3b4

raulk requested changes Dec 17, 2024

View reviewed changes

more clean ups

0647dcb

cryptoAtwill changed the base branch from legacy-txn to main December 18, 2024 13:52

cryptoAtwill mentioned this pull request Dec 18, 2024

feat(node): support ethereum legacy txn #1232

Closed

cryptoAtwill requested a review from raulk December 18, 2024 13:57

raulk changed the title ~~feat(node): support legacy in tx cache~~ feat(node): support legacy transactions Dec 18, 2024

raulk linked an issue Dec 18, 2024 that may be closed by this pull request

Support Eth legacy transactions #927

Closed

raulk reviewed Dec 19, 2024

View reviewed changes

Update fendermint/eth/api/src/apis/eth.rs

68a8f9b

Co-authored-by: raulk <raul@protocol.ai>

raulk approved these changes Dec 20, 2024

View reviewed changes

raulk merged commit 2894821 into main Dec 20, 2024

raulk deleted the legacy-txn-cache branch December 20, 2024 11:32

Conversation

cryptoAtwill commented Dec 16, 2024 • edited by raulk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptoAtwill commented Dec 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raulk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cryptoAtwill commented Dec 16, 2024 •

edited by raulk

Loading