diff --git a/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts b/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts index 9655aba5fc6b..4b31fbe3f668 100644 --- a/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts +++ b/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts @@ -162,7 +162,7 @@ export class JsonRpcHttpClient implements IJsonRpcHttpClient { return this.fetchJson({jsonrpc: "2.0", id: this.id++, ...payload}, opts); }, { - retries: opts?.retryAttempts ?? this.opts?.retryAttempts ?? 1, + retries: opts?.retryAttempts ?? this.opts?.retryAttempts ?? 0, retryDelay: opts?.retryDelay ?? this.opts?.retryDelay ?? 0, shouldRetry: opts?.shouldRetry, signal: this.opts?.signal, diff --git a/packages/beacon-node/src/execution/builder/http.ts b/packages/beacon-node/src/execution/builder/http.ts index b4013c384f81..ca37bfa4072c 100644 --- a/packages/beacon-node/src/execution/builder/http.ts +++ b/packages/beacon-node/src/execution/builder/http.ts @@ -118,7 +118,7 @@ export class ExecutionBuilderHttp implements IExecutionBuilder { async submitBlindedBlock( signedBlindedBlock: allForks.SignedBlindedBeaconBlock ): Promise { - const res = await this.api.submitBlindedBlock(signedBlindedBlock, {retryAttempts: 3}); + const res = await this.api.submitBlindedBlock(signedBlindedBlock, {retryAttempts: 2}); ApiError.assert(res, "execution.builder.submitBlindedBlock"); const {data} = res.response; diff --git a/packages/beacon-node/src/execution/engine/http.ts b/packages/beacon-node/src/execution/engine/http.ts index 91ceabaf2770..b609defb442c 100644 --- a/packages/beacon-node/src/execution/engine/http.ts +++ b/packages/beacon-node/src/execution/engine/http.ts @@ -72,7 +72,7 @@ export const defaultExecutionEngineHttpOpts: ExecutionEngineHttpOpts = { * port/url, one can override this and skip providing a jwt secret. */ urls: ["http://localhost:8551"], - retryAttempts: 3, + retryAttempts: 2, retryDelay: 2000, timeout: 12000, }; @@ -305,7 +305,7 @@ export class ExecutionEngineHttp implements IExecutionEngine { // If we are just fcUing and not asking execution for payload, retry is not required // and we can move on, as the next fcU will be issued soon on the new slot const fcUReqOpts = - payloadAttributes !== undefined ? forkchoiceUpdatedV1Opts : {...forkchoiceUpdatedV1Opts, retryAttempts: 1}; + payloadAttributes !== undefined ? forkchoiceUpdatedV1Opts : {...forkchoiceUpdatedV1Opts, retryAttempts: 0}; const request = this.rpcFetchQueue.push({ method, diff --git a/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts b/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts index cf6d769ed9a3..277073bf10a8 100644 --- a/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts +++ b/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts @@ -225,10 +225,10 @@ describe("eth1 / jsonRpcHttpClient - with retries", function () { }); it("should retry 404", async function () { - let retryCount = 0; + let requestCount = 0; const server = http.createServer((req, res) => { - retryCount++; + requestCount++; res.statusCode = 404; res.end(); }); @@ -251,14 +251,14 @@ describe("eth1 / jsonRpcHttpClient - with retries", function () { const controller = new AbortController(); const eth1JsonRpcClient = new JsonRpcHttpClient([url], {signal: controller.signal}); await expect(eth1JsonRpcClient.fetchWithRetries(payload, {retryAttempts})).rejects.toThrow("Not Found"); - expect(retryCount).toBeWithMessage(retryAttempts, "404 responses should be retried before failing"); + expect(requestCount).toBeWithMessage(retryAttempts + 1, "404 responses should be retried before failing"); }); it("should retry timeout", async function () { - let retryCount = 0; + let requestCount = 0; const server = http.createServer(async () => { - retryCount++; + requestCount++; }); await new Promise((resolve) => server.listen(port, resolve)); @@ -284,13 +284,13 @@ describe("eth1 / jsonRpcHttpClient - with retries", function () { await expect(eth1JsonRpcClient.fetchWithRetries(payload, {retryAttempts, timeout})).rejects.toThrow( "Timeout request" ); - expect(retryCount).toBeWithMessage(retryAttempts, "Timeout request should be retried before failing"); + expect(requestCount).toBeWithMessage(retryAttempts + 1, "Timeout request should be retried before failing"); }); it("should retry aborted", async function () { - let retryCount = 0; + let requestCount = 0; const server = http.createServer(() => { - retryCount++; + requestCount++; // leave the request open until timeout }); @@ -314,14 +314,14 @@ describe("eth1 / jsonRpcHttpClient - with retries", function () { setTimeout(() => controller.abort(), 50); const eth1JsonRpcClient = new JsonRpcHttpClient([url], {signal: controller.signal}); await expect(eth1JsonRpcClient.fetchWithRetries(payload, {retryAttempts, timeout})).rejects.toThrow("Aborted"); - expect(retryCount).toBeWithMessage(1, "Aborted request should be retried before failing"); + expect(requestCount).toBeWithMessage(1, "Aborted request should be retried before failing"); }); it("should not retry payload error", async function () { - let retryCount = 0; + let requestCount = 0; const server = http.createServer((req, res) => { - retryCount++; + requestCount++; res.setHeader("Content-Type", "application/json"); res.end(JSON.stringify({jsonrpc: "2.0", id: 83, error: noMethodError})); }); @@ -344,6 +344,6 @@ describe("eth1 / jsonRpcHttpClient - with retries", function () { const controller = new AbortController(); const eth1JsonRpcClient = new JsonRpcHttpClient([url], {signal: controller.signal}); await expect(eth1JsonRpcClient.fetchWithRetries(payload, {retryAttempts})).rejects.toThrow("Method not found"); - expect(retryCount).toBeWithMessage(1, "Payload error (non-network error) should not be retried"); + expect(requestCount).toBeWithMessage(1, "Payload error (non-network error) should not be retried"); }); }, {timeout: 10_000}); diff --git a/packages/utils/src/retry.ts b/packages/utils/src/retry.ts index 21d3b9a805e4..d875e81c75d9 100644 --- a/packages/utils/src/retry.ts +++ b/packages/utils/src/retry.ts @@ -32,11 +32,13 @@ export type RetryOptions = { */ export async function retry(fn: (attempt: number) => A | Promise, opts?: RetryOptions): Promise { const maxRetries = opts?.retries ?? 5; + // Number of retries + the initial attempt + const maxAttempts = maxRetries + 1; const shouldRetry = opts?.shouldRetry; const onRetry = opts?.onRetry; let lastError: Error = Error("RetryError"); - for (let i = 1; i <= maxRetries; i++) { + for (let i = 1; i <= maxAttempts; i++) { try { // If not the first attempt, invoke right before retrying if (i > 1) onRetry?.(lastError, i); @@ -44,7 +46,11 @@ export async function retry(fn: (attempt: number) => A | Promise, opts?: R return await fn(i); } catch (e) { lastError = e as Error; - if (shouldRetry && !shouldRetry(lastError)) { + + if (i === maxAttempts) { + // Reached maximum number of attempts, there's no need to check if we should retry + break; + } else if (shouldRetry && !shouldRetry(lastError)) { break; } else if (opts?.retryDelay !== undefined) { await sleep(opts?.retryDelay, opts?.signal);