feat: ETH compatibility in Filecoin : Support Homestead and EIP-155 Ethereum transactions("legacy" transactions) in Filecoin #11969
Conversation
Co-authored-by: Rod Vagg <rod@vagg.org>
aarshkshah1992
changed the title
…ctions in Filecoin (#11970) * itests passing for 155 tx * first working version for EIP-155 transactions * green itest * add docs * tests * remove print stmt * remove print stmt * validate signature * changes as per zen's review * correct signature verification
aarshkshah1992
changed the title
There was a problem hiding this comment.
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).
| to = &addr | ||
| } else { | ||
| return nil, nil, | ||
| fmt.Errorf("invalid methodnum %d: only allowed method is InvokeContract(%d)", |
There was a problem hiding this comment.
nit: can also be CreateExternal
| // For EthLegacy155TxArgs, the digest that is used to create a signed transaction includes the `ChainID` but the serialised RLP transaction | ||
| // does not include the `ChainID` as an explicit field. Instead, the `ChainID` is included in the V value of the signature as mentioned above. | ||
| type EthLegacy155TxArgs struct { | ||
| LegacyTx *EthLegacyHomesteadTxArgs |
There was a problem hiding this comment.
As written, this is a guarantee to cause bugs in the future when people accidentally call the "inner" methods on EthLegacyHomesteadTxArgs instead of the correct methods (on EthLegacy155TxArgs). Please rewrite this, you could:
- create an inner struct that's just the fields (no logic), and then have two separate structs that contain that struct
- just duplicate the fields in
EthLegacy155TxArgs, it's fine to do so - if you really want to keep the existing pattern, make
LegacyTxprivate, and rewrite external usages of it to access it properly (eg. by creating a constructor)
There was a problem hiding this comment.
Fixed this by making it private.
When you say this are you referring to the FIP change itself?
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? |
| txArgs, err := ethtypes.EthTxArgsFromUnsignedEthMessage(&msg.Message) | ||
| signatureCopy.Data = make([]byte, len(msg.Signature.Data)) | ||
| copy(signatureCopy.Data, msg.Signature.Data) | ||
| ethTx, err := ethtypes.EthTransactionFromSignedFilecoinMessage(msg) |
There was a problem hiding this comment.
Coming back to this again I'm having some paranoid thoughts. Does this PR open up forking as written?
We release this code before nv23, someone comes in with a legacy Tx, sends it over to an SP with updated software. That SP includes in a block. Now an SP who is not updated yet reads the block. It has this weird signature with one extra byte. It doesn't validate per its code. SP rejects block.
I could be off base and missing some subtlty that prevents this. Just want to think it over with you before merge.
There was a problem hiding this comment.
I agree with @arajasek and @ZenGround0 that this PR would definitely create consensus issues if some nodes are able to execute a transaction and other nodes would reject it. Was there any discussion in the FIP process around how to handle the consensus breaking aspects of supporting this new transaction type?
There was a problem hiding this comment.
@snissn @ZenGround0 I have update this PR to only accept legacy txs if the node is on NV23. If the Node still hasn't upgraded to NV23, it will fail to accept legacy txns.
I have also added an itest for it. It's in test TestLegacyEIP155ValueTransferValidSignatureFailsNV22. I think this should suffice. Let me know if you have any other concerns.
|
All checks have completed 🚫 Cancelled Test / Test (itest-api) (pull_request) |
- I have added network version gating to this and so nodes will not accept legacy txs if they are not on NV23. 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 |
| // SignLegacyHomesteadTransaction signs a legacy Homstead Ethereum transaction in place with the supplied private key. | ||
| func (e *EVM) SignLegacyEIP155Transaction(tx *ethtypes.EthLegacy155TxArgs, privKey []byte, chainID big.Int) { | ||
| preimage, err := tx.ToRlpUnsignedMsg() | ||
| require.NoError(e.t, err) | ||
|
|
||
| // sign the RLP payload | ||
| signature, err := sigs.Sign(crypto.SigTypeDelegated, privKey, preimage) | ||
| require.NoError(e.t, err) | ||
|
|
||
| signature.Data = append([]byte{ethtypes.EthLegacy155TxSignaturePrefix}, signature.Data...) | ||
|
|
||
| chainIdMul := big.Mul(chainID, big.NewInt(2)) | ||
| vVal := big.Add(chainIdMul, big.NewIntUnsigned(35)) | ||
|
|
||
| switch signature.Data[len(signature.Data)-1] { | ||
| case 0: | ||
| vVal = big.Add(vVal, big.NewInt(0)) | ||
| case 1: | ||
| vVal = big.Add(vVal, big.NewInt(1)) | ||
| } | ||
|
|
||
| signature.Data = append(signature.Data[:65], vVal.Int.Bytes()...) | ||
|
|
||
| err = tx.InitialiseSignature(*signature) | ||
| require.NoError(e.t, err) | ||
| } | ||
|
|
||
| // SignLegacyHomesteadTransaction signs a legacy Homstead Ethereum transaction in place with the supplied private key. | ||
| func (e *EVM) SignLegacyHomesteadTransaction(tx *ethtypes.EthLegacyHomesteadTxArgs, privKey []byte) { |
There was a problem hiding this comment.
Can we try to get rid of these specialized methods? Some ideas:
-
Make
SignTransactiontake an interface and the chain id:interface RlpUnsignedMsger { ToRlpUnsignedMsg() ([]byte, error) }
-
Encapsulate the signature enhancement in a standard method in the tx type itself.
|
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) ! 
|
chain/messagepool/messagepool.go
Outdated
| @@ -824,7 +824,16 @@ func (mp *MessagePool) VerifyMsgSig(m *types.SignedMessage) error { | |||
| return nil | |||
| } | |||
|
|
|||
| if err := consensus.AuthenticateMessage(m, m.Message.From); err != nil { | |||
| mp.curTsLk.RLock() | |||
There was a problem hiding this comment.
from a thread safety perspective this block looks ok to me
There was a problem hiding this comment.
Are you sure we want to do this here? We previously did this here:
lotus/chain/messagepool/messagepool.go
Lines 889 to 892 in 9983c46
We intentionally avoid taking any locks or looking at any global state in checkMsg or anything it calls.
Yes.
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. |
chain/consensus/signatures.go
Outdated
| } | ||
| roundTripMsg, err := txArgs.ToUnsignedMessage(msg.Message.From) | ||
|
|
||
| if ethTx.Type() != ethtypes.EIP1559TxType { |
There was a problem hiding this comment.
nit: use && instead of nested ifs
There was a problem hiding this comment.
Removed this.
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.
Because of the refactor. You're now calling |
chain/messagepool/messagepool.go
Outdated
| @@ -824,7 +824,16 @@ func (mp *MessagePool) VerifyMsgSig(m *types.SignedMessage) error { | |||
| return nil | |||
| } | |||
|
|
|||
| if err := consensus.AuthenticateMessage(m, m.Message.From); err != nil { | |||
| mp.curTsLk.RLock() | |||
There was a problem hiding this comment.
Are you sure we want to do this here? We previously did this here:
lotus/chain/messagepool/messagepool.go
Lines 889 to 892 in 9983c46
We intentionally avoid taking any locks or looking at any global state in checkMsg or anything it calls.
chain/consensus/signatures.go
Outdated
| @@ -16,36 +16,51 @@ import ( | |||
| // SignedMessage was signed by the indicated Address, computing the correct | |||
| // signature payload depending on the signature type. The supplied Address type | |||
| // must be recognized by the registered verifier for the signature type. | |||
| func AuthenticateMessage(msg *types.SignedMessage, signer address.Address) error { | |||
| func AuthenticateMessage(msg *types.SignedMessage, signer address.Address, nv network.Version) error { | |||
There was a problem hiding this comment.
We previously enforced network-version specific requirements here:
lotus/chain/consensus/common.go
Lines 238 to 240 in 9983c46
I'd avoid network version logic here.
There was a problem hiding this comment.
@Stebalien I have addressed this.
The same behaviour is also enforced in master for EIP-1559 transactions. There is no behaviour change for EIP-1559 txns in this PR. |
|
@Stebalien Pls can you approve this if lgty ? |
Please read the code and my message more carefully. We are calling 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. |
|
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 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 |
|
Oh hey @arajasek
|
I assume you mean has to be a delegated (f4) address here?
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? |
|
To break this down:
If the answer to either question is (I think the answers are Yes and No respectively, which would be a problem if true, but that's what needs to be confirmed). |
| @@ -113,10 +113,6 @@ func EthTransactionFromSignedFilecoinMessage(smsg *types.SignedMessage) (EthTran | |||
| if smsg == nil { | |||
| return nil, errors.New("signed message is nil") | |||
| } | |||
| // Validate the sender's address format. | |||
There was a problem hiding this comment.
Mpool validation/Block validation already enforces that the sender should either be an f4 address or an ID address that can be resolved to an f4 address. No harm in allowing legacy txns to have ID addresses as sender.
|
|
Thanks a lot for looking into this in detail! I'll give it a second review today, but it should be good to go. |
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:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps