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.

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

Add watchdog for watchers #106

Merged
merged 10 commits into from
Oct 25, 2018
1 change: 1 addition & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ HOME_START_BLOCK=
FOREIGN_START_BLOCK=

LOG_LEVEL=debug
MAX_PROCESSING_TIME=600000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is my understanding correct and 10 min is too much for the watchdog timer? Could you estimate max time expected to handle one iteration of watcher?

Copy link
Author

Choose a reason for hiding this comment

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

I guess it depends on the number of events processed at the same time. I will try sending several transactions to the kovan-sokol bridge and see how much time is spent processing them.

Copy link
Author

Choose a reason for hiding this comment

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

Sent 10 transactions to Sokol. 2 of them were processed in one block, 8 in another. The time difference between the first log, and the log of the sender receiving the last transactions to send was 3 seconds (that is Sokol's block time, isn't it?).

The time is going to depend on the chain, but I guess one minute should be enough in the vast majority of cases (and it can be configured, anyway).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. I think the timout should be comparable with the polling time. I think 3 or 4 times more than the polling time of the corresponding network. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that sounds reasonable. You mean using that as the example, or that the timeout should be computed from the polling time?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, let's set the timeout in the example to be 4 times more that the longest polling time and provide these recommendations in README.

Copy link
Author

Choose a reason for hiding this comment

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

Please check 3171c1e.


fvictorio marked this conversation as resolved.
Show resolved Hide resolved
#testing accs
USER_ADDRESS=0x59c4474184579b9c31b5e51445b6eef91cebf370
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ where the _watcher_ could be one of:
| `REDIS_LOCK_TTL` | Threshold in milliseconds for locking a resource in the Redis DB. Until the threshold is exceeded, the resource is unlocked. Usually it is `1000`. | integer |
| `ALLOW_HTTP` | **Only use in test environments - must be omitted in production environments.**. If this parameter is specified and set to `yes`, RPC URLs can be specified in form of HTTP links. A warning that the connection is insecure will be written to the logs. | `yes` / `no` |
| `LOG_LEVEL` | Set the level of details in the logs. | `trace` / `debug` / `info` / `warn` / `error` / `fatal` |
| `MAX_PROCESSING_TIME` | If this parameter is specified, the workers processes will be killed if this amount of time (in milliseconds) is ellapsed before they finish processing. | integer |

### Useful Commands for Development

