fix: enhanced reliability of eth RPC methods with null checks and retry mechanisms #3349

quiet-node · 2024-12-20T22:00:12Z

Description:
Numerous requests for eth_getBlockByNumber and eth_getBlockByHash have been reported as failing with errors such as Cannot read properties of null and Received an invalid integer type: null.

Additionally, eth_getTransactionReceipt is also reported to be failing with Error invoking RPC: Invalid parameter: hashOrNumber.

These issues stem from regressions in the getBlock() method, which lacks adequate null checks in certain areas. This pull request addresses the problem by implementing proper null checks and enhancing fallback mechanisms and error handling.

Partially, the results of contracts and log responses from the Mirror Node occasionally contain undefined fields, such as transaction_index, block_hash, block_number, and log_index. This issue may arise because the Mirror Node database does not have sufficient time to save all the information before it is fetched. This PR adds a retry mechanism to the fetching methods that checks for these cases, waits for a specified duration, and then retries the fetch operation.

Related issue(s):

Fixes #3345
Fixes #3351

Notes for reviewer:

Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

github-actions · 2024-12-20T22:04:36Z

Test Results

19 files - 4 261 suites - 88 33m 6s ⏱️ - 16m 6s
613 tests - 4 608 ✅ + 5 4 💤 ±0 1 ❌ - 9
715 runs - 194 710 ✅ - 179 4 💤 - 3 1 ❌ - 12

For more details on these failures, see this check.

Results for commit 8687322. ± Comparison against base commit 13d09e9.

This pull request removes 5 and adds 1 tests. Note that renamed tests count towards both.

"before all" hook for "emits an approval event" ‑ RPC Server Acceptance Tests Acceptance tests @erc20 Acceptance Tests HTS token should behave like erc20 transfer from when the token owner is not the zero address when the recipient is not the zero address when the spender has enough allowance "before all" hook for "emits an approval event"
"before all" hook for "reverts" ‑ RPC Server Acceptance Tests Acceptance tests @erc20 Acceptance Tests HTS token should behave like erc20 transfer from when the token owner is not the zero address when the recipient is not the zero address when the spender does not have enough allowance when the token owner has enough balance "before all" hook for "reverts"
"before all" hook in "Debug API Test Suite" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests Debug API Test Suite "before all" hook in "Debug API Test Suite"
"before each" hook for "reverts" ‑ RPC Server Acceptance Tests Acceptance tests @erc20 Acceptance Tests HTS token should behave like erc20 transfer from when the token owner is not the zero address when the recipient is not the zero address when the spender does not have enough allowance "before each" hook for "reverts"
"before each" hook for "should execute "eth_getStorageAt" request to get current state changes" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before each" hook for "should execute "eth_getStorageAt" request to get current state changes"

"after all" hook in "@web-socket-batch-1 eth_getBlockByNumber" ‑ RPC Server Acceptance Tests Acceptance tests @web-socket-batch-1 eth_getBlockByNumber "after all" hook in "@web-socket-batch-1 eth_getBlockByNumber"

♻️ This comment has been updated with latest results.

Nana-EC

Good catch.
Some considerations

packages/relay/src/lib/clients/mirrorNodeClient.ts

Nana-EC · 2024-12-20T23:58:16Z

packages/relay/src/lib/services/ethService/ethCommonService/index.ts

