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

FIP-0091: Add support for legacy Ethereum transactions #995

Merged
merged 15 commits into from
Apr 27, 2024

Conversation

aarshkshah1992
Copy link
Contributor

This FIP adds support for legacy Homestead Ethereum transactions to Filecoin.

Note that only legacy Ethereum transactions with the following parameters will be supported.

type LegacyEthTx struct {
	Nonce int  
	To *EthAddress
	Value big.Int
	GasPrice big.Int
	GasLimit int  
	Input []byte  
	V big.Int 
	R big.Int 
	S big.Int
}

As legacy transactions do not have a ChainID, the same signed contract creation transaction can be used to create the contract on multiple chains and ensure that the address of the contract remains the same on all chains. There are some legacy contracts whose users need support for such chain agnostic transactions and contract creation.

Support for EIP-155 and EIP-2930 transactions is deferred to future FIPs.

Discussion at #962.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fip is really clear and well written.

FIPS/fip-00xx.md Outdated Show resolved Hide resolved
FIPS/fip-00xx.md Outdated

Given that the current signature verification implementation for the delegated signature type(which is the signature type that will be used for legacy ETH transactions in addition to EIP1599 transactions) only recognizes `V` values of 0 or 1, it is necessary to adjust the `V` value in the signature of legacy transactions by subtracting 27. This adjustment must be made prior to the authentication and verification of the message's signature, as well as before extracting the public key and sender's address from the legacy transaction's signature.

Note that this is only a temporary transformation for the purpose of a validating the signature of a legacy transaction and for extracting the public key of the sender from the signature. The `V` value that gets persisted in the chain state for the legacy transaction will be the original `V` value set on the legacy transaction by the user.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know... Can we use V to indicate the version (it can only be one of two values right now)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the V value is signed by the user. So we can't persist the mutated V value to the chain state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Stebalien Aha wait, do you mean that if V is 27 or 28, we just assume it's a legacy transaction ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well,

  1. We record a V in the signature.
  2. There's a V' stored/signed in the transaction.

But they don't have to be the same as long as we can derive one from the other.

But... yeah, we could also say 27/28 means "legacy" (I think). I don't feel strongly about this and you're now more familiar with the code than I am, but I'm bringing this up in case it ends up being a simpler/cleaner solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should go ahead and stick with using the marker byte in the signature to detect transaction types as it is more forward compatible and also makes it easy to add support for more transaction types down the line.

FIPS/fip-00xx.md Outdated

However, legacy Ethereum transactions only have a single `GasPrice` parameter, which specifies the gas price a sender is willing to pay per unit of gas to process the transaction.

Under the FVM fee market framework, when processing legacy transactions, `GasPrice` is adapted to simultaneously fulfill the roles of both `GasFeeCap` and `GasPremium`. In this adaptation, both `GasFeeCap` and `GasPremium` are set equal to `GasPrice`. Initially, the base fee is subtracted, and any remaining attoFIL from the `GasPrice`, after this deduction, is allocated entirely as a priority fee to the miner. This arrangement ensures that the total fee (base fee plus priority fee) paid by the user for a legacy transaction does not surpass the `GasPrice` set by the user on the legacy ETH transaction.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... This strongly incentivizes SPs to hoard these transactions until the gas fee drops. Not much we can do about that, I guess...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same thing would happen in Ethereum as well for legacy transactions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. But it may be worth noting it.

Copy link
Contributor Author

@aarshkshah1992 aarshkshah1992 Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly noted this.

@Schwartz10
Copy link

Schwartz10 commented Apr 18, 2024

Hey thanks for putting this together @aarshkshah1992 - one additional thing to note wrt "Change Motivation" - we also just generically get a lot of user's complaining about wallets not working to do simple sends of FIL (even outside of the GLIF UI). For instance, see this email from last night:

image

Sending FIL natively from Coinbase wallet (and several others like Trust, Phantom) should "just work". This doesn't seem related to being able to deploy smart contracts, but it's still quite important to fix because many user's are stuck with their FIL in wallets that throw this same error. Their only choice right now is to import their same private keys / seed phrases into a new wallet, which is not a safe practice. In an ideal world no wallet provider would send legacy txns, but that's just not the case.

Copy link
Member

@jsoares jsoares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very well drafted -- I wasn't terribly familiar with the details of eth transaction types but this makes it super clear. Nothing to point out from an editorial perspective.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent write-up! LGTM, just barring some typos and getting 1599 and 1559 mixed up (recommend find & replace all).

