Skip to content

Draft: tor-dirmgr: refactor the bootstrapping code

eta requested to merge eta/arti:dirmgr-purification into main

This near-completely reworks the structure of the tor-dirmgr bootstrapping code. A laundry list of various improvements:

  • Before, everything had a reference to the DirMgr (in the form of a WriteDir), making the code impure. This primarily made things hard to read and understand, but arguably also complicated things in case of failure (since you could have committed changes to the running DirMgr). This is now fixed; you pass in an Option<NetDir> as an argument to the bootstrapping function, and get back a netdir.
  • Retrying failed downloads now only happens in one place, and follows the algorithm in #433 (ish).
  • The various state machine pieces are now completely internal to the bootstrapping code (the caller doesn't need to know how they work). These are also no longer long-lived.
  • Relatedly, DirState::reset no longer exists. The reset_time() function still does, but it's just used as a timeout for when to give up with the current bootstrapping attempt.
  • DirState::add_from_cache and DirState::add_from_download have been tidied up into one new function, DirState::add_documents.
    • The notion of "returning a boolean if you made progress" has been rectified: now, failing to make progress counts as an error.
  • The various helper functions used to fetch items from the store, make requests, etc. have been tidied up a bit and given more understandable names.
  • We now pass &dyn Store around instead of a mutex.
  • Probably other stuff.

I think this makes the code a fair deal better, but it's hard to know (and also I did kind of fail to remember everything I did when composing the above list :p). Having the functions not pass around a DirMgr everywhere is the main huge benefit in my eyes (and in general, things only pass around what they absolutely need, making the whole structure easier to follow).

That said, this is a relatively sizeable change, and it's also not done yet (things like the dirfilter need to be put back in, as with exiting early and continuing if we have a usable but not complete netdir, ...). It's up to you, the reviewer, as to whether the ends justify the means!

How to review

I'd definitely review this one in side-by-side mode; what I actually did to make this was start an entirely new file for both bootstrap.rs and state.rs and copy code in from the old files, instead of modifying the old code in-place. For this reason, this will probably be best reviewed by just checking out the tree after the patch and taking a look, instead of trying to use the diff.

If you agree this is a decent direction to take us in, I'll rework it into a series of more reasonable, incremental patches.

Merge request reports