worldcoin / signup-sequencer

signup-sequencer repository
MIT License
68 stars 33 forks source link

Defer tree initialization PRO-498 #682

Closed ewoolsey closed 7 months ago

ewoolsey commented 7 months ago

This is a first draft of deferring tree initialization. I have some open TODOs here that I would like some feedback on.

  1. different versions of Duration are being used in different places. Is this intended? from chrono and std.
  2. We need to delay TaskManager::start until after the tree initialization. It's not clear what the best way to do this is. Multiple routes we could take.
  3. How should we handle the shutdown channel for spawn_monitored_with_backoff
Dzejkop commented 7 months ago
  1. It's a side effect of using tokio and sqlx. tokio uses std::Duration but TIMESTAMPTZ best maps to chrono::DateTime so that sometimes gets converted into chrono::Duration
  2. We actually don't need to delay it - we can have the tasks running without the tree initialised and throwing errors periodically - this would give us better visibility into the initialization period of the sequencer as these errors will show up in our monitoring
  3. If you mean for the purpose of the task ot initialise the tree - we can just start it in the task monitor, no? Then the shutdown sender is handled as before. Also something to note here is that failing to initialize the tree for any other reason than network issues (we do a call to the smart contract for the latest root) should panic and crash the process
ewoolsey commented 7 months ago

We actually don't need to delay it - we can have the tasks running without the tree initialised and throwing errors periodically - this would give us better visibility into the initialization period of the sequencer as these errors will show up in our monitoring

Ah yeah if this is the desired behaviour we can definitely do that. Are we sure we want to throw errors like this though? I usually try and stay away from logging an error when it's expected behaviour, sorta reduces the signal to noise of your errors and can be pretty noisy.

If you think that's the best route though then it's definitely the easiest to do correctly. I can work on that right now if that's what we wanna do.

Dzejkop commented 7 months ago

I feel we should go this way - other option is to do tree initialisation as a separate flow/task but then we'll need to provide custom visibility into it.

We should treat prolonged tree initialisation as an error anyway imo - we should aim for it to be at most 10 seconds or something along those lines.

So yeah, let's do tree initialisation in a TaskMonitor task and have other tasks fail while the tree is uninitialised