From 626bd78cef33835335ff2db1c0643150054dcbb3 Mon Sep 17 00:00:00 2001 From: shahafn Date: Mon, 13 Dec 2021 18:22:34 +0200 Subject: [PATCH] Fixing selection default mechanism (#718) --- packages/common/src/types/Aliases.ts | 3 +- .../test/provider/KnownRelaysManager.test.ts | 39 ++++++++++++---- .../provider/RelaySelectionManager.test.ts | 8 ++-- packages/provider/src/KnownRelaysManager.ts | 44 ++++++++++++------- packages/provider/src/RelayClient.ts | 4 +- 5 files changed, 67 insertions(+), 31 deletions(-) diff --git a/packages/common/src/types/Aliases.ts b/packages/common/src/types/Aliases.ts index 35c46d9b4..77dac2426 100644 --- a/packages/common/src/types/Aliases.ts +++ b/packages/common/src/types/Aliases.ts @@ -7,6 +7,7 @@ import { RelayFailureInfo } from './RelayFailureInfo' import { RelayRegisteredEventInfo } from './GSNContractsDataTypes' import { HttpProvider, IpcProvider, WebsocketProvider } from 'web3-core' import { JsonRpcPayload, JsonRpcResponse } from 'web3-core-helpers' +import BN from 'bn.js' export type Address = string export type EventName = string @@ -20,7 +21,7 @@ export type PingFilter = (pingResponse: PingResponse, gsnTransactionDetails: Gsn export type AsyncDataCallback = (relayRequest: RelayRequest) => Promise export type RelayFilter = (registeredEventInfo: RelayRegisteredEventInfo) => boolean -export type AsyncScoreCalculator = (relay: RelayRegisteredEventInfo, txDetails: GsnTransactionDetails, failures: RelayFailureInfo[]) => Promise +export type AsyncScoreCalculator = (relay: RelayRegisteredEventInfo, txDetails: GsnTransactionDetails, failures: RelayFailureInfo[]) => Promise export function notNull (value: TValue | null | undefined): value is TValue { return value !== null && value !== undefined diff --git a/packages/dev/test/provider/KnownRelaysManager.test.ts b/packages/dev/test/provider/KnownRelaysManager.test.ts index 29f164c2b..2d844eb4a 100644 --- a/packages/dev/test/provider/KnownRelaysManager.test.ts +++ b/packages/dev/test/provider/KnownRelaysManager.test.ts @@ -3,7 +3,7 @@ import { ChildProcessWithoutNullStreams } from 'child_process' import { HttpProvider } from 'web3-core' import { ether } from '@openzeppelin/test-helpers' -import { KnownRelaysManager, DefaultRelayScore } from '@opengsn/provider/dist/KnownRelaysManager' +import { KnownRelaysManager, DefaultRelayScore, DefaultRelayFilter } from '@opengsn/provider/dist/KnownRelaysManager' import { ContractInteractor } from '@opengsn/common/dist/ContractInteractor' import { GSNConfig } from '@opengsn/provider/dist/GSNConfigurator' import { @@ -21,6 +21,7 @@ import { RelayInfoUrl, RelayRegisteredEventInfo } from '@opengsn/common/dist/typ import { createClientLogger } from '@opengsn/provider/dist/ClientWinstonLogger' import { registerForwarderForGsn } from '@opengsn/common/dist/EIP712/ForwarderUtil' import { defaultEnvironment } from '@opengsn/common/dist/Environments' +import { toBN } from 'web3-utils' const StakeManager = artifacts.require('StakeManager') const Penalizer = artifacts.require('Penalizer') @@ -161,7 +162,7 @@ contract('KnownRelaysManager 2', function (accounts) { let logger: LoggerInterface const transactionDetails = { - gas: '0x10000', + gas: '0x10000000', gasPrice: '0x300000', from: '', data: '', @@ -252,6 +253,23 @@ contract('KnownRelaysManager 2', function (accounts) { assert.equal(relays.length, 1) assert.equal(relays[0].relayUrl, 'stakeAndAuthorization2') }) + + it('should use DefaultRelayFilter to remove unsuitable relays when none was provided', async function () { + const knownRelaysManagerWithFilter = new KnownRelaysManager(contractInteractor, logger, config) + // @ts-ignore + assert.equal(knownRelaysManagerWithFilter.relayFilter.toString(), DefaultRelayFilter.toString()) + }) + + describe('DefaultRelayFilter', function () { + it('should filter expensive relayers', function () { + const eventInfo = { relayUrl: 'url', relayManager: accounts[0] } + assert.isFalse(DefaultRelayFilter({ ...eventInfo, pctRelayFee: '101', baseRelayFee: 1e16.toString() })) + assert.isFalse(DefaultRelayFilter({ ...eventInfo, pctRelayFee: '99', baseRelayFee: 2e17.toString() })) + assert.isFalse(DefaultRelayFilter({ ...eventInfo, pctRelayFee: '101', baseRelayFee: 2e17.toString() })) + assert.isTrue(DefaultRelayFilter({ ...eventInfo, pctRelayFee: '100', baseRelayFee: 1e17.toString() })) + assert.isTrue(DefaultRelayFilter({ ...eventInfo, pctRelayFee: '50', baseRelayFee: '0' })) + }) + }) }) describe('#getRelaysSortedForTransaction()', function () { @@ -309,19 +327,24 @@ contract('KnownRelaysManager 2', function (accounts) { const relayScoreOneFailure = await DefaultRelayScore(relayInfoHighFee, transactionDetails, [failure]) const relayScoreTenFailures = await DefaultRelayScore(relayInfoHighFee, transactionDetails, Array(10).fill(failure)) const relayScoreLowFees = await DefaultRelayScore(relayInfoLowFee, transactionDetails, []) - assert.isAbove(relayScoreNoFailures, relayScoreOneFailure) - assert.isAbove(relayScoreOneFailure, relayScoreTenFailures) - assert.isAbove(relayScoreLowFees, relayScoreNoFailures) + assert.isTrue(relayScoreNoFailures.gt(relayScoreOneFailure)) + assert.isTrue(relayScoreOneFailure.gt(relayScoreTenFailures)) + assert.isTrue(relayScoreLowFees.gt(relayScoreNoFailures)) + }) + it('should use DefaultRelayScore to remove unsuitable relays when none was provided', async function () { + const knownRelaysManagerWithFilter = new KnownRelaysManager(contractInteractor, logger, configureGSN({})) + // @ts-ignore + assert.equal(knownRelaysManagerWithFilter.scoreCalculator.toString(), DefaultRelayScore.toString()) }) }) }) describe('getRelaysSortedForTransaction', function () { - const biasedRelayScore = async function (relay: RelayRegisteredEventInfo): Promise { + const biasedRelayScore = async function (relay: RelayRegisteredEventInfo): Promise { if (relay.relayUrl === 'alex') { - return await Promise.resolve(1000) + return await Promise.resolve(toBN(1000)) } else { - return await Promise.resolve(100) + return await Promise.resolve(toBN(100)) } } const knownRelaysManager = new KnownRelaysManager(contractInteractor, logger, configureGSN({}), undefined, biasedRelayScore) diff --git a/packages/dev/test/provider/RelaySelectionManager.test.ts b/packages/dev/test/provider/RelaySelectionManager.test.ts index 421b9209f..594906241 100644 --- a/packages/dev/test/provider/RelaySelectionManager.test.ts +++ b/packages/dev/test/provider/RelaySelectionManager.test.ts @@ -7,7 +7,7 @@ import { HttpClient } from '@opengsn/common/dist/HttpClient' import { HttpWrapper } from '@opengsn/common/dist/HttpWrapper' import { PingResponse } from '@opengsn/common/dist/PingResponse' import { RelaySelectionManager } from '@opengsn/provider/dist/RelaySelectionManager' -import { EmptyFilter, KnownRelaysManager } from '@opengsn/provider/dist/KnownRelaysManager' +import { DefaultRelayFilter, KnownRelaysManager } from '@opengsn/provider/dist/KnownRelaysManager' import { GasPricePingFilter } from '@opengsn/provider/dist/RelayClient' import { PartialRelayInfo, RelayInfo } from '@opengsn/common/dist/types/RelayInfo' import { PingFilter } from '@opengsn/common/dist/types/Aliases' @@ -75,7 +75,7 @@ contract('RelaySelectionManager', function (accounts) { logger }).init() httpClient = new HttpClient(new HttpWrapper(), this.logger) - knownRelaysManager = new KnownRelaysManager(contractInteractor, logger, configureGSN({}), EmptyFilter) + knownRelaysManager = new KnownRelaysManager(contractInteractor, logger, configureGSN({}), DefaultRelayFilter) stubGetRelaysSorted = sinon.stub(knownRelaysManager, 'getRelaysSortedForTransaction') stubGetRelaysSorted.returns(Promise.resolve([[eventInfo]])) relaySelectionManager = await new RelaySelectionManager(transactionDetails, knownRelaysManager, httpClient, GasPricePingFilter, logger, config).init() @@ -120,7 +120,7 @@ contract('RelaySelectionManager', function (accounts) { const penalizer = await Penalizer.new(defaultEnvironment.penalizerConfiguration.penalizeBlockDelay, defaultEnvironment.penalizerConfiguration.penalizeBlockExpiration) relayHub = await deployHub(stakeManager.address, penalizer.address) await stake(stakeManager, relayHub, relayManager, accounts[0]) - await register(relayHub, relayManager, accounts[2], preferredRelayUrl, '666', '777') + await register(relayHub, relayManager, accounts[2], preferredRelayUrl, '666', '77') await contractInteractor.initDeployment({ relayHubAddress: relayHub.address, @@ -162,7 +162,7 @@ contract('RelaySelectionManager', function (accounts) { assert.equal(nextRelay.relayInfo.relayUrl, preferredRelayUrl) assert.equal(nextRelay.relayInfo.relayManager, relayManager) assert.equal(nextRelay.relayInfo.baseRelayFee, '666') - assert.equal(nextRelay.relayInfo.pctRelayFee, '777') + assert.equal(nextRelay.relayInfo.pctRelayFee, '77') }) }) diff --git a/packages/provider/src/KnownRelaysManager.ts b/packages/provider/src/KnownRelaysManager.ts index 3a5371521..0a4d4dc36 100644 --- a/packages/provider/src/KnownRelaysManager.ts +++ b/packages/provider/src/KnownRelaysManager.ts @@ -16,22 +16,34 @@ import { } from '@opengsn/common/dist/types/GSNContractsDataTypes' import { LoggerInterface } from '@opengsn/common/dist/LoggerInterface' import { ContractInteractor } from '@opengsn/common/dist/ContractInteractor' - -export const EmptyFilter: RelayFilter = (): boolean => { +import { MAX_INTEGER } from 'ethereumjs-util' +import { toBN } from 'web3-utils' +import BN from 'bn.js' + +export const DefaultRelayFilter: RelayFilter = function (registeredEventInfo: RelayRegisteredEventInfo): boolean { + const maxPctRelayFee = 100 + const maxBaseRelayFee = 1e17 + if ( + parseInt(registeredEventInfo.pctRelayFee) > maxPctRelayFee || + parseInt(registeredEventInfo.baseRelayFee) > maxBaseRelayFee + ) { + return false + } return true } /** * Basic score is reversed transaction fee, higher is better. * Relays that failed to respond recently will be downgraded for some period of time. */ -export const DefaultRelayScore = async function (relay: RelayRegisteredEventInfo, txDetails: GsnTransactionDetails, failures: RelayFailureInfo[]): Promise { - const gasLimit = parseInt(txDetails.gas ?? '0') - const gasPrice = parseInt(txDetails.gasPrice ?? '0') - const pctFee = parseInt(relay.pctRelayFee) - const baseFee = parseInt(relay.baseRelayFee) - const transactionCost = baseFee + (gasLimit * gasPrice * (100 + pctFee)) / 100 - let score = Math.max(Number.MAX_SAFE_INTEGER - transactionCost, 0) - score = score * Math.pow(0.9, failures.length) +export const DefaultRelayScore: AsyncScoreCalculator = async function (relay: RelayRegisteredEventInfo, txDetails: GsnTransactionDetails, failures: RelayFailureInfo[]): Promise { + const gasLimit = toBN(txDetails.gas ?? '0') + const gasPrice = toBN(txDetails.gasPrice ?? '0') + const pctFee = toBN(relay.pctRelayFee) + const baseFee = toBN(relay.baseRelayFee) + const transactionCost = baseFee.add(gasLimit.mul(gasPrice).muln((100 + pctFee.toNumber()) / 100) + ) + let score = MAX_INTEGER.sub(transactionCost) + score = score.muln(Math.pow(0.9, failures.length)) return score } @@ -51,7 +63,7 @@ export class KnownRelaysManager { constructor (contractInteractor: ContractInteractor, logger: LoggerInterface, config: GSNConfig, relayFilter?: RelayFilter, scoreCalculator?: AsyncScoreCalculator) { this.config = config this.logger = logger - this.relayFilter = relayFilter ?? EmptyFilter + this.relayFilter = relayFilter ?? DefaultRelayFilter this.scoreCalculator = scoreCalculator ?? DefaultRelayScore this.contractInteractor = contractInteractor } @@ -173,9 +185,9 @@ export class KnownRelaysManager { } async _sortRelaysInternal (gsnTransactionDetails: GsnTransactionDetails, activeRelays: RelayInfoUrl[]): Promise { - const scores = new Map() + const scores = new Map() for (const activeRelay of activeRelays) { - let score = 0 + let score = toBN(0) if (isInfoFromEvent(activeRelay)) { const eventInfo = activeRelay as RelayRegisteredEventInfo score = await this.scoreCalculator(eventInfo, gsnTransactionDetails, this.relayFailures.get(activeRelay.relayUrl) ?? []) @@ -187,9 +199,9 @@ export class KnownRelaysManager { .filter(isInfoFromEvent) .map(value => (value as RelayRegisteredEventInfo)) .sort((a, b) => { - const aScore = scores.get(a.relayManager) ?? 0 - const bScore = scores.get(b.relayManager) ?? 0 - return bScore - aScore + const aScore = scores.get(a.relayManager) ?? toBN(0) + const bScore = scores.get(b.relayManager) ?? toBN(0) + return bScore.cmp(aScore) }) } diff --git a/packages/provider/src/RelayClient.ts b/packages/provider/src/RelayClient.ts index 01569d779..7a1278052 100644 --- a/packages/provider/src/RelayClient.ts +++ b/packages/provider/src/RelayClient.ts @@ -19,7 +19,7 @@ import { HttpClient } from '@opengsn/common/dist/HttpClient' import { HttpWrapper } from '@opengsn/common/dist/HttpWrapper' import { RelaySelectionManager } from './RelaySelectionManager' import { RelayedTransactionValidator } from './RelayedTransactionValidator' -import { DefaultRelayScore, EmptyFilter, KnownRelaysManager } from './KnownRelaysManager' +import { DefaultRelayScore, DefaultRelayFilter, KnownRelaysManager } from './KnownRelaysManager' import { createClientLogger } from './ClientWinstonLogger' import { defaultGsnConfig, defaultLoggerConfiguration, GSNConfig, GSNDependencies } from './GSNConfigurator' @@ -448,7 +448,7 @@ export class RelayClient { const httpClient = overrideDependencies?.httpClient ?? new HttpClient(new HttpWrapper(), this.logger) const pingFilter = overrideDependencies?.pingFilter ?? GasPricePingFilter - const relayFilter = overrideDependencies?.relayFilter ?? EmptyFilter + const relayFilter = overrideDependencies?.relayFilter ?? DefaultRelayFilter const asyncApprovalData = overrideDependencies?.asyncApprovalData ?? EmptyDataCallback const asyncPaymasterData = overrideDependencies?.asyncPaymasterData ?? EmptyDataCallback const scoreCalculator = overrideDependencies?.scoreCalculator ?? DefaultRelayScore