hedera-services icon indicating copy to clipboard operation
hedera-services copied to clipboard

fix: bugs found in Hedera > Ethereum conversion framework

Open vtronkov opened this issue 11 months ago • 11 comments

Description: Running all the ConcurrentSuites.ethereumSuites tests and fix all the issues related to the framework the converts Hedera to Ethereum contract calls and contract creates. The bugs found on service level were fixed in https://github.com/hashgraph/hedera-services/pull/12834

Related issue(s):

Fixes #11730 Fixes #12364

Notes for reviewer:

Solved problems

  • Update contract bytecode file, if we have constructor args:
    • When we use HAPI transactions to create a contract that needs constructor arguments, ContractCreateTransactionBody has separate fields for initcodeSource/fileID and constructorParameters. So if args are needed the EVM payload is constructed (args are appended to the init code) in com.hedera.node.app.service.contract.impl.infra.HevmTransactionFactory#initcodeFor But when we use Ethereum transaction we can pass only CallData, which is either the bytecode or the fileID. There is no field in the transaction body to map the arguments. This is why when we convert HAPI to Eht creation and detect arguments, we perform additional file update and append the arguments to the bytecode.
  • Try to convert args/params addresses to EVM addresses:
    • When we try to access any mutable accounts from the EVM frame, we can do it only by EVM address (priority address). So when we convert to ethereum call/create, we also try to convertny params/args that are representing addresses from a long zero address to an EVM address, when it is possible. In order to achieve this, when create is done, we added a getContractInfo query and saved the EVM address of the contract to the spec registry. So when we map calls with params/args we iterate over them and check if they are addresses and if we have already saved the corresponding EVM one.

Refused conversion

  • Skip ethereum contract creation conversion (assert admin key/renew account/memo/staking fields):
    • EthereumTransactionBody doesn’t support these fields, so assertions against them will fail. 
  • Refuse eth conversion because we can't set invalid bytecode to callData in ethereum transaction:
    • When we perform HAPI create transactions, byte codes are uploaded as a file in separate transactions, and parsed as call data (payload) on the node itself. So any invalid byte code will result in some kind of error status. While in ethereum creates the byte code, it must be parsed as call data on the client side, and if it is invalid, we fail to send the transaction. 
  • Refusing ethereum create conversion, because we get INVALID_SIGNATURE upon tokenAssociate, since we have CONTRACT_ID key:
    • The default key validator doesn't allow as to sing tokenAssociate transactions with simple Contrac_ID keys.
  • Refuse conversion because of MODIFYING_IMMUTABLE_CONTRAC:
    • Since it is impossible to set an admin key, or any other special keys by ethereum create, our contracts become immutable. Any test that tries to modify it will fail.
  • Refuse because of __AddressBook.sol:AddressBook___________ placeholder
    • it would be overkill to support that for a single test

Checklist

  • [x] Documented (Code comments, README, etc.)
  • [x] Tested (unit, integration, etc.)

vtronkov avatar Mar 14 '24 03:03 vtronkov

Node: HAPI Test (Restart) Results

2 tests   2 :white_check_mark:  5m 19s :stopwatch: 2 suites  0 :zzz: 2 files    0 :x:

Results for commit 5453a170.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Mar 18 '24 11:03 github-actions[bot]

Node: HAPI Test (Node Death Reconnect) Results

2 tests   2 :white_check_mark:  6m 47s :stopwatch: 2 suites  0 :zzz: 2 files    0 :x:

Results for commit 5453a170.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Mar 18 '24 11:03 github-actions[bot]

Node: HAPI Test (Token) Results

235 tests   234 :white_check_mark:  23m 15s :stopwatch:  17 suites    1 :zzz:  17 files      0 :x:

Results for commit 5453a170.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Mar 18 '24 11:03 github-actions[bot]

Node: HAPI Test (Crypto) Results

335 tests   335 :white_check_mark:  38m 1s :stopwatch:  25 suites    0 :zzz:  25 files      0 :x:

Results for commit 5453a170.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Mar 18 '24 11:03 github-actions[bot]

Node: HAPI Test (Misc) Results

459 tests   449 :white_check_mark:  38m 8s :stopwatch:  77 suites   10 :zzz:  77 files      0 :x:

Results for commit 5453a170.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Mar 18 '24 11:03 github-actions[bot]

Node: HAPI Test (Time Consuming) Results

21 tests   21 :white_check_mark:  54m 7s :stopwatch:  3 suites   0 :zzz:  3 files     0 :x:

Results for commit 5453a170.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Mar 18 '24 12:03 github-actions[bot]

Node: HAPI Test (Smart Contract) Results

589 tests   589 :white_check_mark:  1h 16m 36s :stopwatch:  63 suites    0 :zzz:  63 files      0 :x:

Results for commit 5453a170.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Mar 18 '24 12:03 github-actions[bot]

Node: Unit Test Results

  2 278 files  + 2    2 278 suites  +2   3h 37m 19s :stopwatch: + 4m 1s 118 752 tests +18  118 685 :white_check_mark: +18  67 :zzz: ±0  0 :x: ±0  127 237 runs  +18  127 170 :white_check_mark: +18  67 :zzz: ±0  0 :x: ±0 

Results for commit 5453a170. ± Comparison against base commit 4f8f9818.

This pull request removes 3998 and adds 3779 tests. Note that renamed tests count towards both.

  
             IssuerDN: CN=s-aaaa
            SubjectDN: CN=s-aaaa
           Final Date: Fri Jan 01 00:00:00 UTC 2100
           Public Key: RSA Public Key [2e:28:bc:1e:d3:83:25:92:8e:cb:98:b1:b6:84:06:9c:d5:d8:14:d5],[56:66:d1:a4]
           Start Date: Sat Jan 01 00:00:00 UTC 2000
         SerialNumber: 12482092706667292405
        modulus: c1a0ff5d2372b53d12d12bb87dd03f5e…
        modulus: c1a0ff5d2372b53d12d12bb87dd03f5…
…
com.hedera.node.app.grpc.impl.netty.GrpcServiceBuilderTest ‑ [4] 

com.hedera.node.app.grpc.impl.netty.GrpcServiceBuilderTest ‑ [6] 

com.hedera.node.app.grpc.impl.netty.GrpcServiceBuilderTest ‑ [7]   
  
com.hedera.node.app.service.contract.impl.test.exec.TransactionModuleTest ‑ providesCorrespondingKeyForAvailableEthTxData()
com.hedera.node.app.service.contract.impl.test.exec.TransactionModuleTest ‑ providesNullSenderEcdsaKeyWithUnavailableEthTxData()
com.hedera.node.app.service.contract.impl.test.exec.TransactionModuleTest ‑ providesNullSenderEcdsaKeyWithoutHydratedEthTxData()
com.hedera.node.app.service.contract.impl.test.exec.scope.ActiveContractVerificationStrategyTest ‑ signatureTestApprovesEthSenderKeyWhenDelegating()
com.hedera.node.app.service.contract.impl.test.exec.scope.ActiveContractVerificationStrategyTest ‑ signatureTestUsesContextVerificationWhenNotEthSenderKey()
com.hedera.node.app.service.contract.impl.test.exec.scope.EitherOrVerificationStrategyTest ‑ delegatesIfPossibleAndNotAlreadyValid()
com.hedera.node.app.service.contract.impl.test.exec.scope.EitherOrVerificationStrategyTest ‑ invalidIfNeitherStrategyValid()
…

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Mar 18 '24 12:03 github-actions[bot]

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 61.65%. Comparing base (fbcf796) to head (1da9485). Report is 373 commits behind head on develop.

:exclamation: Current head 1da9485 differs from pull request most recent head 5453a17. Consider uploading reports for the commit 5453a17 to get more accurate results

Files Patch % Lines
...hedera/node/app/hapi/utils/ethereum/EthTxData.java 33.33% 1 Missing and 1 partial :warning:
...contract/impl/infra/EthereumCallDataHydration.java 60.00% 1 Missing and 1 partial :warning:
...ce/contract/impl/infra/HevmTransactionFactory.java 0.00% 0 Missing and 1 partial :warning:
Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #12118      +/-   ##
=============================================
+ Coverage      61.60%   61.65%   +0.05%     
- Complexity     30675    30751      +76     
=============================================
  Files           3400     3401       +1     
  Lines         139317   139389      +72     
  Branches       14567    14586      +19     
=============================================
+ Hits           85822    85944     +122     
+ Misses         49748    49707      -41     
+ Partials        3747     3738       -9     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Mar 18 '24 12:03 codecov-commenter

Compile error on this branch?

tinker-michaelj avatar Apr 22 '24 13:04 tinker-michaelj

Compile error on this branch?

@tinker-michaelj There was this compile error in dev because of a new ResponseCodeEnum type and the branch wasn't up-to-date. I updated the branch and now it should be okay.

vtronkov avatar Apr 22 '24 15:04 vtronkov