Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OG-658 Fixing selection default mechanism #718

Merged
merged 1 commit into from
Dec 13, 2021
Merged

OG-658 Fixing selection default mechanism #718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/common/src/types/Aliases.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -20,7 +21,7 @@ export type PingFilter = (pingResponse: PingResponse, gsnTransactionDetails: Gsn
export type AsyncDataCallback = (relayRequest: RelayRequest) => Promise<PrefixedHexString>

export type RelayFilter = (registeredEventInfo: RelayRegisteredEventInfo) => boolean
export type AsyncScoreCalculator = (relay: RelayRegisteredEventInfo, txDetails: GsnTransactionDetails, failures: RelayFailureInfo[]) => Promise<number>
export type AsyncScoreCalculator = (relay: RelayRegisteredEventInfo, txDetails: GsnTransactionDetails, failures: RelayFailureInfo[]) => Promise<BN>

export function notNull<TValue> (value: TValue | null | undefined): value is TValue {
return value !== null && value !== undefined
Expand Down
39 changes: 31 additions & 8 deletions packages/dev/test/provider/KnownRelaysManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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')
Expand Down Expand Up @@ -161,7 +162,7 @@ contract('KnownRelaysManager 2', function (accounts) {
let logger: LoggerInterface

const transactionDetails = {
gas: '0x10000',
gas: '0x10000000',
gasPrice: '0x300000',
from: '',
data: '',
Expand Down Expand Up @@ -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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you check toString that its not constant for all functions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is the function code as string.

})

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 () {
Expand Down Expand Up @@ -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<number> {
const biasedRelayScore = async function (relay: RelayRegisteredEventInfo): Promise<BN> {
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)
Expand Down
8 changes: 4 additions & 4 deletions packages/dev/test/provider/RelaySelectionManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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')
})
})

Expand Down
44 changes: 28 additions & 16 deletions packages/provider/src/KnownRelaysManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. should be exported constants
  2. on mainnet, might 0.1eth is a good value , but on (say) polygon, a maximum of 0.1 matic is no good. you might say "so the client should override the default" - but the relayers can't control the clients, and most clients will stick to defaults.
  3. So this makes the "maxBaseRelayFee" useless on cheap networks - even though I think it is more important there (relayer's cost has almost nothing to do with gas price, so a factor is less favorable)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why should they be exported? it's a default implementation, by no means it has any bearing on other implementations, I don't see why we should export these.

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<number> {
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<BN> {
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want to make it the max possible number? we do want to have a number higher than max possible gas so 100eth gas price * arbitrum gas limit yields 1e20 * 1e10 ~ 1e30
our current score uses only negative parameters, but we should allow algorithms to increase the score. by using MAX_INTEGER we actively prevent it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(yes, and I also don't think using BN is good thing - it ties our code to some BigNumber implementation, and if/when we change to ethers, we to change the API (or carry a requirement for another BN library)
We don't need the "bit accuracy" of big numbers at all

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forget it. what we need is weighted scoring, and this is not something we'll now.

score = score.muln(Math.pow(0.9, failures.length))
return score
}

Expand All @@ -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
}
Expand Down Expand Up @@ -173,9 +185,9 @@ export class KnownRelaysManager {
}

async _sortRelaysInternal (gsnTransactionDetails: GsnTransactionDetails, activeRelays: RelayInfoUrl[]): Promise<RelayInfoUrl[]> {
const scores = new Map<string, number>()
const scores = new Map<string, BN>()
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) ?? [])
Expand All @@ -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)
})
}

Expand Down
4 changes: 2 additions & 2 deletions packages/provider/src/RelayClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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
Expand Down