From e9217168e440d23e0a920af234889f8a17f2f2fd Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Wed, 16 Jun 2021 13:37:13 +1000 Subject: [PATCH 1/2] improve 502 handling with guard node in fault in some cases --- ts/session/onions/onionPath.ts | 6 ++-- ts/session/onions/onionSend.ts | 2 +- ts/session/snode_api/onions.ts | 53 ++++++++++++++++++++++++++++------ 3 files changed, 48 insertions(+), 13 deletions(-) diff --git a/ts/session/onions/onionPath.ts b/ts/session/onions/onionPath.ts index 841ab1535..3d2a45cae 100644 --- a/ts/session/onions/onionPath.ts +++ b/ts/session/onions/onionPath.ts @@ -156,9 +156,9 @@ export async function getOnionPath(toExclude?: Snode): Promise> { /** * If we don't know which nodes is causing trouble, increment the issue with this full path. */ -export async function incrementBadPathCountOrDrop(sndeEd25519: string) { +export async function incrementBadPathCountOrDrop(snodeEd25519: string) { const pathWithSnodeIndex = onionPaths.findIndex(path => - path.some(snode => snode.pubkey_ed25519 === sndeEd25519) + path.some(snode => snode.pubkey_ed25519 === snodeEd25519) ); if (pathWithSnodeIndex === -1) { @@ -187,7 +187,7 @@ export async function incrementBadPathCountOrDrop(sndeEd25519: string) { // a guard node is dropped when the path is dropped completely (in dropPathStartingWithGuardNode) for (let index = 1; index < pathWithIssues.length; index++) { const snode = pathWithIssues[index]; - await incrementBadSnodeCountOrDrop({ snodeEd25519: snode.pubkey_ed25519 }); + await incrementBadSnodeCountOrDrop({ snodeEd25519: snode.pubkey_ed25519, guardNodeEd25519 }); } if (newPathFailureCount >= pathFailureThreshold) { diff --git a/ts/session/onions/onionSend.ts b/ts/session/onions/onionSend.ts index 650a5b07d..3af4cd57d 100644 --- a/ts/session/onions/onionSend.ts +++ b/ts/session/onions/onionSend.ts @@ -190,7 +190,7 @@ export const sendViaOnion = async ( ); } catch (e) { window?.log?.warn('sendViaOnionRetryable failed ', e); - console.warn('error to show to user', e); + // console.warn('error to show to user', e); return null; } diff --git a/ts/session/snode_api/onions.ts b/ts/session/snode_api/onions.ts index 8c2459b3f..f8c4b7124 100644 --- a/ts/session/snode_api/onions.ts +++ b/ts/session/snode_api/onions.ts @@ -211,11 +211,17 @@ function processOxenServerError(statusCode: number, body?: string) { async function process421Error( statusCode: number, body: string, + guardNodeEd25519: string, associatedWith?: string, lsrpcEd25519Key?: string ) { if (statusCode === 421) { - await handle421InvalidSwarm({ snodeEd25519: lsrpcEd25519Key, body, associatedWith }); + await handle421InvalidSwarm({ + snodeEd25519: lsrpcEd25519Key, + guardNodeEd25519, + body, + associatedWith, + }); } } @@ -228,11 +234,13 @@ async function process421Error( async function processOnionRequestErrorAtDestination({ statusCode, body, + guardNodeEd25519, destinationEd25519, associatedWith, }: { statusCode: number; body: string; + guardNodeEd25519: string; destinationEd25519?: string; associatedWith?: string; }) { @@ -242,10 +250,16 @@ async function processOnionRequestErrorAtDestination({ window?.log?.info('processOnionRequestErrorAtDestination. statusCode nok:', statusCode); process406Error(statusCode); - await process421Error(statusCode, body, associatedWith, destinationEd25519); + await process421Error(statusCode, body, guardNodeEd25519, associatedWith, destinationEd25519); processOxenServerError(statusCode, body); if (destinationEd25519) { - await processAnyOtherErrorAtDestination(statusCode, body, destinationEd25519, associatedWith); + await processAnyOtherErrorAtDestination( + statusCode, + body, + guardNodeEd25519, + destinationEd25519, + associatedWith + ); } } @@ -277,6 +291,7 @@ async function processAnyOtherErrorOnPath( if (nodeNotFound) { await exports.incrementBadSnodeCountOrDrop({ snodeEd25519: nodeNotFound, + guardNodeEd25519, associatedWith, }); @@ -291,6 +306,7 @@ async function processAnyOtherErrorOnPath( async function processAnyOtherErrorAtDestination( status: number, body: string, + guardNodeEd25519: string, destinationEd25519: string, associatedWith?: string ) { @@ -309,6 +325,7 @@ async function processAnyOtherErrorAtDestination( if (nodeNotFound) { await exports.incrementBadSnodeCountOrDrop({ snodeEd25519: destinationEd25519, + guardNodeEd25519, associatedWith, }); // if we get a nodeNotFound at the desitnation. it means the targetNode to which we made the request is not found. @@ -325,6 +342,7 @@ async function processAnyOtherErrorAtDestination( // if (nodeNotFound) { await exports.incrementBadSnodeCountOrDrop({ snodeEd25519: destinationEd25519, + guardNodeEd25519, associatedWith, }); @@ -343,7 +361,13 @@ async function processOnionRequestErrorOnPath( window?.log?.warn('errorONpath:', ciphertext); } process406Error(httpStatusCode); - await process421Error(httpStatusCode, ciphertext, associatedWith, lsrpcEd25519Key); + await process421Error( + httpStatusCode, + ciphertext, + guardNodeEd25519, + associatedWith, + lsrpcEd25519Key + ); await processAnyOtherErrorOnPath(httpStatusCode, guardNodeEd25519, ciphertext, associatedWith); } @@ -457,6 +481,7 @@ export async function processOnionResponse({ await processOnionRequestErrorAtDestination({ statusCode: status, body: jsonRes?.body, // this is really important. the `.body`. the .body should be a string. for isntance for nodeNotFound but is most likely a dict (Record)) + guardNodeEd25519: guardNode.pubkey_ed25519, destinationEd25519: lsrpcEd25519Key, associatedWith, }); @@ -503,9 +528,11 @@ function isSnodeResponse(arg: any): arg is SnodeResponse { async function handle421InvalidSwarm({ body, snodeEd25519, + guardNodeEd25519, associatedWith, }: { body: string; + guardNodeEd25519: string; snodeEd25519?: string; associatedWith?: string; }) { @@ -543,7 +570,7 @@ async function handle421InvalidSwarm({ await dropSnodeFromSwarmIfNeeded(associatedWith, snodeEd25519); } } - await exports.incrementBadSnodeCountOrDrop({ snodeEd25519, associatedWith }); + await exports.incrementBadSnodeCountOrDrop({ snodeEd25519, guardNodeEd25519, associatedWith }); // this is important we throw so another retry is made and we exit the handling of that reponse throw new pRetry.AbortError(exceptionMessage); @@ -556,17 +583,23 @@ async function handle421InvalidSwarm({ * * So after this call, if the snode keeps getting errors, we won't contact it again * - * @param snodeEd25519 the snode ed25519 which cause issues + * @param snodeEd25519 the snode ed25519 which cause issues (this might be a nodeNotFound) + * @param guardNodeEd25519 the guard node ed25519 of the current path in use. a nodeNoteFound ed25519 is not part of any path, so we fallback to this one if we need to increment the bad path count of the current path in use * @param associatedWith if set, we will drop this snode from the swarm of the pubkey too * @param isNodeNotFound if set, we will drop this snode right now as this is an invalid node for the network. */ export async function incrementBadSnodeCountOrDrop({ snodeEd25519, + guardNodeEd25519, associatedWith, }: { snodeEd25519: string; + guardNodeEd25519: string; associatedWith?: string; }) { + if (!guardNodeEd25519) { + console.warn('We need a guardNodeEd25519 at all times'); + } const oldFailureCount = snodeFailureCount[snodeEd25519] || 0; const newFailureCount = oldFailureCount + 1; snodeFailureCount[snodeEd25519] = newFailureCount; @@ -593,8 +626,10 @@ export async function incrementBadSnodeCountOrDrop({ 'dropSnodeFromPath, got error while patching up... incrementing the whole path as bad', e ); - // if dropSnodeFromPath throws, it means there is an issue patching up the path, increment the whole path issues count - await OnionPaths.incrementBadPathCountOrDrop(snodeEd25519); + // If dropSnodeFromPath throws, it means there is an issue patching up the path, increment the whole path issues count + // but using the guardNode we got instead of the snodeEd25519. + // + await OnionPaths.incrementBadPathCountOrDrop(guardNodeEd25519); } } else { window?.log?.warn( @@ -839,7 +874,7 @@ export async function lokiOnionFetch( return retriedResult; } catch (e) { window?.log?.warn('onionFetchRetryable failed ', e); - console.warn('error to show to user'); + // console.warn('error to show to user'); throw e; } } From 4b9d2c0692ac219421efb8448d7e29080376cf1a Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Wed, 16 Jun 2021 15:36:06 +1000 Subject: [PATCH 2/2] fix tests --- ts/test/session/unit/onion/OnionErrors_test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/ts/test/session/unit/onion/OnionErrors_test.ts b/ts/test/session/unit/onion/OnionErrors_test.ts index 1227e4441..437669d94 100644 --- a/ts/test/session/unit/onion/OnionErrors_test.ts +++ b/ts/test/session/unit/onion/OnionErrors_test.ts @@ -287,6 +287,7 @@ describe('OnionPathsErrors', () => { expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(1); expect(incrementBadSnodeCountOrDropSpy.firstCall.args[0]).to.deep.eq({ snodeEd25519: targetNode, + guardNodeEd25519: guardSnode1.pubkey_ed25519, associatedWith, }); }); @@ -331,6 +332,7 @@ describe('OnionPathsErrors', () => { expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(1); expect(incrementBadSnodeCountOrDropSpy.firstCall.args[0]).to.deep.eq({ snodeEd25519: targetNode, + guardNodeEd25519: guardSnode1.pubkey_ed25519, associatedWith, }); }); @@ -368,6 +370,7 @@ describe('OnionPathsErrors', () => { expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(1); expect(incrementBadSnodeCountOrDropSpy.firstCall.args[0]).to.deep.eq({ snodeEd25519: targetNode, + guardNodeEd25519: guardSnode1.pubkey_ed25519, associatedWith, }); }); @@ -407,6 +410,7 @@ describe('OnionPathsErrors', () => { expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(1); expect(incrementBadSnodeCountOrDropSpy.firstCall.args[0]).to.deep.eq({ snodeEd25519: targetNode, + guardNodeEd25519: guardSnode1.pubkey_ed25519, associatedWith, }); }); @@ -483,6 +487,7 @@ describe('OnionPathsErrors', () => { expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(1); expect(incrementBadSnodeCountOrDropSpy.firstCall.args[0]).to.deep.eq({ snodeEd25519: failingSnode.pubkey_ed25519, + guardNodeEd25519: guardSnode1.pubkey_ed25519, associatedWith, }); }); @@ -518,6 +523,7 @@ describe('OnionPathsErrors', () => { expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(1); expect(incrementBadSnodeCountOrDropSpy.firstCall.args[0]).to.deep.eq({ snodeEd25519: failingSnode.pubkey_ed25519, + guardNodeEd25519: guardSnode1.pubkey_ed25519, associatedWith, }); }); @@ -560,14 +566,17 @@ describe('OnionPathsErrors', () => { expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(3); expect(incrementBadSnodeCountOrDropSpy.args[0][0]).to.deep.eq({ snodeEd25519: failingSnode.pubkey_ed25519, + guardNodeEd25519: guardSnode1.pubkey_ed25519, associatedWith, }); expect(incrementBadSnodeCountOrDropSpy.args[1][0]).to.deep.eq({ snodeEd25519: failingSnode.pubkey_ed25519, + guardNodeEd25519: guardSnode1.pubkey_ed25519, associatedWith, }); expect(incrementBadSnodeCountOrDropSpy.args[2][0]).to.deep.eq({ snodeEd25519: failingSnode.pubkey_ed25519, + guardNodeEd25519: guardSnode1.pubkey_ed25519, associatedWith, }); expect(incrementBadPathCountOrDropSpy.callCount).to.eq(0); @@ -610,6 +619,7 @@ describe('OnionPathsErrors', () => { for (let index = 0; index < 6; index++) { expect(incrementBadSnodeCountOrDropSpy.args[index][0]).to.deep.eq({ snodeEd25519: oldOnionPaths[0][(index % 2) + 1].pubkey_ed25519, + guardNodeEd25519: guardNode.pubkey_ed25519, }); }