chore(fix): Hitting unhealthy node 10 times

[ivaylogarnev-limechain]

Description:
This PR refactors and fixes the unhealthy node error handling logic in Executable.js. Additionally, it introduces a few unit tests to confirm this.

Related issue(s):
#2804

Checklist

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

[0xivanov]

some notes:

1. Before always advance'd (before the if (!node.isHealthy())), now we have 2 advance calls, is this necessary?
2. Around line 740 in executable.js, we have client._network.increaseBackoff(node); which removes the node from the healthy nodes list and sets new value for _readmitTime. This logic works as expected, right?

[ivaylogarnev-limechain]

1. Both advances handle different scenarios in the retry/rotation logic.

   1.1 The first advance() inside the health check is specifically for when a node is unhealthy.

   1.2 The second advance() is part of the normal node rotation logic when trying different nodes for retries.
   (tested in AccountInfoMocking.js - should retry on INTERNAL and retry multiple nodes)

2. Yes, it still reaches that point because this code accounts for the scenario where the node is initially healthy but throws an error after making a request. If the error is a GrpcService or HttpError, the increaseBackOff() method is triggered, as expected.

[0xivanov]

About the 1st one - this means we advance in both cases - can we have only 1 advance above if (!node.isHealthy()) on line 644?

[ivaylogarnev-limechain]

The issue is that with a single advance before the health check, we're effectively skipping the health check of the first node and this would break the node health checking functionality as demonstrated by the failing test "should skip unhealthy node and execute with healthy node".

[0xivanov]

I see.

[0xivanov]

We have it("should retry on UNAVAILABLE", async function () { , what is the behaviour there with more than 1 node?

[ivaylogarnev-limechain]

Test added that covers this case.

[ivaylonikolov7]

Did you manage to test the new functionality on testnet where we actually have unhealthy nodes? I know it's hard to make integration tests for this functionality because you can't make localnode unhealthy easily and then you are going to need a second account that will execute the transaction on?

[ivaylogarnev-limechain]

Did you manage to test the new functionality on testnet where we actually have unhealthy nodes? I know it's hard to make integration tests for this functionality because you can't make localnode unhealthy easily and then you are going to need a second account that will execute the transaction on?

This functionality is being tested in AccountBalanceIntegrationTest.js under the test case "can connect to testnet with TLS."

Currently, node 5 on the testnet is down, yet the tests still pass. If you explicitly hardcode the nodeAccountId to only use node 5 with:

.setNodeAccountIds([new AccountId(5)])

it will throw the error:
"Network connectivity issue: All nodes are unhealthy. Original node list: 0.0.5