@@ -334,7 +335,7 @@ export class CommonService implements ICommonService {
        new Log({
          address: log.address,
          blockHash: toHash32(log.block_hash),
-          blockNumber: numberTo0x(log.block_number),
+          blockNumber: nullableNumberTo0x(log.block_number),


Q: Why would a block number be null?

Yes, this is one of the strange cases where block_number, transaction_index, or blockHash are falsy, leading to 500 errors. The retry mechanism should address the issue. Should I revert this change and throw a more informative error message even if those fields remain falsy after the retry? Hmm maybe that would be better so the clients can resubmit the requests themselves other than having null in the response. Wdyt?

Hmmm, if a retry is ensured to resolve the issue then maybe we can manage one iteration in the short term.
We should open up an issue in the mirror node repo to investigate this issue so this can be resolved.

packages/relay/src/receiptsRootUtils.ts

acuarica · 2024-12-21T00:27:53Z

packages/relay/src/lib/clients/mirrorNodeClient.ts

@@ -620,7 +620,10 @@ export class MirrorNodeClient {
      requestDetails,
    );

-    await this.cacheService.set(cachedLabel, block, MirrorNodeClient.GET_BLOCK_ENDPOINT, requestDetails);
+    if (block) {


not sure if I understand correctly, but the errors described in the issue were because of caching null blocks?

This is an issue I noticed and an attempt securing the code. While it may or may not be directly related to the problem, we should ensure the block is not null before storing it in the cache. Otherwise, we risk repeatedly retrieving null from the cache, which likely has a TTL.

packages/relay/src/receiptsRootUtils.ts

packages/relay/src/lib/clients/mirrorNodeClient.ts

packages/relay/src/lib/eth.ts

Nana-EC · 2024-12-21T03:20:48Z

packages/relay/src/lib/eth.ts

@@ -2531,11 +2544,12 @@ export class EthImpl implements Eth {
    if (blockResponse == null) return null;
    const timestampRange = blockResponse.timestamp;
    const timestampRangeParams = [`gte:${timestampRange.from}`, `lte:${timestampRange.to}`];
-    const contractResults = await this.mirrorNodeClient.getContractResults(
+    const contractResults = await this.mirrorNodeClient.getContractResultWithRetry(


Are we just being cautious or does this case actually need to have retry support?

The contractResults here was a contributing factor to the 500 errors. In the rare cases mentioned, contractResults contains null fields. When passed to buildReceiptRootHashes, the function does not handle these null values correctly, resulting in 500 errors. Therefore, this scenario requires a retry if such a rare case occurs.

Since these cases occur only on the mainnet and at the time of the transaction, I plan to release these updates that include additional debugging information for improved analysis. If the issues persist, it will be easier to pinpoint the root causes and escalate them to the mainnet team more effectively.

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

…Contract MN methods Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

quiet-node · 2024-12-23T15:29:33Z

packages/relay/src/lib/clients/sdkClient.ts

+      this.logger.warn(
+        e,
+        `${requestDetails.formattedRequestId} Transaction failed while executing transaction via the SDK: transactionId=${transaction.transactionId}, callerName=${callerName}, txConstructorName=${txConstructorName}`,
+      );
+


Note for Rereviewers:
This is an effort to enhance debugging.

quiet-node · 2024-12-23T15:32:19Z

packages/server/tests/acceptance/rpc_batch1.spec.ts

+          await relay.pollForValidTransactionReceipt(transactionHash);
+


Note for Rereviewers:
This is an effort to stablize the flaky test.

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> Revert "fix: reverted licenses" This reverts commit d3c860a. Reapply "fix: reverted licenses" This reverts commit 50a9acd5031490f3f805042a70bfdae7c589146d.

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

sonarqubecloud · 2024-12-23T18:20:28Z

Quality Gate failed

Failed conditions
14.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

codecov · 2024-12-24T02:22:14Z

Codecov Report

Attention: Patch coverage is 82.35294% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.99%. Comparing base (13d09e9) to head (8687322).

Files with missing lines	Patch %	Lines
packages/relay/src/lib/clients/mirrorNodeClient.ts	79.16%	2 Missing and 3 partials ⚠️
.../lib/services/ethService/ethCommonService/index.ts	76.92%	2 Missing and 1 partial ⚠️
packages/relay/src/receiptsRootUtils.ts	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3349      +/-   ##
==========================================
- Coverage   85.04%   84.99%   -0.06%     
==========================================
  Files          65       69       +4     
  Lines        4508     4717     +209     
  Branches     1023     1061      +38     
==========================================
+ Hits         3834     4009     +175     
- Misses        330      394      +64     
+ Partials      344      314      -30

Flag	Coverage Δ
config-service	`98.14% <ø> (ø)`
relay	`79.53% <76.47%> (-0.20%)`	⬇️
server	`83.28% <ø> (?)`
ws-server	`36.66% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
packages/relay/src/lib/clients/sdkClient.ts	`69.52% <100.00%> (+1.85%)`	⬆️
packages/relay/src/lib/eth.ts	`86.14% <100.00%> (ø)`
...kages/relay/src/lib/services/debugService/index.ts	`85.71% <100.00%> (ø)`
packages/relay/src/receiptsRootUtils.ts	`96.07% <87.50%> (-1.80%)`	⬇️
.../lib/services/ethService/ethCommonService/index.ts	`91.30% <76.92%> (-2.59%)`	⬇️
packages/relay/src/lib/clients/mirrorNodeClient.ts	`89.05% <79.16%> (-0.70%)`	⬇️

... and 13 files with indirect coverage changes

quiet-node added the bug Something isn't working label Dec 20, 2024

quiet-node added this to the 0.62.0 milestone Dec 20, 2024

quiet-node self-assigned this Dec 20, 2024

quiet-node requested review from a team as code owners December 20, 2024 22:00

quiet-node linked an issue Dec 20, 2024 that may be closed by this pull request

[Failed-Request-Alert-Tuning] Investigate and resolve the bug causing the Received an invalid integer type: null error in EthImpl.getBlock() function #3346

Closed

quiet-node requested a review from stoyanov-st December 20, 2024 22:00

quiet-node modified the milestones: 0.62.0, 0.63.0 Dec 20, 2024

quiet-node force-pushed the 3346-failed-request-alert-tuning-investigate-and-resolve-the-bug-causing-the-received-an-invalid-integer-type-null-error-in-ethimplgetblock-function branch from 675eb4a to d265305 Compare December 20, 2024 23:07

Nana-EC reviewed Dec 21, 2024

View reviewed changes

acuarica reviewed Dec 21, 2024

View reviewed changes

quiet-node force-pushed the 3346-failed-request-alert-tuning-investigate-and-resolve-the-bug-causing-the-received-an-invalid-integer-type-null-error-in-ethimplgetblock-function branch from 936f1e9 to b959fc7 Compare December 21, 2024 02:20

Nana-EC reviewed Dec 21, 2024

View reviewed changes

quiet-node force-pushed the 3346-failed-request-alert-tuning-investigate-and-resolve-the-bug-causing-the-received-an-invalid-integer-type-null-error-in-ethimplgetblock-function branch from 19d9838 to e1c8fd0 Compare December 21, 2024 23:14

quiet-node marked this pull request as draft December 21, 2024 23:14

quiet-node force-pushed the 3346-failed-request-alert-tuning-investigate-and-resolve-the-bug-causing-the-received-an-invalid-integer-type-null-error-in-ethimplgetblock-function branch 7 times, most recently from 3b96fc4 to aad493c Compare December 22, 2024 20:12

quiet-node added 3 commits December 22, 2024 14:50

fix: fixed flaky precheck test

152b18c

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

fix: handle log null entities more gracefully in getBlock()

bc26b45

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

fix: added null check to root hash builder

548cdeb

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

quiet-node force-pushed the 3346-failed-request-alert-tuning-investigate-and-resolve-the-bug-causing-the-received-an-invalid-integer-type-null-error-in-ethimplgetblock-function branch 2 times, most recently from a5a0e1f to 78fa644 Compare December 22, 2024 21:07

fix: modified getContractResultWithRetry() to accept more general get…

d97250d

…Contract MN methods Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

quiet-node force-pushed the 3346-failed-request-alert-tuning-investigate-and-resolve-the-bug-causing-the-received-an-invalid-integer-type-null-error-in-ethimplgetblock-function branch 7 times, most recently from bac3c74 to d7f5829 Compare December 23, 2024 15:22

quiet-node commented Dec 23, 2024

View reviewed changes

quiet-node marked this pull request as ready for review December 23, 2024 16:19

quiet-node mentioned this pull request Dec 23, 2024

[Failed-Request-Alert-Tuning] eth_getTransactionReceipt method returns too many 500 errors #3351

Open

quiet-node added 4 commits December 23, 2024 12:17

fix: reused getContractResultWithRetry for getContractResults

ae23d06

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

fix: added getContractResultsLogsWithRetry()

a99288f

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

fix: reverted licenses

a410b15

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> Revert "fix: reverted licenses" This reverts commit d3c860a. Reapply "fix: reverted licenses" This reverts commit 50a9acd5031490f3f805042a70bfdae7c589146d.

fix: checked empty blockHash for getHistoricalBlockResponse()

8687322

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

quiet-node force-pushed the 3346-failed-request-alert-tuning-investigate-and-resolve-the-bug-causing-the-received-an-invalid-integer-type-null-error-in-ethimplgetblock-function branch from d6756f0 to 8687322 Compare December 23, 2024 18:19

quiet-node changed the title ~~fix: added proper null checks for getBlock() method~~ fix: enhanced reliability of eth RPC methods with null checks and retry mechanisms Dec 23, 2024

quiet-node modified the milestones: 0.63.0, 0.63.1 Dec 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enhanced reliability of eth RPC methods with null checks and retry mechanisms #3349

fix: enhanced reliability of eth RPC methods with null checks and retry mechanisms #3349

quiet-node commented Dec 20, 2024 •

edited

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading

Nana-EC left a comment

Nana-EC Dec 20, 2024

quiet-node Dec 21, 2024

Nana-EC Dec 23, 2024

acuarica Dec 21, 2024

quiet-node Dec 21, 2024

Nana-EC Dec 21, 2024

quiet-node Dec 21, 2024

quiet-node Dec 21, 2024

quiet-node Dec 23, 2024

quiet-node Dec 23, 2024

sonarqubecloud bot commented Dec 23, 2024

codecov bot commented Dec 24, 2024

		await relay.pollForValidTransactionReceipt(transactionHash);

fix: enhanced reliability of eth RPC methods with null checks and retry mechanisms #3349

Are you sure you want to change the base?

fix: enhanced reliability of eth RPC methods with null checks and retry mechanisms #3349

Conversation

quiet-node commented Dec 20, 2024 • edited Loading

github-actions bot commented Dec 20, 2024 • edited Loading

Test Results

Nana-EC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 23, 2024

Quality Gate failed

codecov bot commented Dec 24, 2024

Codecov Report

quiet-node commented Dec 20, 2024 •

edited

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading