feat: ETH compatibility in Filecoin : Support EIP-155 Ethereum transactions in Filecoin #11970
Conversation
aarshkshah1992
changed the title
aarshkshah1992
changed the title
| @@ -126,15 +127,6 @@ func TestLegacyHomesteadSignatures(t *testing.T) { | |||
| "", | |||
| true, | |||
| }, | |||
| { | |||
| "0xf86f830131cf8504a817c800825208942cf1e5a8250ded8835694ebeb90cfa0237fcb9b1882ec4a5251d1100008026a0f5f8d2244d619e211eeb634acd1bea0762b7b4c97bba9f01287c82bfab73f911a015be7982898aa7cc6c6f27ff33e999e4119d6cd51330353474b98067ff56d930", | |||
There was a problem hiding this comment.
If this were specific to legacy homestead tx type this should still fail. We should probably move these to a filename that isn't specific to homestead.
There was a problem hiding this comment.
But this particular serialised tx has a ChainID of 1 in it's V value so it will be parsed as a EIP-155 tx.
The way we differentiate b/w EIP-155 & Homestead transactions is simply by looking at the V value and checking if it is 27 or 28.
There was a problem hiding this comment.
| if vUint64 == 27 || vUint64 == 28 { | ||
| return big.NewInt(0) | ||
| } | ||
| return big.NewIntUnsigned((vUint64 - 35) / 2) |
There was a problem hiding this comment.
Checking for understanding: this is always equal to the other valid value: big.NewIntUnsigned((vUint64 - 35) / 2) because of truncating division?
There was a problem hiding this comment.
@ZenGround0 Please can you elaborate on the question ? I don't understand this one.
There was a problem hiding this comment.
Which other valid value ?
There was a problem hiding this comment.
I meant other valid value: big.NewIntUnsigned((vUint64 - 36) / 2). Depending on the value of r and the curve when creating the signature the message creator could have set v = chainID * 2 +36
There was a problem hiding this comment.
Actually now I'm confused how you can deterministically recover without more information. Take for example a chain ID of 22. If parity is 0 then v value is 79. But for a chain id of 21 and a parity of 1 we also encode a v value of 79
(79 - 35)/2: 22
(79 - 36)/2: 21
So I think you'll need to determine the correct parity from the r value before decoding .
There was a problem hiding this comment.
It would be worth seeing how ethereum does this. Maybe there are some conventions around always choosing one parity or the other.
There was a problem hiding this comment.
@ZenGround0 Posted on Filecoin Slack but reproducing here for visibility:
So I dug deep into this.
- Looks like go-ethereum simply assumes a "recovery_id"(AKA "Y parity") of 0 when extracting the ChainId from the V value:
- https://github.com/ethereum/go-ethereum/blob/804afb8faa0e3796fb8e6f39beabce356bc98298/core/types/transaction_signing.go#L567
- https://github.com/ethereum/go-ethereum/blob/804afb8faa0e3796fb8e6f39beabce356bc98298/core/types/transaction.go#L225
- There is no code in go-ethereum that somehow manipulates this V value to detect the "recovery_id" before deriving the ChainID from it
Having said that, this is not a big concern because:
The only check we need to perform is that the user has signed the correct ChainId and that is done here by inserting the expected ChainID in the digest:
- Really, this is all we need. Once past this, we don't need to validate the ChainId /derive the ChainID from the V value. We only really do it as a sanity check.
- So we can just completely get rid of deriveEIP155ChainId /validateEIP155ChainId call after ensuring that the user has signed the correct digest
- I am however still inclined to keep this around because in all my testing with CoinBase Wallet etc, the correct ChainId is indeed derived
- If at all in the future, users complain of their EIP-155 transactions not going through, we can remove this check/look into it
Let me know what you think and thanks for eagle eyes 👀
I also agree that we should get Raul/Steb/someone else with deep ETH knowledge to look at this before we ship it but want to make sure you are satisfied here before we move on to that. Thanks a lot. (edited)
There was a problem hiding this comment.
This has been resolved offline with @ZenGround0
|
@ZenGround0 I have addressed your review. Please can you give it another look ? |
aarshkshah1992
merged commit b2a10b4
into
feat/legacy-homestead-tx
…thereum transactions("legacy" transactions) in Filecoin (#11969)
* poc for eth legacy tx
* print statements
* finished
* tests work
* remove print statements
* Remove all print statements
* remove extraneous changes
* cleaned up code and interface
* run make jen
* dont duplicate signature
* go mod tidy and remove prints
* clean up tests
* test for conversion
* changes as per review
* more unit tests for legacy txns
* Apply suggestions from code review
Co-authored-by: Rod Vagg <rod@vagg.org>
* address review comments from Rodd
* changes as per zen's 2nd review
* go mod tidy
* feat: ETH compatibility in Filecoin : Support EIP-155 Ethereum transactions 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
* gate tx by Network Version
* handle arajsek review
* fix imports order
* fix lint
* dont lock in mpool for network gating ETH messages
* sender can be an ID address
---------
Co-authored-by: Rod Vagg <rod@vagg.org>
Related Issues
For #11859 (for the EIP-155 transactions support)
FIP at filecoin-project/FIPs#995
Proposed Changes
This PR enables support for EIP-155 transactions with chain replay protection 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