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

[action] support pre-EIP155 unprotected transaction #3965

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

dustinxie
Copy link
Member

@dustinxie dustinxie commented Nov 9, 2023

Description

Currently, our chain only supports Ether-compatible tx with EIP-155 signature. This PR adds support for old unprotected tx (before EIP-155)

For more details, see https://iotex.larksuite.com/wiki/QN4RwY6fOilXrWk08VHuj9uGs5d

Fixes #(issue)

Type of change

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • [] Code refactor or improvement
  • [] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
  • [] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • make test
  • [] fullsync
  • [] Other test (please specify)

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

go.mod Show resolved Hide resolved
action/sealedenvelope.go Outdated Show resolved Hide resolved
action/sealedenvelope.go Outdated Show resolved Hide resolved
action/rlp_tx.go Outdated Show resolved Hide resolved
action/rlp_tx.go Outdated Show resolved Hide resolved
action/rlp_tx.go Outdated Show resolved Hide resolved
action/rlp_tx.go Outdated Show resolved Hide resolved
action/rlp_tx.go Outdated Show resolved Hide resolved
api/web3server.go Outdated Show resolved Hide resolved
action/rlp_tx.go Outdated Show resolved Hide resolved
action/rlp_tx_test.go Outdated Show resolved Hide resolved
api/web3server.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Attention: 503 lines in your changes are missing coverage. Please review.

Comparison is base (e1f0636) 75.38% compared to head (6d21367) 76.14%.
Report is 101 commits behind head on master.

❗ Current head 6d21367 differs from pull request most recent head 16e0be0. Consider uploading reports for the commit 16e0be0 to get more accurate results

Files Patch % Lines
blockindex/contractstaking/event_handler.go 67.23% 45 Missing and 13 partials ⚠️
action/protocol/staking/staking_statereader.go 69.76% 35 Missing and 17 partials ⚠️
action/protocol/execution/evm/evm.go 42.16% 47 Missing and 1 partial ⚠️
action/protocol/staking/protocol.go 33.33% 28 Missing ⚠️
...tion/protocol/staking/contractstake_bucket_type.go 0.00% 24 Missing ⚠️
blockindex/contractstaking/util.go 39.47% 22 Missing and 1 partial ⚠️
action/protocol/rewarding/fund.go 23.07% 19 Missing and 1 partial ⚠️
api/coreservice.go 69.69% 14 Missing and 6 partials ⚠️
api/websocket.go 0.00% 20 Missing ⚠️
action/protocol/execution/evm/evmstatedbadapter.go 0.00% 19 Missing ⚠️
... and 29 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3965      +/-   ##
==========================================
+ Coverage   75.38%   76.14%   +0.76%     
==========================================
  Files         303      330      +27     
  Lines       25923    28138    +2215     
==========================================
+ Hits        19541    21425    +1884     
- Misses       5360     5614     +254     
- Partials     1022     1099      +77     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

action/rlp_tx.go Outdated Show resolved Hide resolved
api/web3server_utils.go Outdated Show resolved Hide resolved
action/rlp_tx.go Outdated Show resolved Hide resolved
@Liuhaai
Copy link
Member

Liuhaai commented Nov 10, 2023

one question: does this pr depend on #3967 ?

}

