Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Make offchain worker calls more future proof. #4618

Merged
merged 3 commits into from
Jan 14, 2020
Merged

Conversation

tomusdrw
Copy link
Contributor

This will help debugging possible issues when we migrate to future versions.

  1. Display an error if version is not supported.
  2. Support only version 1 and 2. Will error out when we ever switch to v3.

@tomusdrw tomusdrw added A0-please_review Pull request needs code review. M4-core labels Jan 14, 2020
@bkchr
Copy link
Member

bkchr commented Jan 14, 2020

Maybe we should add some variant of CanAuthorWith that prevents authoring when the offchain api is not present? Maybe that would be detected earlier?^^

@@ -139,6 +140,8 @@ impl<Client, Storage, Block> OffchainWorkers<
});
futures::future::Either::Left(runner.process())
} else {
let help = "Consider turning off offchain workers if they are not part of your runtime.";
Copy link
Member

Choose a reason for hiding this comment

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

Problem is that validators need the offchain api, otherwise they don't send the heartbeat transactions. So, turning it off is not the best idea?

@gavofyork
Copy link
Member

CI failing, too.

@gavofyork
Copy link
Member

i don't see how not authoring when off-chain workers are not present helps. off-chain workers are designed to not be required for authoring.

@bkchr
Copy link
Member

bkchr commented Jan 14, 2020

i don't see how not authoring when off-chain workers are not present helps. off-chain workers are designed to not be required for authoring.

Yeah I know, but still we currently have some connection. At least to my understanding. Not all validators could produce a block, but could also not execute the offchain worker to send the heartbeat. If you are not selected and not send the tx you are slashed. Sounds not like such an optional feature. In the future when we got offchain phragmen, it could probably be even worse? Or would we switch to on-chain phragmen when nobody provided an offchain one?

@tomusdrw tomusdrw added A8-looksgood and removed A0-please_review Pull request needs code review. A7-looksgoodtestsfail labels Jan 14, 2020
@tomusdrw
Copy link
Contributor Author

Not all validators could produce a block, but could also not execute the offchain worker to send the heartbeat.

Wouldn't then disabling authoring if you can't run offchain workers be even worse? Currently if you can't run offchain workers you at least have some chance of producing blocks, with the change you are proposing it would stall completely.

I suppose a good idea for validators now would be to monitor the node logs for error messages like these and then take some immediate actions. Perhaps better to somehow standardize critical messages reporting? I'm thinking about even posting to some URL given in CLI, so that you can configure IFTTT for instance to send you an email (this wouldn't require any programming knowledge).

@tomusdrw
Copy link
Contributor Author

BTW. CI issue magically resolved, I guess the failure was just spurious.

@bkchr
Copy link
Member

bkchr commented Jan 14, 2020

BTW. CI issue magically resolved, I guess the failure was just spurious.

Yeah, I restarted it a second time.

@bkchr
Copy link
Member

bkchr commented Jan 14, 2020

Not all validators could produce a block, but could also not execute the offchain worker to send the heartbeat.

Wouldn't then disabling authoring if you can't run offchain workers be even worse? Currently if you can't run offchain workers you at least have some chance of producing blocks, with the change you are proposing it would stall completely.

I suppose a good idea for validators now would be to monitor the node logs for error messages like these and then take some immediate actions. Perhaps better to somehow standardize critical messages reporting? I'm thinking about even posting to some URL given in CLI, so that you can configure IFTTT for instance to send you an email (this wouldn't require any programming knowledge).

Yeah, you are right with the authoring.

I also thought about reporting, maybe we could also send some standardized message to telemetry.

@bkchr bkchr merged commit 37be263 into master Jan 14, 2020
@bkchr bkchr deleted the td-offchain-warn branch January 14, 2020 21:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants