hashgraph / hedera-smart-contracts

Contains Hedera Smart Contract Service supporting files
Apache License 2.0
38 stars 53 forks source link

feat: account nonce discrepancies tests #654

Closed natanasow closed 8 months ago

natanasow commented 8 months ago

Description:

There are no e2e tests for checks that are happening between the ingestion of the transaction and its evm execution.

Add e2e tests based on these in services and on the current test plan.

Related issue(s):

Fixes #653

Follow up issue:

Notes for reviewer:

Checklist

natanasow commented 8 months ago

A draft PR for nonce discrepancies tests. Several PRs need to be merged and released before we can run these tests in the CI. Locally I've tested it against a custom build of services (PR#11045) and mirror node importer (PR#7617) and it's all green which is such a relief right now :four_leaf_clover:.

github-actions[bot] commented 8 months ago

Test Results

242 tests  ±0   236 :heavy_check_mark: ±0   7m 33s :stopwatch: +9s   72 suites ±0       6 :zzz: ±0    14 files   ±0       0 :x: ±0 

Results for commit 1255097c. ± Comparison against base commit 8cabc7fe.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both. ``` should NOT be able to aggregate 150 calls to processLongInputTx ‑ Multicall Test Suite payable calls with large input should NOT be able to aggregate 150 calls to processLongInputTx ``` ``` should NOT be able to aggregate 200 calls to processLongInputTx ‑ Multicall Test Suite payable calls with large input should NOT be able to aggregate 200 calls to processLongInputTx ```

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

quiet-node commented 8 months ago

@natanasow I attempted to run the test locally using the latest version, local-node@v2.18.0. Unfortunately, all the units failed as null values were compared to the expected ones. After tracing down the issue, I identified that the error is originating from test/hts-precompile/utils.js:getAccountId().

Specifically, within the getAccountId() function, the error is triggered by the line await query.execute(client), resulting in the following error:

TypeError: Cannot read properties of undefined (reading 'accountId')

Any thoughts on what I might have missed?

natanasow commented 8 months ago

@natanasow I attempted to run the test locally using the latest version, local-node@v2.18.0. Unfortunately, all the units failed as null values were compared to the expected ones. After tracing down the issue, I identified that the error is originating from test/hts-precompile/utils.js:getAccountId().

Specifically, within the getAccountId() function, the error is triggered by the line await query.execute(client), resulting in the following error:

TypeError: Cannot read properties of undefined (reading 'accountId')

Any thoughts on what I might have missed?

@quiet-node Just pushed some fixes and added a new pipeline for these tests. Could you try again locally, and keep in mind that if you're using the latest local node, some tests will fail because there are still no official images for HIP-844.

quiet-node commented 8 months ago

@natanasow I attempted to run the test locally using the latest version, local-node@v2.18.0. Unfortunately, all the units failed as null values were compared to the expected ones. After tracing down the issue, I identified that the error is originating from test/hts-precompile/utils.js:getAccountId(). Specifically, within the getAccountId() function, the error is triggered by the line await query.execute(client), resulting in the following error:

TypeError: Cannot read properties of undefined (reading 'accountId')

Any thoughts on what I might have missed?

@quiet-node Just pushed some fixes and added a new pipeline for these tests. Could you try again locally, and keep in mind that if you're using the latest local node, some tests will fail because there are still no official images for HIP-844.

Thanks for the update! I have tried it out locally and looks like the 2nd and the 4th units failed.

 Discrepancies - Nonce Test Suite
    ✔ should not update nonce when intrinsic gas handler check failed (65ms)
    1) should not update nonce when offered gas price and allowance are zero handler check failed
    ✔ should not update nonce when offered gas price is less than current and sender does not have enough balance handler check failed (2062ms)
    2) should not update nonce when offered gas price is less than current and gas allowance is less than remaining fee handler check failed
    ✔ should not update nonce when offered gas price is bigger than current and sender does not have enough balance handler check failed (1712ms)
    ✔ should not update nonce  when sender does not have enough balance handler check failed (1397ms)
    ✔ should update nonce after evm reversion due contract logic (1574ms)
    ✔ should update nonce after evm reversion due insufficient gas (1324ms)
    ✔ should update nonce after evm reversion due insufficient transfer amount (1077ms)
    ✔ should update nonce after evm reversion due sending value to ethereum precompile 0x2 (1572ms)
    ✔ should update nonce after evm reversion due sending value to hedera precompile0 x167 (1324ms)
    ✔ should update nonce after successful internal call (1070ms)
    ✔ should update nonce after successful internal transfer (2935ms)
    ✔ should update nonce after successful internal contract deployment (1323ms)

  12 passing (23s)
  2 failing

  1) Discrepancies - Nonce Test Suite
       should not update nonce when offered gas price and allowance are zero handler check failed:

      AssertionError: expected 2 to equal 3
      + expected - actual

      -2
      +3

      at expectNonIncrementedNonce (test/discrepancies/Nonce.js:90:36)
      at Context.<anonymous> (test/discrepancies/Nonce.js:135:5)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  2) Discrepancies - Nonce Test Suite
       should not update nonce when offered gas price is less than current and gas allowance is less than remaining fee handler check failed:

      AssertionError: expected 4 to equal 5
      + expected - actual

      -4
      +5

      at expectNonIncrementedNonce (test/discrepancies/Nonce.js:90:36)
      at Context.<anonymous> (test/discrepancies/Nonce.js:182:5)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