FIPS/fip-00xx.md Outdated Show resolved Hide resolved
FIPS/fip-00xx.md Outdated Show resolved Hide resolved
FIPS/fip-00xx.md Outdated Show resolved Hide resolved
FIPS/fip-00xx.md Outdated
The Filecoin network already supports EIP-1599 transactions. These transactions have a designated transaction type of `0x02` and have the following parameters:

```
type EIP1599Tx struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gofmtting would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

FIPS/fip-00xx.md Outdated Show resolved Hide resolved
FIPS/fip-00xx.md Outdated
3. Distinguish the transaction type based on the signature length:
- If the signature is 66 bytes long and starts with `0x80`, it is translated to a legacy ETH transaction.
- If the signature is 65 bytes long, it corresponds to an EIP-1559 transaction.
4. Validate the transaction parameters and signature.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's obvious, but we'd also strip the leading 0x80 byte from the signature of a legacy tx.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raulk You mean before verifying the signature, right ? Yes, ofcourse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this leading marker byte will ofcourse make it's way to the chain state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly noted this.

FIPS/fip-00xx.md Outdated Show resolved Hide resolved
FIPS/fip-00xx.md Outdated Show resolved Hide resolved
FIPS/fip-00xx.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent write-up! LGTM, just barring some typos and getting 1599 and 1559 mixed up (recommend find & replace all).

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent write-up! LGTM, just barring some typos and getting 1599 and 1559 mixed up (recommend find & replace all).

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent write-up! LGTM, just barring some typos and getting 1599 and 1559 mixed up (recommend find & replace all).

FIPS/fip-00xx.md Outdated

## Specification

Filecoin supports EIP-1559 transactions by converting them into standard Filecoin messages. These messages are distinguished by a unique "delegated" signature type, which encapsulates the original Ethereum transaction signature. The validation process for these messages involves:
Copy link
Member

@raulk raulk Apr 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Peer review) We need a subsection on Eth JSON-RPC support. We should scope exactly which methods are affected and how. I think eth_sendRawTransaction, eth_getTransactionByHash, eth_getBlockBy* are all affected.

FIPS/fip-00xx.md Outdated

Enabling support for legacy ETH transactions with this FIP allows for the straightforward deployment of these contracts on the Filecoin network by replaying the original, signed legacy contract deployment transactions on the Filecoin network.

## Specification
Copy link
Member

@raulk raulk Apr 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Peer review) Have you discarded the need for changes at the EVM runtime level? If so, may be worth stating that explicitly to aid implementers' understanding of scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly noted this.

@aarshkshah1992
Copy link
Contributor Author

@Schwartz10 Thanks for pointing that out. I have added the point about wallet support for legacy transactions to the Change Motivation section

FIPS/fip-00xx.md Outdated
@@ -0,0 +1,165 @@
---
fip: "XXXX"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assigning 0091

FIPS/fip-00xx.md Outdated Show resolved Hide resolved
FIPS/fip-00xx.md Outdated
}
```

The main distinction relevant to this FIP is that legacy transactions do not contain a `ChainID` and so the transactions are not scoped to a specific chain. This allows replaying the same signed contract creation transaction across multiple chains for deploying a contract and also enables the contract to have the same address across chains.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows replaying the same signed contract creation transaction across multiple chains

Just a question, is the risk only exposed to contract tx, but not to pure value transfer across wallet accounts (eth accounts) as well? (I'm curious how those two type of tx are differentiated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do value transfers with this as well using legacy transactions i.e. you can use the same signed value transfer message across networks for value transfer (the nonces, state etc should align for the transaction to be considered valid).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way it is written right now seems excluded this use case

FIPS/fip-00xx.md Outdated Show resolved Hide resolved
@jennijuju jennijuju changed the title FIP-XXXX: Add support for legacy Ethereum transactions FIP-0091: Add support for legacy Ethereum transactions Apr 23, 2024
FIPS/fip-0091.md Outdated
title: Add support for legacy Homstead Ethereum Trasnactions
author: Aarsh (@DharmaBum)
discussions-to: https://github.com/filecoin-project/FIPs/discussions/962
status: Accepted
Copy link
Member

@jennijuju jennijuju Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DO NOT MERGE. Please don’t change main key info w/o requesting fip editors’s re-review after their approvals.
This FIP is not Accepted. Fip editors’ approval on this PR only suggests the FIP draft is content ready to be merged to FIP repo. It doesn’t imply FIP’s readiness for the network. FIP must go through core dev and community’s review, and a last call period before it is accepted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jennijuju Apologies, I thought having 2 approvals meant that the FIP is now accepted. I have changed it back to Draft status.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good! Now you are a fip process master 😆😆

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.

6 participants