Support loading of custom consensus and/or disable automatic consenus retrieval

We've been looking into using Arti as a Tor library for mobile apps. In our settings, the amount of daily data sent (kilobytes) is very small compared to retrieving the consensus (megabytes). To reduce mobile bandwidth load, we made some modifications to Arti to run with a smaller consensus that is updated less frequently. See LightArti.

We are currently running with some modifications to stock Arti to facilitate loading a custom consensus. We made the following changes:

• We removed the code related to fetching the consensus, certificates, and microdescriptors from the network, the static data related to Tor directory authorities, and the code to renew these documents regularly.
• Instead, we customized tor-dirmgr to obtain consensus + descriptor information from files that have been provided out-of-band (in our case, we cache these data for a few days, and just point tor-dirmgr to the directory containing these files)

You can find a (rough) diff wrt an older version of arti here.

It would be fantastic if we could make loading of a custom consensus part of stock Arti so that we can update LightArti to use that interface directly. Here are some of our ideas that we think might work:

• To split the retrival and loading of documents in DirMgr, and to add the ability to load directory information from raw documents, like raw consensus data, or raw microdescriptors data.
• To modify DirCfg to allow the bootstrap to instantiate a DirMgr from pre-loaded documents, maybe in the same way it can customize authorities with the set_authorities method.
• As we not need to refresh the directory info regularly, it could be worth it to "subclass" DirMgr into two "classes", the original one and a more lightweight "subclass" which contains only some code to load directory information from raw data and to use it in the other parts of the library, but without the code to refresh the data periodically from the network.

mentioned in issue #268 (closed)

Thanks for the tickets!

Here's a thought that @eta and I came up with today, though it is likely to need some thought and tweaking before it would work out.

We think that architecturally, the best way to support this would be to let developers "plug in" their own implementations of DirMgr. That is, there could be a trait that looks something like this:

trait DirProvider {
    type EventStream: futures::stream::Stream<Item=DirEvent>;
    fn latest_netdir(&self) -> Option<Arc<NetDir>>;
    fn events(&self) -> Self::EventStream;
    // ... (maybe other stuff?)
}

And it should be possible to plug in a DirProvider in place of our standard DirMgr, using some kind of TorClientBuilder or something like that. That way, you could just write your own DirMgr that would fetch directory information however you think best, filter it however you think best, and then expose that to the rest of the code.

We'd want the trait, and the ability to plug in new implementations, to be behind some "experimental-api" feature, since we expect that this kind of facility will be more unstable than the rest of Arti.

If there were something like that, would you be able to implement your replacement DirMgr in a way that would generate new NetDirs from modified consensus documents and from your block lists? The APIs in tor_netdoc::doc::*::build could help you post-process consensuses to remove whatever relays you wanted to block (for #268 (closed)), or if you wanted to make other changes. We might need to extend those APIs, though. We might also want to expose more of tor-dirmgr so that you could use its pieces to implement your own DirProvider.

What do you think? We're about to go on vacation, but we'll be back around January 6 if you'd like to chat to develop this kind of idea more, or look for other ways to get this to work.

Happy new year!

mentioned in issue #215 (closed)

added Backlog label

changed milestone to %Arti 0.1.0 release: Okay for experimental embedding

assigned to @nickm

(Whoops, that wasn't the ticket I thought it was)

unassigned @nickm

added Sponsor 119 label

added User Feedback label

added API Break label and removed Backlog label

added Backlog label

changed time estimate to 24h

I think this is a good idea to put a trait. So we'll look if we can come up with a first implementation which hides the addition behind a feature flag.

@cgrigis @tharvik

@nickm Hi! We finally have some time to work on this.

We looked a bit deeper into the existing Arti code and how our modifications impact it, and we have a few questions regarding the best way to move forward.

• The DirProvider trait you suggest sounds indeed like the most generic approach, as we would then be able to implement a much simpler DirMgr suited to our needs. However:

  • It looks like this is a non-trivial change, that requires the implementation of the trait, and modification of the existing code so that it uses the trait instead of direct calls to DirMgr. Is that correct, or is it in fact easier than we think? :)
  • Wouldn't keeping that trait itself hidden behind a feature require a fairly large duplication of code?
  • It might be more than we need (tbc :) ).