// ExtractTypeSigPubkey extracts tx type, signature, and pubkey
func ExtractTypeSigPubkey(tx *types.Transaction) (iotextypes.Encoding, []byte, crypto.PublicKey, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

split to 2 funcs for clarity

unprotectedTx := "0xf8a58085174876e800830186a08080b853604580600e600039806000f350fe7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe03601600081602082378035828234f58015156039578182fd5b8082525050506014600cf31ba02222222222222222222222222222222222222222222222222222222222222222a02222222222222222222222222222222222222222222222222222222222222222"
for _, chainID := range []uint32{_evmNetworkID, _evmNetworkID + 1, 0} {
_, _, _, err := DecodeRawTx(unprotectedTx, chainID)
r.Equal(crypto.ErrInvalidKey, err)
Copy link
Member Author

Choose a reason for hiding this comment

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

test to verify new unprotected tx will cause error using current code

@dustinxie dustinxie changed the title [action] support unprotected and EIP-2930 transaction [action] support pre-EIP15 unprotected transaction Nov 14, 2023
@dustinxie
Copy link
Member Author

one question: does this pr depend on #3967 ?

yes, this PR has now rebased to 3967, pls check again

err error
)
if g := cs.Genesis(); g.IsSumatra(cs.TipHeight()) {
tx, err = action.DecodeEtherTx(dataStr.String())
Copy link
Member Author

Choose a reason for hiding this comment

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

use new code path after hard-fork

@dustinxie dustinxie changed the title [action] support pre-EIP15 unprotected transaction [action] support pre-EIP155 unprotected transaction Nov 15, 2023
switch txType {
case iotextypes.Encoding_IOTEX_PROTOBUF:
// native tx use same signature format as that of Homestead
return types.HomesteadSigner{}, nil
case iotextypes.Encoding_ETHEREUM_RLP:
case iotextypes.Encoding_ETHEREUM_EIP155:
Copy link
Member

@Liuhaai Liuhaai Nov 15, 2023

Choose a reason for hiding this comment

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

iotextypes.Encoding_ETHEREUM_RLP & iotextypes.Encoding_ETHEREUM_PRE_EIP155?
Otherwise, there will be backwards compatibility issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Encoding_ETHEREUM_UNPROTECTED is a better name/term, Ethereum call it unprotected tx, and there is Protected() bool func

Copy link
Member Author

@dustinxie dustinxie Nov 15, 2023

Choose a reason for hiding this comment

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

there won't be backwards compatibility issue, _EIP155 is an alias of _RLP

Encoding_ETHEREUM_EIP155 == Encoding_ETHEREUM_RLP == 1

the reason to create _EIP155 is because now we have 3 types (all using RLP encoding), so old _RLP is not correct now

"04830579b50e01602c2015c24e72fbc48bca1cca1e601b119ca73abe2e0b5bd61fcb7874567e091030d6b644f927445d80e00b3f9ca0c566c21c30615e94c343da",
"8d38efe45794d7fceea10b2262c23c12245959db",
},
{
Copy link
Member

Choose a reason for hiding this comment

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

what error if integrated with `TestRlpDecodeVerify``

Copy link
Member Author

@dustinxie dustinxie Nov 15, 2023

Choose a reason for hiding this comment

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

the error is crypto.ErrInvalidKey, see func TestBackwardComp(t *testing.T) below (I've added a comment)

Copy link
Member

Choose a reason for hiding this comment

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

ok, could testdata be shared in Var ?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated in latest commit

@@ -1589,6 +1591,11 @@ func (core *coreService) ActionsInActPool(actHashes []string) ([]action.SealedEn
return ret, nil
}

// Genesis returns the genesis of the chain
func (core *coreService) Genesis() genesis.Genesis {
return core.bc.Genesis()
Copy link
Member

Choose a reason for hiding this comment

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

coreService is the adapter for other services, where Genesis() shouldn't be exposed and tx decoding should be done

Copy link
Member Author

@dustinxie dustinxie Nov 15, 2023

Choose a reason for hiding this comment

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

tx decoding is for web3 only, gRPC won't use it, so I think tx decoding should be in web3, not coreService

Copy link
Member

Choose a reason for hiding this comment

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

there are some grpc-only interfaces in coreservice as well

Copy link
Member Author

@dustinxie dustinxie Nov 15, 2023

Choose a reason for hiding this comment

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

ok, but that is not the reason to add another one ... here are my 3 thoughts when adding this to coreService:

  1. coreService already has existing similar funcs like ChainID(), EvmNetworkID(), TipHeight(), these actually all belong to the Blockchain() interface, so I don't see why cannot add a new Genesis(). We should have another PR to add a Blockchain() funcs to deprecate these
  2. like you said, we can add a DecodeTx() funcs instead of Genesis(), but given the facts in 1, doing so does not make our programming better IMO. The DecodeTx() is just a wrapper of some calls, it's not simpler or better readable than Genesis()
  3. finally, after the hard-fork, we will switch to use the new code path, this Genesis() func won't be needed anymore, it will be deleted at that time

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dustinxie dustinxie merged commit 0e50cc2 into iotexproject:master Nov 15, 2023
3 checks passed
@dustinxie dustinxie mentioned this pull request Dec 7, 2023
2 tasks
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.

3 participants