From 05e198a64f43afbf035fee406f27e0b35cb90721 Mon Sep 17 00:00:00 2001 From: Darius Parvin Date: Mon, 14 Feb 2022 22:45:29 -0800 Subject: [PATCH] feat: add retry logic to external signer if the request to the external signer fails with ECONNREFUSED, retry the request three times with exponentially increasing timeout up to 3 times. Ticket: BG-43083 --- modules/express/README.md | 2 +- modules/express/package.json | 4 +- modules/express/src/clientRoutes.ts | 16 ++- modules/express/src/retryPromise.ts | 48 ++++++++ .../test/integration/externalSigner.ts | 110 ++++++++++++++++++ yarn.lock | 55 ++++++++- 6 files changed, 225 insertions(+), 10 deletions(-) create mode 100644 modules/express/src/retryPromise.ts diff --git a/modules/express/README.md b/modules/express/README.md index 71a802d4ad..027c2f923b 100644 --- a/modules/express/README.md +++ b/modules/express/README.md @@ -168,7 +168,7 @@ BitGo Express currently supports the following proxy protocols: BitGo Express can be run in an external signing mode, where the signing of transactions is performed in a separate instance of BitGo Express that has access to the private keys. This may be preferable for users who would like to apply their signature to their transactions using a more secure environment than BitGo SDK or BitGo Express, such as a signer with restricted access or network connectivity. -To set up BitGo Express with an external signer, a url to the external signer instance of BitGo Express must be provided using the `externalSignerUrl` configuration option. The corresponding external signer instance of BitGo Express must have `signerMode` set, and `signerFileSystemPath` set to the path of a json containing the private key. +To set up BitGo Express with an external signer, a url to the external signer instance of BitGo Express must be provided using the `externalSignerUrl` configuration option. The corresponding external signer instance of BitGo Express must have `signerMode` set, and `signerFileSystemPath` set to the path of a json containing the private key. Note that if BitGo Express encounters an `ECONNREFUSED` error when requesting the external signer for a signature, it will retry the request for up to 15 seconds. The JSON file containing the unencrypted private key(s) must be in the format `"": ""` wallet ID and private key. Here is an example json file containing two wallet IDs and their corresponding private keys. diff --git a/modules/express/package.json b/modules/express/package.json index 59026a7461..0afbecc8d6 100644 --- a/modules/express/package.json +++ b/modules/express/package.json @@ -57,7 +57,7 @@ "@types/morgan": "^1.7.35", "@types/nock": "^9.3.1", "@types/node": "^11.11.4", - "@types/sinon": "^7.0.6", + "@types/sinon": "^10.0.11", "@types/supertest": "^2.0.11", "bignumber.js": "^8.0.1", "lint-staged": "^9.2.0", @@ -68,7 +68,7 @@ "should": "^13.2.3", "should-http": "^0.1.1", "should-sinon": "^0.0.6", - "sinon": "^6.3.5", + "sinon": "^13.0.1", "supertest": "^4.0.2", "supertest-as-promised": "https://github.com/BitGo/supertest-as-promised/archive/a7f4b612b9fa090ae33a9616c41862aec2b25c7e.tar.gz" }, diff --git a/modules/express/src/clientRoutes.ts b/modules/express/src/clientRoutes.ts index 3b31f56605..36cfec4d51 100755 --- a/modules/express/src/clientRoutes.ts +++ b/modules/express/src/clientRoutes.ts @@ -28,6 +28,7 @@ import { Config } from './config'; import { ApiResponseError } from './errors'; import { promises as fs } from 'fs'; import * as assert from 'assert'; +import { retryPromise } from './retryPromise'; const { version } = require('bitgo/package.json'); const pjson = require('../package.json'); @@ -1025,11 +1026,16 @@ export function createCustomSigningFunction(externalSignerUrl: string): CustomSi txPrebuild: TransactionPrebuild; pubs?: string[]; }): Promise { - const { body: signedTx } = await superagent - .post(`${externalSignerUrl}/api/v2/${params.coin.getChain()}/sign`) - .type('json') - .send({ txPrebuild: params.txPrebuild, pubs: params.pubs }); - + const { body: signedTx } = await retryPromise( + () => + superagent + .post(`${externalSignerUrl}/api/v2/${params.coin.getChain()}/sign`) + .type('json') + .send({ txPrebuild: params.txPrebuild, pubs: params.pubs }), + (err, tryCount) => { + console.error(`attempt number ${tryCount}`); + } + ); return signedTx; }; } diff --git a/modules/express/src/retryPromise.ts b/modules/express/src/retryPromise.ts new file mode 100644 index 0000000000..9e8cad6285 --- /dev/null +++ b/modules/express/src/retryPromise.ts @@ -0,0 +1,48 @@ +/** + * Thrown in `retryPromise()` + * + * @prettier + */ + +export class ErrorMaxRetriesExceededError extends Error { + constructor(maxTries: number) { + super(`giving up after reaching max retry limit of ${maxTries}`); + } +} + +/** + * Retries a promise (like a request) if it returns with 'ECONNREFUSED'. Retries are delayed with an exponential backoff. + * @param {Function} func - Promise to execute. When it throws an error, it is called and passed to onError + * @param {Function} onError - Error handler. Called with error as argument. + * If an error should not be retried, the handler should re-throw the passed error. + * @param params + * @param {Number} params.retryLimit - the maximum number of retries to attempt before giving up. + */ +export async function retryPromise( + func: () => Promise, + onError: (err: Error, tryCount: number) => void = () => ({}), + params: { retryLimit: number } = { retryLimit: 3 } +): Promise { + let tryCount = 0; + + while (tryCount < params.retryLimit) { + tryCount += 1; + try { + return await func(); + } catch (err) { + if (err.code === 'ECONNREFUSED') { + onError(err, tryCount); + } else { + throw new Error(err); + } + } + + // if we are going to make another attempt, delay first with exponential backoff + if (tryCount < params.retryLimit) { + const secondsToWait = 2 ** (tryCount - 1) + Math.random(); + await new Promise((res) => setTimeout(res, Math.round(secondsToWait * 1000))); + } + } + + throw new ErrorMaxRetriesExceededError(params.retryLimit); +} diff --git a/modules/express/test/integration/externalSigner.ts b/modules/express/test/integration/externalSigner.ts index 4e93680c8f..81b6dc13cb 100644 --- a/modules/express/test/integration/externalSigner.ts +++ b/modules/express/test/integration/externalSigner.ts @@ -19,6 +19,7 @@ describe('Custom signing function', () => { debug: true, env: 'test', externalSignerUrl, + timeout: 60000, }; const app = expressApp(args); @@ -85,4 +86,113 @@ describe('Custom signing function', () => { postProcessPrebuildStub.restore(); verifyTransactionStub.restore(); }); + + it('should retry requests to external signer on an "ECONNREFUSED" error', async function () { + const bgUrl = Environments.test.uri; + // setup persistent nock to external signer + const signernock = nock(externalSignerUrl) + .post('/api/v2/btc/sign') + .times(3) + .replyWithError({ code: 'ECONNREFUSED' }); + + // setup nock to wallet platform GET /wallet/fakeid + const wpWalletnock = nock(bgUrl) + .get('/api/v2/btc/wallet/fakeid') + .reply(200, { id: 'fakeid', keys: ['abc', 'def', 'ghi'], coinSpecific: {} }); + const wpKeychainNocks = [ + nock(bgUrl).get('/api/v2/btc/key/abc').reply(200, { pub: 'xpubabc' }), + nock(bgUrl).get('/api/v2/btc/key/def').reply(200, { pub: 'xpubdef' }), + nock(bgUrl).get('/api/v2/btc/key/ghi').reply(200, { pub: 'xpubghi' }), + ]; + const wpLatestBlockNock = nock(bgUrl).get('/api/v2/btc/public/block/latest').reply(200); + + const wpBuildnock = nock(bgUrl) + .post('/api/v2/btc/wallet/fakeid/tx/build') + .reply(200, { wpBuild: 'WP build response' }); + + const postProcessPrebuildStub = sinon.stub(Btc.prototype, 'postProcessPrebuild').resolvesArg(0); + const verifyTransactionStub = sinon.stub(Btc.prototype, 'verifyTransaction').resolves(true); + + const clock = sinon.useFakeTimers(); + + // make request to express application to initiate send + const resultPromise = agent + .post('/api/v2/btc/wallet/fakeid/sendcoins') + .type('json') + .send({ address: 'abc', amount: 123 }); + + // every 10 "fake" ms, push the clock forward another 10 "fake" ms + const interval = clock.setInterval(async () => { + await clock.tickAsync(10); + }, 10); + + // start running the setInterval loop above to push the clock forward + clock.next(); + + const result = await resultPromise; + + result.ok.should.be.false(); + + result.should.have.property('text'); + result.text.should.match(/giving up after reaching max retry limit of/); + + signernock.done(); + wpKeychainNocks.forEach((s) => s.done()); + wpLatestBlockNock.done(); + wpWalletnock.done(); + wpBuildnock.done(); + + clock.clearInterval(interval as any); + clock.restore(); + postProcessPrebuildStub.restore(); + verifyTransactionStub.restore(); + }); + + it('should not retry requests to external signer for an error other than "ECONNREFUSED"', async function () { + const bgUrl = Environments.test.uri; + // setup nocks to external signer + const signernock = nock(externalSignerUrl).post('/api/v2/btc/sign').replyWithError({ code: 'not ECONNREFUSED' }); + + // setup nock to wallet platform GET /wallet/fakeid + const wpWalletnock = nock(bgUrl) + .get('/api/v2/btc/wallet/fakeid') + .reply(200, { id: 'fakeid', keys: ['abc', 'def', 'ghi'], coinSpecific: {} }); + const wpKeychainNocks = [ + nock(bgUrl).get('/api/v2/btc/key/abc').reply(200, { pub: 'xpubabc' }), + nock(bgUrl).get('/api/v2/btc/key/def').reply(200, { pub: 'xpubdef' }), + nock(bgUrl).get('/api/v2/btc/key/ghi').reply(200, { pub: 'xpubghi' }), + ]; + const wpLatestBlockNock = nock(bgUrl).get('/api/v2/btc/public/block/latest').reply(200); + + const wpBuildnock = nock(bgUrl) + .post('/api/v2/btc/wallet/fakeid/tx/build') + .reply(200, { wpBuild: 'WP build response' }); + + const postProcessPrebuildStub = sinon.stub(Btc.prototype, 'postProcessPrebuild').resolvesArg(0); + const verifyTransactionStub = sinon.stub(Btc.prototype, 'verifyTransaction').resolves(true); + + // check to make sure request to external signer is not attempted a second time + nock.emitter.on('no match', (req) => { + if (req.path === '/api/v2/btc/sign') { + throw new Error(`Unexpected retry request was sent to ${req.path}`); + } + }); + + // make request to express application to initiate send + const result = await agent + .post('/api/v2/btc/wallet/fakeid/sendcoins') + .type('json') + .send({ address: 'abc', amount: 123 }); + + result.ok.should.be.false(); + + signernock.done(); + wpKeychainNocks.forEach((s) => s.done()); + wpLatestBlockNock.done(); + wpWalletnock.done(); + wpBuildnock.done(); + + postProcessPrebuildStub.restore(); + verifyTransactionStub.restore(); + }); }); diff --git a/yarn.lock b/yarn.lock index 279da87c12..3062ec856e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2283,13 +2283,20 @@ resolved "https://registry.yarnpkg.com/@sindresorhus/is/-/is-0.14.0.tgz#9fb3a3cf3132328151f353de4632e01e52102bea" integrity sha512-9NET910DNaIPngYnLLPeg+Ogzqsi9uM4mSboU5y6p8S5DzMTVEsJZrawi+BoDNUVBa2DhJqQYUFvMDfgU062LQ== -"@sinonjs/commons@^1", "@sinonjs/commons@^1.0.2", "@sinonjs/commons@^1.3.0", "@sinonjs/commons@^1.4.0", "@sinonjs/commons@^1.7.0": +"@sinonjs/commons@^1", "@sinonjs/commons@^1.0.2", "@sinonjs/commons@^1.3.0", "@sinonjs/commons@^1.4.0", "@sinonjs/commons@^1.6.0", "@sinonjs/commons@^1.7.0", "@sinonjs/commons@^1.8.3": version "1.8.3" resolved "https://registry.yarnpkg.com/@sinonjs/commons/-/commons-1.8.3.tgz#3802ddd21a50a949b6721ddd72da36e67e7f1b2d" integrity sha512-xkNcLAn/wZaX14RPlwizcKicDk9G3F8m2nU3L7Ukm5zBgTwiT0wsoFAHx9Jq56fJA1z/7uKGtCRu16sOUCLIHQ== dependencies: type-detect "4.0.8" +"@sinonjs/fake-timers@>=5", "@sinonjs/fake-timers@^9.0.0": + version "9.1.0" + resolved "https://registry.yarnpkg.com/@sinonjs/fake-timers/-/fake-timers-9.1.0.tgz#8c92c56f195e0bed4c893ba59c8e3d55831ca0df" + integrity sha512-M8vapsv9qQupMdzrVzkn5rb9jG7aUTEPAZdMtME2PuBaefksFZVE2C1g4LBRTkF/k3nRDNbDc5tp5NFC1PEYxA== + dependencies: + "@sinonjs/commons" "^1.7.0" + "@sinonjs/formatio@^3.0.0", "@sinonjs/formatio@^3.2.1": version "3.2.2" resolved "https://registry.yarnpkg.com/@sinonjs/formatio/-/formatio-3.2.2.tgz#771c60dfa75ea7f2d68e3b94c7e888a78781372c" @@ -2312,6 +2319,15 @@ array-from "^2.1.1" lodash "^4.17.15" +"@sinonjs/samsam@^6.1.1": + version "6.1.1" + resolved "https://registry.yarnpkg.com/@sinonjs/samsam/-/samsam-6.1.1.tgz#627f7f4cbdb56e6419fa2c1a3e4751ce4f6a00b1" + integrity sha512-cZ7rKJTLiE7u7Wi/v9Hc2fs3Ucc3jrWeMgPHbbTCeVAB2S0wOBbYlkJVeNSL04i7fdhT8wIbDq1zhC/PXTD2SA== + dependencies: + "@sinonjs/commons" "^1.6.0" + lodash.get "^4.4.2" + type-detect "^4.0.8" + "@sinonjs/text-encoding@^0.7.1": version "0.7.1" resolved "https://registry.yarnpkg.com/@sinonjs/text-encoding/-/text-encoding-0.7.1.tgz#8da5c6530915653f3a1f38fd5f101d8c3f8079c5" @@ -2959,11 +2975,23 @@ dependencies: "@types/node" "*" +"@types/sinon@^10.0.11": + version "10.0.11" + resolved "https://registry.yarnpkg.com/@types/sinon/-/sinon-10.0.11.tgz#8245827b05d3fc57a6601bd35aee1f7ad330fc42" + integrity sha512-dmZsHlBsKUtBpHriNjlK0ndlvEh8dcb9uV9Afsbt89QIyydpC7NcR+nWlAhASfy3GHnxTl4FX/aKE7XZUt/B4g== + dependencies: + "@types/sinonjs__fake-timers" "*" + "@types/sinon@^7.0.6", "@types/sinon@^7.5.0": version "7.5.2" resolved "https://registry.yarnpkg.com/@types/sinon/-/sinon-7.5.2.tgz#5e2f1d120f07b9cda07e5dedd4f3bf8888fccdb9" integrity sha512-T+m89VdXj/eidZyejvmoP9jivXgBDdkOSBVQjU9kF349NEx10QdPNGxHeZUaj1IlJ32/ewdyXJjnJxyxJroYwg== +"@types/sinonjs__fake-timers@*": + version "8.1.1" + resolved "https://registry.yarnpkg.com/@types/sinonjs__fake-timers/-/sinonjs__fake-timers-8.1.1.tgz#b49c2c70150141a15e0fa7e79cf1f92a72934ce3" + integrity sha512-0kSuKjAS0TrGLJ0M/+8MaFkGsQhZpB6pxOmvS3K8FYI72K//YmdfoW9X2qPsAKh1mkwxGD5zib9s1FIFed6E8g== + "@types/superagent@*", "@types/superagent@^4.1.3": version "4.1.15" resolved "https://registry.yarnpkg.com/@types/superagent/-/superagent-4.1.15.tgz#63297de457eba5e2bc502a7609426c4cceab434a" @@ -10571,6 +10599,17 @@ nise@^1.4.5, nise@^1.5.2: lolex "^5.0.1" path-to-regexp "^1.7.0" +nise@^5.1.1: + version "5.1.1" + resolved "https://registry.yarnpkg.com/nise/-/nise-5.1.1.tgz#ac4237e0d785ecfcb83e20f389185975da5c31f3" + integrity sha512-yr5kW2THW1AkxVmCnKEh4nbYkJdB3I7LUkiUgOvEkOp414mc2UMaHMA7pjq1nYowhdoJZGwEKGaQVbxfpWj10A== + dependencies: + "@sinonjs/commons" "^1.8.3" + "@sinonjs/fake-timers" ">=5" + "@sinonjs/text-encoding" "^0.7.1" + just-extend "^4.0.2" + path-to-regexp "^1.7.0" + no-case@^3.0.4: version "3.0.4" resolved "https://registry.yarnpkg.com/no-case/-/no-case-3.0.4.tgz#d361fd5c9800f558551a8369fc0dcd4662b6124d" @@ -12949,6 +12988,18 @@ simple-git@^1.85.0: dependencies: debug "^4.0.1" +sinon@^13.0.1: + version "13.0.1" + resolved "https://registry.yarnpkg.com/sinon/-/sinon-13.0.1.tgz#2a568beca2084c48985dd98e276e065c81738e3c" + integrity sha512-8yx2wIvkBjIq/MGY1D9h1LMraYW+z1X0mb648KZnKSdvLasvDu7maa0dFaNYdTDczFgbjNw2tOmWdTk9saVfwQ== + dependencies: + "@sinonjs/commons" "^1.8.3" + "@sinonjs/fake-timers" "^9.0.0" + "@sinonjs/samsam" "^6.1.1" + diff "^5.0.0" + nise "^5.1.1" + supports-color "^7.2.0" + sinon@^6.3.5: version "6.3.5" resolved "https://registry.yarnpkg.com/sinon/-/sinon-6.3.5.tgz#0f6d6a5b4ebaad1f6e8e019395542d1d02c144a0" @@ -13731,7 +13782,7 @@ supports-color@^5.3.0, supports-color@^5.5.0: dependencies: has-flag "^3.0.0" -supports-color@^7.1.0: +supports-color@^7.1.0, supports-color@^7.2.0: version "7.2.0" resolved "https://registry.yarnpkg.com/supports-color/-/supports-color-7.2.0.tgz#1b7dcdcb32b8138801b3e478ba6a51caa89648da" integrity sha512-qpCAvRl9stuOHveKsn7HncJRvv501qIacKzQlO/+Lwxc9+0q2wLyv4Dfvt80/DPn2pqOBsJdDiogXGR9+OvwRw==