• In the mean time, in order to port our changes to the latest Arti code as well as best restrict the changes that we need, we have worked on a modified PR (see https://github.com/c4dt/arti/pull/3). As you can see, it is essentially using flat files for the cache instead of sqlite, and conditionally removing some relays from the consensus, along with the addition of a few methods to Authority and UnvalidatedMdConsensus which allow us to to so.

  For the first part, we were thinking of abstracting the sqlite storage with a Store trait, implement our version using flat files, and plugging it instead of sqlite when activating our "experimental" feature.

  For the second part, though, it seems harder: we essentially need to be able to remove some relays deemed unreliable from the consensus. We have not yet found a better place to do this than within the implementation of GetConsensusState::add_from_cache(); if we do it when reading the consensus from the flat file, we end up with an invalid signature on the consensus file itself (we don't have the key to resign at this point). So we will need alternate code activated only by our feature, until we find a better abstraction.

What do you think?

It looks like this is a non-trivial change, that requires the implementation of the trait, and modification of the existing code so that it uses the trait instead of direct calls to DirMgr. Is that correct, or is it in fact easier than we think?

I think it wouldn't be too bad: the only code that we'd have to change would be in arti_client, and the only changes we'd need to make in it would be to change the type of the dirmgr field, and make sure that the trait is defined so that it can provide what arti_client currently needs.

To avoid changing the type of TorClient too much, it might be best to keep the dirmgr in some kind of a BoxedDirProvider field. That's probably okay, since calling the functions on DirMgr shouldn't be critical path. (We'd have to use some adaptors to make a Box<dyn DirProvider>, since it isn't object-safe as-is fwict.)

(It might be best to defer making changes of this kind till after #293 (closed), since that will probably refator client creation a fair bit.)

Wouldn't keeping that trait itself hidden behind a feature require a fairly large duplication of code?

Not necessarily. The idea here would be that the trait exists unconditionally, and is used unconditionally, but that the functions used to create a new TorClient using an instance of this trait would be behind the feature. (Or, they would be public only if the feature was enabled.)

It might be more than we need (tbc :) ).

That's absolutely true: any general solution will be more than you need here. The idea of having a general solution would be to support future extensions and replacements to the directory system, and any extension/replacement mechanism will be more complex than just patching the code. ;)

---

For the first part, we were thinking of abstracting the sqlite storage with a Store trait, implement our version using flat files, and plugging it instead of sqlite when activating our "experimental" feature.

I'd definitely be in favor of a Store trait, if we can do it cleanly (and I think we can!). We'll need one down the line in order to support environments (like wasm-based ones) where sqlite isn't available.

So we will need alternate code activated only by our feature, until we find a better abstraction.

Hm. Maybe it would be handy to have a "preprocess partialnetdir" hook that gets to run on a &mut PartialNetDir before trying to get any microdescriptors. (Or maybe it could be a hook that creates a PartialNetDir from an owned consensus.) Then the API that overrides that hook could be behind a feature.

Would all that work, do you think? (and @eta, what do you think about it?)

I'd be in favour of doing the DirProvider change, and perhaps the Store trait as well; however, I'm somewhat opposed to the idea of adding a bunch of hooks to our existing directory manager code to support extensions, since that seems like it'd be hard for us to support if we refactored the internals in a later release. Swapping the whole thing out sounds like a cleaner approach.

mentioned in issue #20 (closed)

added 1 deleted label

mentioned in merge request !317 (closed)

mentioned in merge request !318 (merged)

Thank you for your detailed answer! I gave a first shot in !318 (merged).

I think it wouldn't be too bad: the only code that we'd have to change would be in arti_client, and the only changes we'd need to make in it would be to change the type of the dirmgr field, and make sure that the trait is defined so that it can provide what arti_client currently needs.

To avoid changing the type of TorClient too much, it might be best to keep the dirmgr in some kind of a BoxedDirProvider field. That's probably okay, since calling the functions on DirMgr shouldn't be critical path. (We'd have to use some adaptors to make a Box<dyn DirProvider>, since it isn't object-safe as-is fwict.)

(It might be best to defer making changes of this kind till after #293 (closed), since that will probably refator client creation a fair bit.)

I started on this, please let me know what you think. It seemed to go OK, but when I rebased on main today, I ran into an issue with the recent refactoring which has a call to self.dirmgr.bootstrap(): the fn is async, and apparently this is not yet supported to be put in a trait :-/ .

Not necessarily. The idea here would be that the trait exists unconditionally, and is used unconditionally, but that the functions used to create a new TorClient using an instance of this trait would be behind the feature. (Or, they would be public only if the feature was enabled.)

So I tried for a while to create a new function to use an instance of DirProvider, but got stuck in type definitions and the way the dirmgr instance is used in TorClient, so for now I have conditionally changed the function, so as to have something where feedback can be given. I will give it another go.

That's absolutely true: any general solution will be more than you need here. The idea of having a general solution would be to support future extensions and replacements to the directory system, and any extension/replacement mechanism will be more complex than just patching the code. ;)

Completely agreed. :) I just wasn't sure it was worth doing such a refactoring now, but as you said it actually has less implications than I thought.

I'd definitely be in favor of a Store trait, if we can do it cleanly (and I think we can!). We'll need one down the line in order to support environments (like wasm-based ones) where sqlite isn't available.

See @tharvik's !317 (closed) .

Hm. Maybe it would be handy to have a "preprocess partialnetdir" hook that gets to run on a &mut PartialNetDir before trying to get any microdescriptors. (Or maybe it could be a hook that creates a PartialNetDir from an owned consensus.) Then the API that overrides that hook could be behind a feature.

Actually, using a completely custom DirProvider implementation, we are able to do everything we need without requiring further hooks. We needed a couple of extra methods on UnvalidatedMdConsensus though.

Would all that work, do you think? (and @eta, what do you think about it?)

Yes, I think this should definitely work. Thank you for your valuable advice and guidance!

I started on this, please let me know what you think. It seemed to go OK, but when I rebased on main today, I ran into an issue with the recent refactoring which has a call to self.dirmgr.bootstrap(): the fn is async, and apparently this is not yet supported to be put in a trait :-/ .

You can add the following in front of a trait to allow async - it comes from the async_trait crate:

#[async_trait]

You'll have to repeat it in the actual implementation of the trait.

You can add the following in front of a trait to allow async - it comes from the trait_async crate:

Yes, thank you, I had seen this possibility from the error message. But I wanted to push the PR before spending more time in that direction. :)

I think the "canonical" version people use is actually called async_trait. Not sure what the other version is about...

I think the "canonical" version people use is actually called async_trait. Not sure what the other version is about...

TIL: trait_async is the older version of async_trait. But as I used the latter in my projects, it's all about bad memory - sorry for the confusion...

Still hoping to merge !347 (merged), but probably not in %Arti 0.1.0 release: Okay for experimental embedding.

removed milestone %Arti 0.1.0 release: Okay for experimental embedding

removed 1 deleted label

Thanks for all the hard work and design! New that we've got !347 (merged) and !318 (merged) done, I think we can merge this.Please reopen if I'm wrong, though...

closed