Expand Down
4 changes: 2 additions & 2 deletions src/events/processAffirmationRequests/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const rootLogger = require('../../services/logger')
const { web3Home } = require('../../services/web3')
const promiseLimit = require('promise-limit')
const bridgeValidatorsABI = require('../../../abis/BridgeValidators.abi')
const { MAX_CONCURRENT_EVENTS } = require('../../utils/constants')
const { EXIT_CODES, MAX_CONCURRENT_EVENTS } = require('../../utils/constants')
const estimateGas = require('./estimateGas')
const {
AlreadyProcessedError,
Expand Down Expand Up @@ -64,7 +64,7 @@ function processAffirmationRequestsBuilder(config) {
)
} else if (e instanceof InvalidValidatorError) {
logger.fatal({ address: config.validatorAddress }, 'Invalid validator')
process.exit(10)
process.exit(EXIT_CODES.INCOMPATIBILITY)
} else if (e instanceof AlreadySignedError) {
logger.info(`Already signed affirmationRequest ${affirmationRequest.transactionHash}`)
return
Expand Down
4 changes: 2 additions & 2 deletions src/events/processSignatureRequests/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const {
AlreadySignedError,
InvalidValidatorError
} = require('../../utils/errors')
const { MAX_CONCURRENT_EVENTS } = require('../../utils/constants')
const { EXIT_CODES, MAX_CONCURRENT_EVENTS } = require('../../utils/constants')

const { VALIDATOR_ADDRESS_PRIVATE_KEY } = process.env

Expand Down Expand Up @@ -81,7 +81,7 @@ function processSignatureRequestsBuilder(config) {
)
} else if (e instanceof InvalidValidatorError) {
logger.fatal({ address: config.validatorAddress }, 'Invalid validator')
process.exit(10)
process.exit(EXIT_CODES.INCOMPATIBILITY)
} else if (e instanceof AlreadySignedError) {
logger.info(`Already signed signatureRequest ${signatureRequest.transactionHash}`)
return
Expand Down
4 changes: 2 additions & 2 deletions src/events/processTransfers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const {
AlreadySignedError,
InvalidValidatorError
} = require('../../utils/errors')
const { MAX_CONCURRENT_EVENTS } = require('../../utils/constants')
const { EXIT_CODES, MAX_CONCURRENT_EVENTS } = require('../../utils/constants')
const estimateGas = require('../processAffirmationRequests/estimateGas')

const limit = promiseLimit(MAX_CONCURRENT_EVENTS)
Expand Down Expand Up @@ -61,7 +61,7 @@ function processTransfersBuilder(config) {
)
} else if (e instanceof InvalidValidatorError) {
logger.fatal({ address: config.validatorAddress }, 'Invalid validator')
process.exit(10)
process.exit(EXIT_CODES.INCOMPATIBILITY)
} else if (e instanceof AlreadySignedError) {
logger.info(`Already signed transfer ${transfer.transactionHash}`)
return
Expand Down
18 changes: 15 additions & 3 deletions src/sender.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ const { getNonce, getChainId } = require('./tx/web3')
const {
addExtraGas,
checkHTTPS,
privateKeyToAddress,
syncForEach,
waitForFunds,
privateKeyToAddress
watchdog
} = require('./utils/utils')
const { EXTRA_GAS_PERCENTAGE } = require('./utils/constants')
const { EXIT_CODES, EXTRA_GAS_PERCENTAGE } = require('./utils/constants')

const { VALIDATOR_ADDRESS_PRIVATE_KEY, REDIS_LOCK_TTL } = process.env

fvictorio marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -32,6 +33,8 @@ const nonceLock = `lock:${config.id}:nonce`
const nonceKey = `${config.id}:nonce`
let chainId = 0

const maxProcessingTime = process.env.MAX_PROCESSING_TIME
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to define a default value which will be used if MAX_PROCESSING_TIME was missed by some reason in the configuration file.

Copy link
Author

Choose a reason for hiding this comment

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

This was intentional, to allow not setting the watchdog. If you think it should always be enabled, we can add a default value here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What risks can you see if we enable the watchdog by default? In this case the user could disable the watchdog by zeroing the configuration parameter explicitly.

Copy link
Author

Choose a reason for hiding this comment

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

That is a bit complicated. Javascript treats 0 as a falsy value, so doing 0 || 10 evaluates to 10.

On the other hand, the environment variables are strings, so '0' || 10 does evaluate to '0', but it's not good to rely on that; we could easily "break" it if we add environment variables validation and casting, like we did in poa-bridge-contracts/deploy.

A possible solution is to use a negative value to disable it. That will work both with strings and with numbers: '-1' < 0 is equivalent to -1 < 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand this.
Could you use something like:

if (process.env.MAX_PROCESSING_TIME === '0') {
...
} else {
...
}

And later, when you add environment variables validation and casting you will change this.

Copy link
Author

Choose a reason for hiding this comment

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

That can work. This is what I did (see f38688c):

  1. If value is explicitly set to 0, the watchdog is not used.
  2. If the value is not set, it is computed using 4 * maxPollingTime
  3. Otherwise, the set value is used

I didn't want to add this logic twice (in the watcher and the sender), so I moved it to the config. And since the senders weren't including the base config, I also added it to them.

What do you think?


async function initialize() {
try {
const checkHttps = checkHTTPS(process.env.ALLOW_HTTP, logger)
Expand All @@ -44,7 +47,16 @@ async function initialize() {
chainId = await getChainId(web3Instance)
connectSenderToQueue({
queueName: config.queue,
cb: main
cb: options => {
if (maxProcessingTime) {
return watchdog(() => main(options), maxProcessingTime, () => {
logger.fatal('Max processing time reached')
process.exit(EXIT_CODES.MAX_TIME_REACHED)
})
}

return main(options)
}
})
} catch (e) {
logger.error(e.message)
akolotov marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
6 changes: 5 additions & 1 deletion src/utils/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,9 @@ module.exports = {
maxTimeout: 360000,
randomize: true
},
DEFAULT_UPDATE_INTERVAL: 600000
DEFAULT_UPDATE_INTERVAL: 600000,
EXIT_CODES: {
INCOMPATIBILITY: 10,
MAX_TIME_REACHED: 11
}
}
18 changes: 18 additions & 0 deletions src/utils/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,23 @@ function setIntervalAndRun(f, interval) {
return handler
}

/**
* Run function `f` and return its result, unless `timeout` milliseconds pass before `f` ends. If that happens, run
* `kill` instead.
*
* @param {Function} f The function to run. It is assumed that it's an async function.
* @param {Number} timeout Max time in milliseconds to wait for `f` to finish.
* @param {Function} kill Function that will be called if `f` takes more than `timeout` milliseconds.
*/
async function watchdog(f, timeout, kill) {
const timeoutHandler = setTimeout(kill, timeout)

const result = await f()
clearTimeout(timeoutHandler)

return result
}

function add0xPrefix(s) {
if (s.indexOf('0x') === 0) {
return s
Expand All @@ -85,5 +102,6 @@ module.exports = {
waitForFunds,
addExtraGas,
setIntervalAndRun,
watchdog,
privateKeyToAddress
}
14 changes: 12 additions & 2 deletions src/watcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ const { redis } = require('./services/redisClient')
const logger = require('./services/logger')
const rpcUrlsManager = require('./services/getRpcUrlsManager')
const { getRequiredBlockConfirmations, getEvents } = require('./tx/web3')
const { checkHTTPS } = require('./utils/utils')
const { checkHTTPS, watchdog } = require('./utils/utils')
const { EXIT_CODES } = require('./utils/constants')

if (process.argv.length < 3) {
logger.error('Please check the number of arguments, config file was not provided')
fvictorio marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -30,6 +31,8 @@ const eventContract = new web3Instance.eth.Contract(config.eventAbi, config.even
const lastBlockRedisKey = `${config.id}:lastProcessedBlock`
let lastProcessedBlock = BN.max(config.startBlock.sub(ONE), ZERO)

const maxProcessingTime = process.env.MAX_PROCESSING_TIME
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to define a default value which will be used if MAX_PROCESSING_TIME was missed by some reason in the configuration file.


async function initialize() {
try {
const checkHttps = checkHTTPS(process.env.ALLOW_HTTP, logger)
Expand All @@ -51,7 +54,14 @@ async function initialize() {
async function runMain({ sendToQueue }) {
fvictorio marked this conversation as resolved.
Show resolved Hide resolved
try {
if (connection.isConnected() && redis.status === 'ready') {
await main({ sendToQueue })
if (maxProcessingTime) {
await watchdog(() => main({ sendToQueue }), maxProcessingTime, () => {
logger.fatal('Max processing time reached')
process.exit(EXIT_CODES.MAX_TIME_REACHED)
})
} else {
await main({ sendToQueue })
}
}
} catch (e) {
logger.error(e)
Expand Down