feat: ETH compatibility in Filecoin : Support Homestead and EIP-155 Ethereum transactions("legacy" transactions) in Filecoin

[aarshkshah1992]

Related Issues

For #11859

Proposed Changes

FIP discussion at filecoin-project/FIPs#962.
FIP proposal at filecoin-project/FIPs#995

Implement Ethereum Homestead transactions and EIP-155 transactions(both are referred to as "legacy" transactions) in Filecoin.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

• Commits have a clear commit message.
• PR title is in the form of of <PR type>: <area>: <change being made>
  • example: fix: mempool: Introduce a cache for valid signatures
  • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
  • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
• If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
• New features have usage guidelines and / or documentation updates in
  • Lotus Documentation
  • Discussion Tutorials
• Tests exist for new functionality or change in behavior
• CI is green

[arajasek]

Some notes:

• The new logic for the two types of Homestead transactions seems correct, assuming the FIP is correctly specified. I'll stare at it more in second review, but everything seems to do what it should.
• I'm not too strongly in favour of this change, it's adding a lot more edgecase-handling to what is already one of the gnarliest parts of the Filecoin protocol. I see the strong support for it in the FIP discussion, and this is about as lightweight an approach as can be expected for this change, so I'm happy to disagree-and-commit. But I do think this is a step in the wrong direction.
  • In particular, I don't think we should lean into an attitude of "well, it's just in the client, so it's okay". It's still very much sharp edges in the protocol, and the fact that it's in the client only means that it needs to be (potentially incorrectly / incompatibly) re-implemented by Forest and Venus too.
• Directly related to the previous point, I unfortunately don't think this PR is safe to land as-is, because I strongly suspect it unintentionally breaks existing behaviour in subtle ways. In general, it seems like this PR will tighten conditions on Eth1559 transactions. I don't think there's anything wrong with the changes, but they'll still break consensus on nv22 (so, if anything, we'll have to put them behind a network version check).
  • As an example (the only one I can see so far, though there might be more), I'm pretty sure we can have messages with ID addresses as the sender carrying a Delegated signature today. I think that behaviour will change with this PR (it'll become invalid).

[ZenGround0]

But I do think this is a step in the wrong direction.

When you say this are you referring to the FIP change itself?

As an example (the only one I can see so far, though there might be more), I'm pretty sure we can have messages with ID addresses as the sender carrying a Delegated signature today. I think that behaviour will change with this PR (it'll become invalid).

Can you explain this more. My understanding is that EIP1559 Txs won't have any behavior changes at all with this change. How does this PR/FIP impact ID address sender of EIP1559 Tx?

[aarshkshah1992]

So tested this with CB Wallet on devnet after implementing Network Version Gating and looks good now. Transactions fail for NV22(when devnet epoch < NV23 epoch) but work for NV23(when devnet epoch >= NV23 epoch) !

[arajasek]

But I do think this is a step in the wrong direction.

When you say this are you referring to the FIP change itself?

Yes.

As an example (the only one I can see so far, though there might be more), I'm pretty sure we can have messages with ID addresses as the sender carrying a Delegated signature today. I think that behaviour will change with this PR (it'll become invalid).

Can you explain this more. My understanding is that EIP1559 Txs won't have any behavior changes at all with this change. How does this PR/FIP impact ID address sender of EIP1559 Tx?

EIP-1559 Txs shouldn't have any behaviour changes with this FIP. I'm concerned that this PR will cause some changes (example above) due to the refactor, which is consensus-breaking.

[arajasek]

@arajasek @ZenGround0

* I agree that this FIP needs to be implemented by both Forrest and Venus but that should not be a reason to not ship a protocol change that has been requested by multiple users. We need to build for what they need.

The objection isn't that 3 implementations have to ship the change, the objection is that this is further messing up what is already a pretty janky part of the Filecoin protocol.

This is the part that I do not understand. Why will messages with ID addresses and a delegated signature become invalid with this PR ? I

Because of the refactor. You're now calling EthTransactionFromSignedFilecoinMessage instead of EthTxArgsFromUnsignedEthMessage as part of signature authentication, which is asserting that the sender is an f4 address.

[aarshkshah1992]

@arajasek

This is the part that I do not understand. Why will messages with ID addresses and a delegated signature become invalid with this PR ? I

Because of the refactor. You're now calling EthTransactionFromSignedFilecoinMessage instead of EthTxArgsFromUnsignedEthMessage as part of signature authentication, which is asserting that the sender is an f4 address.

The same behaviour is also enforced in master for EIP-1559 transactions. There is no behaviour change for EIP-1559 txns in this PR.
See

lotus/chain/types/ethtypes/eth_transactions.go

Line 68 in 1879570