Just like in the CI.

So as you said some tests will fail because there are still no official images for HIP-844, I'm not sure if we should approve failing tests or we should wait for the official images for HIP-844.

@Nana-EC thoughts?

natanasow commented 8 months ago

@natanasow I attempted to run the test locally using the latest version, local-node@v2.18.0. Unfortunately, all the units failed as null values were compared to the expected ones. After tracing down the issue, I identified that the error is originating from test/hts-precompile/utils.js:getAccountId(). Specifically, within the getAccountId() function, the error is triggered by the line await query.execute(client), resulting in the following error:

TypeError: Cannot read properties of undefined (reading 'accountId')

Any thoughts on what I might have missed?

@quiet-node Just pushed some fixes and added a new pipeline for these tests. Could you try again locally, and keep in mind that if you're using the latest local node, some tests will fail because there are still no official images for HIP-844.

Thanks for the update! I have tried it out locally and looks like the 2nd and the 4th units failed.

 Discrepancies - Nonce Test Suite
    ✔ should not update nonce when intrinsic gas handler check failed (65ms)
    1) should not update nonce when offered gas price and allowance are zero handler check failed
    ✔ should not update nonce when offered gas price is less than current and sender does not have enough balance handler check failed (2062ms)
    2) should not update nonce when offered gas price is less than current and gas allowance is less than remaining fee handler check failed
    ✔ should not update nonce when offered gas price is bigger than current and sender does not have enough balance handler check failed (1712ms)
    ✔ should not update nonce  when sender does not have enough balance handler check failed (1397ms)
    ✔ should update nonce after evm reversion due contract logic (1574ms)
    ✔ should update nonce after evm reversion due insufficient gas (1324ms)
    ✔ should update nonce after evm reversion due insufficient transfer amount (1077ms)
    ✔ should update nonce after evm reversion due sending value to ethereum precompile 0x2 (1572ms)
    ✔ should update nonce after evm reversion due sending value to hedera precompile0 x167 (1324ms)
    ✔ should update nonce after successful internal call (1070ms)
    ✔ should update nonce after successful internal transfer (2935ms)
    ✔ should update nonce after successful internal contract deployment (1323ms)

  12 passing (23s)
  2 failing

  1) Discrepancies - Nonce Test Suite
       should not update nonce when offered gas price and allowance are zero handler check failed:

      AssertionError: expected 2 to equal 3
      + expected - actual

      -2
      +3

      at expectNonIncrementedNonce (test/discrepancies/Nonce.js:90:36)
      at Context.<anonymous> (test/discrepancies/Nonce.js:135:5)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  2) Discrepancies - Nonce Test Suite
       should not update nonce when offered gas price is less than current and gas allowance is less than remaining fee handler check failed:

      AssertionError: expected 4 to equal 5
      + expected - actual

      -4
      +5

      at expectNonIncrementedNonce (test/discrepancies/Nonce.js:90:36)
      at Context.<anonymous> (test/discrepancies/Nonce.js:182:5)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

Just like in the CI.

So as you said some tests will fail because there are still no official images for HIP-844, I'm not sure if we should approve failing tests or we should wait for the official images for HIP-844.

@Nana-EC thoughts?

IMO, we should NOT merge this into develop till official images of HIP-844 are released. Once they are released, we should update the local node first and release a new version of it with the updated images. After that, I have to circle back here and bump the local node version in this repo. With the newest version, which must include HIP-844, these tests have to succeed and we will be free to merge it.