if !IsEthAddress(smsg.Message.From) {

in master.

[aarshkshah1992]

@Stebalien Pls can you approve this if lgty ?

[arajasek]

@arajasek

Because of the refactor. You're now calling EthTransactionFromSignedFilecoinMessage instead of EthTxArgsFromUnsignedEthMessage as part of signature authentication, which is asserting that the sender is an f4 address.

The same behaviour is also enforced in master for EIP-1559 transactions. There is no behaviour change for EIP-1559 txns in this PR. See

lotus/chain/types/ethtypes/eth_transactions.go

Line 68 in 1879570

if !IsEthAddress(smsg.Message.From) {

in master.

Please read the code and my message more carefully. We are calling EthTxArgsFromUnsignedEthMessage on master, which does not assert that the sender is an f4 address. AFAIC tell nothing currently running on mainnet does.

With this refactor, we will start asserting that the sender of EIP-1559 transactions is an f4 address. If I'm right about all of this, that would be a consensus break. I may not be right, but we need to be sure of that, and this PR cannot land until we are.

[aarshkshah1992]

Oh hey @arajasek

I see what you mean. We do NOT in fact allow EIP-1559 messages where sender is a non-ETH(f4) address and fail these transactions during signature verification.

See:

lotus/lib/sigs/delegated/init.go

Line 65 in 1879570

maybeaddr, err := address.NewDelegatedAddress(builtin.EthereumAddressManagerActorID, addrHash[12:])

Basically, the sender of a delegated signature type MUST be an f4 address for the signature verification to succeed.

[arajasek]

Oh hey @arajasek

I see what you mean. We do NOT in fact allow EIP-1559 messages where sender is a non-ETH(f4) address and fail these transactions during signature verification.

See:

lotus/lib/sigs/delegated/init.go

Line 65 in 1879570

maybeaddr, err := address.NewDelegatedAddress(builtin.EthereumAddressManagerActorID, addrHash[12:])

Basically, the sender of a delegated signature type MUST be an f4 address for the signature verification to succeed.

@aarshkshah1992 Thanks for digging further! I’m not quite convinced yet -- the line you linked to extracts the address from the signature carried by the message (which can obviously never be an ID address). But what happens if the sender field of a message is an ID address? That case will definitely not be supported (invalid message) after this refactor, because of the additional checks in EthTransactionFromSignedFilecoinMessage. But is that possible on mainnet today? If so, this refactor is unsafe.

[aarshkshah1992]

Oh hey @arajasek

• The line immediately below that at

  lotus/lib/sigs/delegated/init.go

  Line 70 in cf8d187

  if maybeaddr != a {

  ensures that the address passed down to the Verify function matches the address extracted from the signature and the address extracted from the signature has to be an ID address as per

  lotus/lib/sigs/delegated/init.go

  Line 65 in cf8d187

  maybeaddr, err := address.NewDelegatedAddress(builtin.EthereumAddressManagerActorID, addrHash[12:])

• You can see that the sender of the message is indeed passed as an argument to the signature Verify function at

  lotus/chain/consensus/signatures.go

  Line 47 in cf8d187

  if err := sigs.Verify(&msg.Signature, signer, digest); err != nil {

  .

[arajasek]

The line immediately below that at

lotus/lib/sigs/delegated/init.go

Line 70 in cf8d187

if maybeaddr != a {

ensures that the address passed down to the Verify function matches the address extracted from the signature and the address extracted from the signature has to be an ID address as per

lotus/lib/sigs/delegated/init.go

Line 65 in cf8d187

maybeaddr, err := address.NewDelegatedAddress(builtin.EthereumAddressManagerActorID, addrHash[12:])

I assume you mean has to be a delegated (f4) address here?

You can see that the sender of the message is indeed passed as an argument to the signature Verify function at

lotus/chain/consensus/signatures.go

Line 47 in cf8d187

if err := sigs.Verify(&msg.Signature, signer, digest); err != nil {

Is the sender of the message being passed? I see the signer being passed as an argument here, but going up one more level it looks like that has already been resolved to a deterministic address. AFAIC tell this confirms my concern?

[arajasek]

To break this down:

• Using master today, can I send a message carrying an f4 signature on mainnet whose From field is an ID address?
  • Yes / No / I don't know.
• With this refactor, can I send a(n EIP-1559) message carrying an f4 signature on mainnet whose From field is an ID address?
  • Yes / No / I don't know.

If the answer to either question is I don't know, we need to figure out the actual answer (with confidence). If the answer to both questions isn't the same (both Yes or both No), we have a consensus split caused by this change, and so it isn't safe to land.

(I think the answers are Yes and No respectively, which would be a problem if true, but that's what needs to be confirmed).

[aarshkshah1992]

@arajasek

• So based on feat: ETH compatibility in Filecoin : Support Homestead and EIP-155 Ethereum transactions("legacy" transactions) in Filecoin #11969 (comment), I wrote some tests on master
• The test is at [DO NOT MERGE] EIP-1559 transaction with the Sender as an ID address fails #12061 (has some new code in mpool validation to get it to work)
• You can't get EIP-1559 messages with non-ID addresses through the Lotus mpool verification implementation -> it has to have an f4 address for mpool inclusion
• However, if an incoming block has an EIP-1559 transaction with the sender as an ID address (albeit one that can be resolved to a f4 address) -> it will go through and you are right here 👍
• So, if a Miner decides to mine a block using their own custom mpool implementation that does not use/copy the Lotus mpool implementation -> EIP-1559 transactions with ID senders will indeed go through on master but fail on our branch with the legacy TX implementation (tried that as well)
• So yeah, this is indeed a consensus breaking change for the protocol 👍
• I've fixed this in the latest commit on the PR -> basically have completely removed the "sender should be an f4 address" check. If the sender is an ID address that can not be resolved to an f4 address, signature verification will anyways fail. And we want to keep the legacy tx behaviour with the EIP-1559 tx behaviour.

[arajasek]

Thanks a lot for looking into this in detail! I'll give it a second review today, but it should be good to go.

[rvagg]

🚢

[Stebalien]

The network-version checks look correct to me.

I would, however, recommend making them a bit more generic (can also happen in a followup so we can get this merged). Basically, combine the IsValidSecpkSigType, the "is delegate signature", and the eth check into a single IsValidSignedMessageForSending check.